RFR (XXS): 8133456: HeapRegionManager::shrink_by() iterates suboptimally across regions

[Tom Benson]

Hi,
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.

Tom

On 8/21/2015 4:38 AM, Thomas Schatzl wrote:
> Hi,
>
> 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.
>
> Thanks,
>    Thomas
>
>