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`.

In `G1CollectionSetRegionList::remove` you would need to iterate over all elements that are being removed, which is not the case for now.

The other reason is that `reclaimable_bytes` depends on known live bytes in that region. While currently we exclude regions that may change their contents (e.g. current allocation region) from the collection set, I prefer to be absolutely sure that the values that we are working on do not change and the calculations keep being consistent, i.e. snapshotting the (sum of) reclaimable bytes (one could also snapshot the individual values, but I do not see a gain here).

There does not seem to be any other advantage removing this than not having this additional member (i.e. some simplifications this allows).

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

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