RFR: 8140326: G1: Consider putting regions where evacuation failed into next collection set [v2]

[Ivan Walulya]

On Mon, 5 Jun 2023 10:06:42 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> This change adds management of retained regions, i.e. trying to evacuate evacuation failed regions asap.
>> 
>> The advantage is that evacuation failed regions do not need to wait until the next marking to be cleaned out; as they are often very sparsely occupied (often being eden regions), this occupies a lot of space, potentially causing additional evacuation failures later on.
>> Another use of this change will be region pinning, which are basically evacuation failed regions that can not be reclaimed as long as they are pinned - however as soon as they are unpinned, they should be reclaimed for the same reasons as well.
>> 
>> It consists of several behavioral changes:
>> 
>> During garbage collection:
>> 
>> ... in the Evacuation phase:
>> * always collect dirty cards into evacuation failed regions proactively. In tests, the amount of cards/live objects per evacuation failed region is typically very small. Dirty cards are always put into the global refinement buffer immediately, assuming that we will keep most if not all evacuation failed regions.
>> 
>> ... during Post Evacuation 2/Free Collection Set phase:
>> * determine whether the region will be retained (kept for "immediate" evacuation) or not. Highly occupied regions are assumed to stay (mostly) live at least until the next marking, so do not bother with them.
>> 
>> These "retained" regions are collected in a new "from retained" set in the collection set candidates and managed separately from "from marking" regions. Having the "from retained" and "from marking" sets separate in the collection set candidates is easier to manage than to use a single list and the picking stuff from it. Particularly wrt to making sure that mixed gcs preferentially pick from the "from marking" list first, then second from the "from retained" list.
>> 
>> ... determining the collection set during the pause:
>>   * during gc, the collection set is preferentially (first) populated with regions from the "from marking" candidates (these are the important regions to get cleaned out), second from the "from retained" list.
>>   * g1 reserves up to 20% of max gc pause time for retained regions as optional candidates (this is a random number) to make sure that these are cleared out asap to free memory. There is also a minimum number of regions to take from the retained regions list.
>> 
>> During marking
>> 
>> ... changes to marking proper
>> * retained regions will not be marked through during concurrent mark, i.e. they are considered outside of ...
>
> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Doc additions

Changes requested by iwalulya (Reviewer).

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

> 215: 
> 216: void G1CollectionSetCandidates::remove(G1CollectionCandidateRegionList* other) {
> 217:   // During removal we exploit the fact that elements in the marking_regions,

Suggestion:

  // During removal, we exploit the fact that elements in the marking_regions,

src/hotspot/share/gc/g1/g1CollectionSetChooser.cpp line 35:

> 33: 
> 34: // Determine collection set candidates (from marking): For all regions determine
> 35: // whether they should be a collection set candidates, calculate their efficiency,

Suggestion:

// whether they should be collection set candidates, calculate their efficiency,

src/hotspot/share/gc/g1/g1CollectionSetChooser.cpp line 153:

> 151:       // alloc region (we should not consider those for collection
> 152:       // before we fill them up).
> 153:       if (should_add(r) && !G1CollectedHeap::heap()->is_old_gc_alloc_region(r)) {

Pre-existing: why not move the `!G1CollectedHeap::heap()->is_old_gc_alloc_region(r)` to `should_add` helper?

src/hotspot/share/gc/g1/g1CollectionSetChooser.cpp line 155:

> 153:       if (should_add(r) && !G1CollectedHeap::heap()->is_old_gc_alloc_region(r)) {
> 154:         add_region(r);
> 155:         assert(r->rem_set()->is_complete(), "must be %u", r->hrm_index());

Maybe the assert comes before the `add_region(r);`

src/hotspot/share/gc/g1/g1ParScanThreadState.hpp line 115:

> 113:   // the local DCQS. This is an approximation, as cards that would be added later
> 114:   // outside of evacuation failure will not be subtracted again.
> 115:   size_t _evac_failure_enqeued_cards;

Suggestion:

  size_t _evac_failure_enqueued_cards;

src/hotspot/share/gc/g1/g1Policy.cpp line 1534:

> 1532: void G1Policy::select_candidates_from_retained(G1CollectionCandidateList* retained_list,
> 1533:                                                double time_remaining_ms,
> 1534:                                                G1CollectionCandidateRegionList* initial_old_regions,

Might be called on empty `retained_list`, probably better to have an early return. Same should be added to `select_candidates_from_retained`

src/hotspot/share/gc/g1/g1Policy.cpp line 1544:

> 1542:   uint num_optional_regions_selected = 0;
> 1543:   uint num_expensive_regions_selected = 0;
> 1544:   uint num_unreclaimable_regions = 0;

how is `num_unreclaimable_regions` set?

test/hotspot/jtreg/gc/g1/TestVerifyGCType.java line 147:

> 145:         output.shouldHaveExitValue(0);
> 146: 
> 147: System.out.println(output.getStdout());

seems like leftover from debugging

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

PR Review: https://git.openjdk.org/jdk/pull/14220#pullrequestreview-1461900812
PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1217759095
PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1217831930
PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1217847077
PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1217835890
PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1217851534
PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1217658060
PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1217696095
PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1217873522