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

[Thomas Schatzl]

On Sat, 6 May 2023 22:38:36 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/g1CollectionSetCandidates.hpp line 46:
> 
>> 44: class G1CollectionSetRegionList {
>> 45:   GrowableArray<HeapRegion*> _regions;
>> 46:   size_t _reclaimable_bytes;
> 
> I don't see the necessity of  `G1CollectionSetRegionList::_reclaimable_bytes`. Seems to me, one can calculate it on the fly in the for-loop of `G1CollectionSetCandidates::remove`.

(After deleting the other message)
There is a use in `G1CollectionSetRegionList::remove` where not having this value would add a loop over the `other` list. If you insist, I can change that.

> src/hotspot/share/gc/g1/g1CollectionSetChooser.cpp line 256:
> 
>> 254:     candidates->merge_candidates_from_marking(_result.array(),
>> 255:                                               _num_regions_added - num_pruned,
>> 256:                                               _reclaimable_bytes_added - pruned_wasted_bytes);
> 
> Could `prune` modify `_result` and fields in-place? Requiring caller to do `_num_regions_added - num_pruned` seems an unnecessary overhead.

Okay, changed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188379382
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188379944