RFR (S): 8013872: G1: HeapRegionSeq::shrink_by() has invalid assert
bengt.rutisson at oracle.com
Tue May 7 08:45:09 UTC 2013
Thanks for looking at this!
On 5/7/13 10:29 AM, Thomas Schatzl wrote:
> On Mon, 2013-05-06 at 13:24 -0700, Bengt Rutisson wrote:
>> Hi everyone,
>> Could I have a couple of reviews for this change?
>> 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
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!
More information about the hotspot-gc-dev