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

[Thomas Schatzl]

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.

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.

Thanks,
Thomas