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

[John Cuthbertson]

Hi Bengt,

This looks good. The simplification to use region numbers is inspired.

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
>