RFR(M) 8150721: Don't explicitly manage G1 young regions in YoungList

[Mikael Gerdin]

On 2016-05-03 11:21, Thomas Schatzl wrote:
> Hi Mikael,
>
> On Thu, 2016-04-28 at 14:37 +0200, Mikael Gerdin wrote:
>> Hi,
>>
>> Please review this change to change G1 to stop explicit tracking of
>> all
>> young regions in the YoungList class.
>> With JDK-8150393 the survivor regions are tracked in a
>> GrowableArray.
>> This removes the final dependency on keeping a linked list of young
>> regions in YoungList and this change is a cleanup to get rid of a lot
>> of
>> code related to that.
>>
>> There are a few notable changes due to this cleanup:
>> * During full GCs, calls to uninstall the surv rate group for young
>> regions were moved to TearDownRegionSetsClosure
>> * The young RSet sampling thread can no longer iterate over the young
>> list to sample RSets, it must instead walk the incremental collection
>> set. I would argue that this is more correct anyway since eden
>> regions which had not yet been added to the collection set would not
>> have any interesting RSets anyway.
>>
>> I decided to make eden and survivors two different classes for now,
>> since it's more clear that they are actually distinct that way.
>> The reason for keeping G1EdenRegions around when it's practically
>> empty is that I plan to move the eden SurvRateGroup object these
>> YoungList replacements to get rid of set_region_{eden,survivor}
>>
>> Webrevs:
>> Webrev for entire change
>> http://cr.openjdk.java.net/~mgerdin/8150721/webrev.0.full/
>
>    - there are significant merge errors due to recent changes.

I've rebased the patches
http://cr.openjdk.java.net/~mgerdin/8150721/webrev.0.rebased.full/

>
>    - pre-existing: in G1CollectedHeap::check_young_list_empty() there is
> no need to iterate over the regions if young_regions_count() is already
> zero. The method could also be made ASSERT only (or do the usual
> PRODUCT_ONLY decoration trick).

I suspect that the point of doing the iteration was to get the log_error 
for the regions which were erroneously young.
I can make the method and closure ASSERT only.

>
>    - the comment in line 5144 of g1CollectedHeap.cpp is out of date now.

Removed and slightly reworded the following comment.

>
>    - g1ConcurrentMark.cpp:313: s/comparision/comparison

Done

>
>    - I am not sure about the gain of the assert in
> line g1ConcurrentMark.cpp:313, but I won't object to have it.

The purpose is to ensure that it's safe to cast _claimed_survivor_index 
to uint.

>
>    - g1DefaultPolicy.cpp:399-401: I think the CR is not needed in 399,
> and the comment should be removed as it seems obsolete.

Agree, removed.

>
>    - in a follow-up CR, maybe change the name of youngList.* to
> something fitting the naming scheme.

I'll file a follow-up CR for the renaming and check with Stefan that 
he's ok with that approach.

>
>    - the includes in youngList.cpp should be trimmed down.

Will do.

/Mikael

>
> Thanks,
>    Thomas
>