RFR: JDK-8148759: G1AllocRegion::_count inconsistently used if more than one context is active

[Bengt Rutisson]

Hi Thomas,

Thanks for looking at this!

On 2016-03-18 11:14, Thomas Schatzl wrote:
> Hi Bengt,
>
> On Thu, 2016-03-17 at 18:04 +0100, Bengt Rutisson wrote:
>> Hi everyone,
>>
>> Could I have a couple of reviews for this change?
>>
>> http://cr.openjdk.java.net/~brutisso/8148759/webrev.00
>> https://bugs.openjdk.java.net/browse/JDK-8148759
>>
>> The _count value is per G1GCAllocRegion, so if we have more than one
>> survivor alloc region we will notice too late that we have exceeded
>> the maximum number of regions we had decided to use for survivors.
>>
>> The proposed patch uses young_list()->survivor_length() instead,
>> which is a "global" value for all alloc regions. This value used to
>> be updated when we retired a region, but to make proper use of it it
>> needs to be updated when we pick a new survivor region. Thus, I had
>> to move the call to young_list()->add_survivor_region(). I've gone
>> through and checked other users of the values updated by
>> add_survivor_region() and I haven't found anyone that is using this
>> value inside of the actual evacuation path, which is where the
>> difference would be noticeable.
>    we talked about this offline a bit and I can kind of understand the
> reasoning of others to not have this check in G1CollectorPolicy.
>
> I would prefer to move the check into an extra method like
> generation_is_full(InCSetState dest) that is checked instead of
> inlining it (so that it can be easily identified later to be moved in
> some kind of heap sizing/heap shaping or even evacuation policy).

Agreed. Here is an updated webrev:
http://cr.openjdk.java.net/~brutisso/8148759/webrev.01/

And a diff compared to the last one:
http://cr.openjdk.java.net/~brutisso/8148759/webrev.00-01.diff/

I've checked this version with Thomas, Stefan J and Jesper offline. So, 
I'll go ahead an push this now.

Thanks for the reviews everyone!

Bengt


>
> Thanks,
>    Thomas