RFR (S): 8013872: G1: HeapRegionSeq::shrink_by() has invalid assert

[Bengt Rutisson]

Hi John,

On 5/7/13 2:08 AM, John Cuthbertson wrote:
> Hi Bengt,
>
> This looks good. The simplification to use region numbers is inspired.

Thanks! :)

Bengt

>
> JohnC
>
> On 5/6/2013 1:24 PM, Bengt Rutisson wrote:
>>
>> Hi everyone,
>>
>> Could I have a couple of reviews for this change?
>> http://cr.openjdk.java.net/~brutisso/8013872/webrev.00/
>>
>> Background:
>> The method HeapRegionSeq::shrink_by() has an invalid assert. It wants 
>> to assert that we leave at least one region, but it triggers too 
>> early so if we want to shrink down to one single region we hit the 
>> assert.
>>
>> When I started fixing the assert I thought the code was unnecessarily 
>> complicated. The method G1CollectedHeap::shrink_helper() uses both a 
>> number of regions and byte sizes to handle the shrinking. Both of 
>> these concepts are propagated down to HeapRegionSeq::shrink_by(), but 
>> that is not really necessary. It is enough for 
>> HeapRegionSeq::shrink_by() to know about the number of regions. That 
>> simplifies the code quite a bit and removes the need to have asserts 
>> that check that the byte sizes and region counts match.
>>
>> So, rather than just fixing or removing the failing assert I 
>> refactored the code a bit to reduce the need for several of the 
>> asserts. I also changed the increment/decrement_length() methods that 
>> were a bit strange.
>>
>> I think the HeapRegionSeq::expand_by() method can be simplified in a 
>> similar way, but I would like to limit this change to the shrinking.
>>
>> I've tested the changes with JPRT and by running SPECjbb2005 with 
>> -XX:+PrintAdaptiveSizePolicy enabled to see that I get some heap 
>> shrinking.
>>
>> The JTreg test that I am adding only fails on fastdebug or debug 
>> builds before my fix since it is an assert that is failing. 
>> Unfortunately there is no way to control that this test should only 
>> be run on fastdebug or debug builds.
>>
>> Thanks,
>> Bengt
>>
>