RFR (XXS): 8133456: HeapRegionManager::shrink_by() iterates suboptimally across regions
tom.benson at oracle.com
Fri Aug 21 12:03:41 UTC 2015
The change looks good to me.
On the in/out parameter question, I'd say I slightly favor the two
parameters. On a related trivial note, I'd also prefer it slightly if
find_available_from_idx_reverse had the index as return value, with the
count as the output parameter. Same for find_unavailable_from_idx.
Then they would be consistent with find_contiguous and
find_highest_free, and there wouldn't be 2 index params to look odd.
But that's another change.
On 8/21/2015 4:38 AM, Thomas Schatzl wrote:
> 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.
>> 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