RFR: 8306541: Refactor collection set candidate handling to prepare for JDK-8140326 [v5]
Albert Mingkun Yang
ayang at openjdk.org
Mon May 8 08:25:30 UTC 2023
On Wed, 3 May 2023 15:35:20 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 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.cpp line 229:
> 227: verify();
> 228:
> 229: _marking_regions.merge(candidate_infos, num_infos);
Could we avoid `merge` in the name? It suggests there's existing data there already. Maybe "populate_marking_candidates" or sth.
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`.
src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 55:
> 53: // Remove the given list of HeapRegion* from this list. Assumes that the given
> 54: // list is a prefix of this list.
> 55: void remove(G1CollectionSetRegionList* list);
Maybe `remove_prefix`?
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?)
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.
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1186746076
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1186754322
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1186745526
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1186747757
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1186747085
PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1186748274
More information about the hotspot-dev
mailing list