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

[Mikael Gerdin]

Hi again,

On 2016-05-03 13:43, Mikael Gerdin wrote:
>
>
> 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.

New webrevs are available:
Incremental
http://cr.openjdk.java.net/~mgerdin/8150721/webrev.0_to_1/

Full
http://cr.openjdk.java.net/~mgerdin/8150721/webrev.1.full/

The incremental webrev is based on webrev.0.rebased.full.

/Mikael

>
> /Mikael
>
>>
>> Thanks,
>>    Thomas
>>