RFR: 8343782: G1: Use one G1CardSet instance for multiple old gen regions [v3]

Tue Dec 3 13:55:49 UTC 2024

On Mon, 2 Dec 2024 12:06:31 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Hi all,
>> 
>> Please review this change to assign multiple collection candidate regions to a single instance of a G1CardSet. Currently, we maintain a 1:1 mapping of old-gen regions and G1CardSet instances, assuming these regions are collected independently. However, regions are collected in batches for performance reasons to meet the G1MixedGCCountTarget.
>> 
>> In this change, at the end of the Remark phase, we batch regions that we anticipate will be collected together into a collection group while selecting remembered set rebuild candidates. Regions in a collection group should be evacuated at the same time because they are assigned to the same G1CardSet instances. This implies that we do not need to maintain cross-region remembered set entries for regions within the same collection group.
>> 
>> The benefit is a reduction in the memory overhead of the remembered set and the remembered set merge time during the collection pause. One disadvantage is that this approach decreases the flexibility during evacuation: you can only evacuate all regions that share a particular G1CardSet at the same time. Another downside is that pinned regions that are part of a collection group have to be partially evacuated when the collection group is selected for evacuation. This removes the optimization in the mainline implementation where the pinned regions are skipped to allow for potential unpinning before evacuation.
>> 
>> In this change, we make significant changes to the collection set implementation as we switch to group selection instead of region selection. Consequently, many of the changes in the PR are about switching from region-centered collection set selection to a group-centered approach.
>> 
>> Note: The batching is based on the sort order by reclaimable bytes which may change the evacuation order in which regions would have been evacuated when sorted by gc efficiency.
>> 
>> We have not observed any regressions on internal performance testing platforms. Memory comparisons for the Cachestress benchmark for different heap sizes are attached below.
>> 
>> Testing: Mach5 Tier1-6
>> 
>> ![16GB](https://github.com/user-attachments/assets/3224c2f1-172d-4d76-ba28-bf483b1b1c95)
>> ![32G](https://github.com/user-attachments/assets/abd10537-41a9-4cf9-b668-362af12fe949)
>> ![64GB](https://github.com/user-attachments/assets/fa87eefc-cf8a-4fb5-9fc4-e7151498bf73)
>> ![128GB](https://github.com/user-attachments/assets/c3a59e32-6bd7-43e3-a3e4-c472f71aa544)
>
> Ivan Walulya has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - Update src/hotspot/share/gc/g1/g1CollectionSet.cpp
>    
>    Co-authored-by: Thomas Schatzl <59967451+tschatzl at users.noreply.github.com>
>  - Update src/hotspot/share/gc/g1/g1_globals.hpp
>    
>    Co-authored-by: Thomas Schatzl <59967451+tschatzl at users.noreply.github.com>
>  - Update src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp
>    
>    Co-authored-by: Thomas Schatzl <59967451+tschatzl at users.noreply.github.com>
>  - Update src/hotspot/share/gc/g1/g1CollectionSet.cpp
>    
>    Co-authored-by: Thomas Schatzl <59967451+tschatzl at users.noreply.github.com>

src/hotspot/share/gc/g1/g1CardSet.cpp line 788:

> 786:   G1HeapRegion* r = G1CollectedHeap::heap()->region_at(region_idx);
> 787:   assert(r->rem_set()->card_set() != this, "must be");
> 788: #endif

Since this introduces local vars, can they be grouped in a `{}` scope?

src/hotspot/share/gc/g1/g1CollectionSet.cpp line 358:

> 356:     }
> 357: 
> 358:     uint num_optional_regions = _optional_groups.num_regions();

Seems unused.

src/hotspot/share/gc/g1/g1CollectionSet.cpp line 516:

> 514: 
> 515:   for (G1CSetCandidateGroup* group : *retained_groups) {
> 516:     assert(group->length() == 1, "Retained groups should have only 1 region");

Should this property be documented in where `_retain_groups` is defined, if it is an invariant?

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

> 318:   }
> 319: 
> 320:   _from_marking_groups.append(current);

I wonder if this part can be written somehow to eliminate some "duplicate" code, so that the following occur only once.

_from_marking_groups.append(current);
current = new G1CSetCandidateGroup(G1CollectedHeap::heap()->card_set_config());
num_added_to_group = 0;

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

> 72:   G1CardSet _card_set;
> 73: 
> 74:   //

Missing comment?

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

> 111:   void clear();
> 112: 
> 113:   void abandon();

It's not obvious how the two APIs differ, and which one to use in a certain scenario. Some docs would be nice.

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

> 170: };
> 171: 
> 172: // Tracks all collection set candidates, i.e. regions that could/should be evacuated soon.

Seems outdated now that fields are group list.

src/hotspot/share/gc/g1/g1_globals.hpp line 285:

> 283:           "may exceed this limit as it is calculated based on G1MixedGCCountTarget.")       \
> 284:           range(1, 256)                                                     \
> 285:                                                                             \

Better wrap text to align ``.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1867639848
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1867744730
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1867758198
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1867699763
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1867657960
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1867718224
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1867760746
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1867636495