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

[Thomas Schatzl]

On Tue, 2 May 2023 07:49:42 GMT, Albert Mingkun Yang <ayang 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
>
> src/hotspot/share/gc/g1/g1CollectionSet.cpp line 328:
> 
>> 326:     assert(_optional_old_regions.length() == 0, "must be");
>> 327: 
>> 328:     if (collector_state()->in_mixed_phase()) {
> 
> Why checking the same condition again (L322 the first time)?

In https://bugs.openjdk.org/browse/JDK-8140326 the first condition will change to something like "are there collection set candidates" and retained regions will be added later.
Will remove.

> src/hotspot/share/gc/g1/g1CollectionSet.cpp line 329:
> 
>> 327: 
>> 328:     if (collector_state()->in_mixed_phase()) {
>> 329:       time_remaining_ms = _policy->select_candidates_from_marking(&candidates()->marking_regions(),
> 
> `time_remaining_ms` seems unused after the assignment.

Same reason as above. Later changes will need/use this. Removed.

> src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp line 47:
> 
>> 45: }
>> 46: 
>> 47: void G1CollectionCandidateList::append_unsorted(HeapRegion* r) {
> 
> Some methods in this file seem never used.

They are used in https://bugs.openjdk.org/browse/JDK-8140326 . I will look through and remove unused ones.

> src/hotspot/share/gc/shared/ptrQueue.hpp line 43:
> 
>> 41: class BufferNode;
>> 42: class PtrQueueSet;
>> 43: class PtrQueue : public CHeapObj<mtGC> {
> 
> Why is this required?
> 
> (Seems to work fine without it when I tried it.)

Required for https://bugs.openjdk.org/browse/JDK-8140326. Will remove.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1182448685
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1182449132
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1182451617
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1182452932