RFR (XXS): 8133456: HeapRegionManager::shrink_by() iterates suboptimally across regions
thomas.schatzl at oracle.com
Fri Aug 21 08:38:42 UTC 2015
On Fri, 2015-08-21 at 10:27 +0200, Bengt Rutisson wrote:
> Hi Thomas,
> On 2015-08-21 10:05, Thomas Schatzl wrote:
> > Hi all,
> > can I have reviews for this one-line change that improves efficiency
> > during heap shrinking?
> > The problem was that the increment to skip over already scanned regions
> > has been too conservative. In some cases it would mean that regions in
> > the region table would be scanned twice.
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8133456
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8133456/webrev/
> The fix looks good, but do we really need both cur and idx_last_found?
> After your fix we just set them to the same value. Technically it would
> be enough that find_empty_from_idx_reverse() takes one parameter that is
> both the in and out value. But it may be cleaner to have the two separated.
> I'll leave it up to you if you want to change it. I'm fine with the
> proposed patch too.
I would be fine with either way too, but I have received some review
comments in the past (recently about 8067336 where originally I had one
in and one inout parameter instead of three parameters (in/in/out) for
all methods) to avoid inout parameters since it is easier to understand.
Let's get another opinion.
More information about the hotspot-gc-dev