RFR: 8306541: Refactor collection set candidate handling to prepare for JDK-8140326 [v3]

[Ivan Walulya]

On Tue, 2 May 2023 13:41:28 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this refactoring of collection set candidate set handling.
>> 
>> The idea is to improve the interface to collection set candidates and prepare for having collection set candidates available at any time to evacuate them at any young collection.
>> 
>> These preparations to allow for multiple sources for these candidates (from the marking, as now, and from retained regions, i.e. evacuation failed regions as per [JDK-8140326](https://bugs.openjdk.org/browse/JDK-8140326)).
>> 
>> This patch only uses candidates from marking at this time.
>>  
>> Also moves gc efficiency out of HeapRegion and associate it to the list element as it's not used otherwise.
>>     
>> In detail:
>>     * the collection set candidates set is not temporarily allocated any more, but the candidate collection set object is available all the time.
>>     
>>     * G1CollectionSetCandidates is the main class, representing the current candidates. Contains the "from marking" candidate list only (at this point).
>>     
>>     * there are several additional helper sets/lists
>>       * G1CollectionSetRegionList: list of HeapRegion*, typically sorted by efficiency (but not necessarily). Also does not contain gc efficiences.
>>       * G1CollectionCandidateList: list of candidates, i.e. HeapRegion* with their gc efficiency. Building block for the actual collection set candidates list.
>>     
>> All these sets implement C++ iterators for simpler use in various places.
>>     
>> Testing:
>>  - this patch only: tier1-3, gha
>>  - with JDK-8140326 tier1-7 (or 8?)
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'master' into 8306541-refactor-cset-candidates
>  - ayang review - remove unused methods
>  - Whitespace fixes
>  - typo
>  - More cleanup
>  - Cleanup
>  - Cleanup
>  - Refactor collection set candidates
>    
>    Improve the interface to collection set candidates and prepare for having collection set
>    candidates at any time. Preparations to allow for multiple sources for these candidates
>    (from the marking, as now, and from retained, i.e. evacuation failed regions). This patch
>    only uses candidates from marking at this time.
>    
>    Also moves gc efficiency out of HeapRegion and associate it to the list element as it's
>    not used otherwise.
>    
>    * the collection set candidates set is not temporarily allocated any more, but the candidate
>    set object must be available all the time.
>    
>    * G1CollectionSetCandidates is the main class, representing the current candidates. Contains
>      the "from marking" candidate list only (at this point).
>    
>    * there are several additional helper sets/lists
>      * G1CollectionSetRegionList: list of HeapRegion*, typically sorted by efficiency (but not
>        necessarily). Also does not contain gc efficiences.
>      * G1CollectionCandidateList: list of candidates, i.e. HeapRegion* with their gc efficiency.
>        Building block for the actual collection set candidates list.
>    
>    All these sets implement C++ iterators for simpler use in various places.
>    
>    Everything else are changes to use these helper sets/lists throughout.
>    
>    Some additional FIXME for log messages to remove are in there. Please ignore.

src/hotspot/share/gc/g1/g1CollectionSet.hpp line 155:

> 153:   // When doing mixed collections we can add old regions to the collection set, which
> 154:   // will be collected only if there is enough time. We call these optional regions.
> 155:   // This member records the current number of regions that are of that type that

Comment needs to be revised

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp line 50:

> 48:   guarantee((uint)_candidates.length() >= other->length(), "must be");
> 49: 
> 50:   if ((other->length() == 0) || (_candidates.length() == 0)) {

`guarantee((uint)_candidates.length() >= other->length(), "must be");` implies that the second part of the predicate is not necessary i.e  `|| (_candidates.length() == 0)`

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1183278338
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1183285839