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

[Thomas Schatzl]

On Sat, 6 May 2023 21:22:27 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
>> 
>>  - Merge branch 'master' into 8306541-refactor-cset-candidates
>>  - ayang, iwalulya review
>>    
>>    fix inlining in g1CollectionSet.inline.hpp
>>  - 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/g1CollectionSetChooser.cpp line 198:
> 
>> 196:       if (should_add(r) && !G1CollectedHeap::heap()->is_old_gc_alloc_region(r)) {
>> 197:         add_region(r);
>> 198:       } else if (r->is_old() && !r->is_collection_set_candidate()) {
> 
> Why the additional predicate? (IOW, what regions will be misplaced without the new predicate?)

That is a change that is necessary later - when pinned/evacuation failure regions are part of the candidates, they show up here. Will remove for now. Apologies.

> src/hotspot/share/gc/g1/heapRegion.inline.hpp line 301:
> 
>> 299:   if (is_old_or_humongous() && !is_collection_set_candidate()) {
>> 300:     set_top_at_mark_start(top());
>> 301:   }
> 
> Unclear why these checks are required.

Same as above, some change necessary for later.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188310565
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188311135