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

[Albert Mingkun Yang]

On Tue, 9 May 2023 11:10:27 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 incrementally with one additional commit since the last revision:
> 
>   iwalulya review

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

> 272:   verify_helper(&_marking_regions, from_marking, reclaimable_bytes, verify_map);
> 273: 
> 274:   assert(length() >= marking_regions_length(), "must be");

Don't get what the intention is here, given `uint length() const { return marking_regions_length(); }`.

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 43:

> 41: 
> 42: // A set of HeapRegion*.
> 43: class G1CollectionSetRegionList {

Now that this is just a region-list, maybe drop the "CollectionSet" part?

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 65:

> 63:   G1CollectionSetRegionListIterator end() const { return _regions.end(); }
> 64: 
> 65:   void verify() PRODUCT_RETURN;

Seems unimplemented.

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 181:

> 179:   uint _last_marking_candidates_length;
> 180: 
> 181:   size_t _reclaimable_bytes;

Where is this used other than asset?

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 214:

> 212: 
> 213:   bool is_empty() const;
> 214:   bool has_no_more_marking_candidates() const;

Maybe the positive variant, sth like `has_marking_candidates`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188615037
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188572380
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188571518
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188596562
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188606695