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

[Bengt Rutisson]

Hi Thomas,

Thanks for looking at this!

On 5/7/13 10:29 AM, Thomas Schatzl wrote:
> Hi,
>
> On Mon, 2013-05-06 at 13:24 -0700, 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.
> Is there a CR/RFE for this?
>
>> 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 for cleaning that up, and thanks for your detailed analyses.
>
> One nit:
>
> - test: ... without having allocated many object*s*.. ; ... we will
> shrink to one region -> ... the GC will shrink it (or: the heap) to one
> region.

Good catch! Fixed.

> You may also fix Min/MaxHeapFreeRatio to some low values to force the
> shrinking in case the default values change, but I think it's fine.

Good point. But I'll leave it out for now.


Thanks again for looking at this!
Bengt

>
> Thanks,
> Thomas
>