RFR (S): 8013872: G1: HeapRegionSeq::shrink_by() has invalid assert
thomas.schatzl at oracle.com
Tue May 7 08:29:26 UTC 2013
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.
- test: ... without having allocated many object*s*.. ; ... we will
shrink to one region -> ... the GC will shrink it (or: the heap) to one
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.
More information about the hotspot-gc-dev