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

[Thomas Schatzl]

On Wed, 11 Dec 2024 17:33:24 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 19 additional commits since the last revision:
> 
>  - use reset_table_scanner_for_groups
>  - Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1
>  - Print Group details in G1PrintRegionLivenessInfoClosure
>  - Albert Review 2
>  - Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1
>  - Albert Review
>  - 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>
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/cd28e7cc...554b7f52

Changes requested by tschatzl (Reviewer).

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

> 291: 
> 292:   uint num_added_to_group = 0;
> 293:   // ids 0 and 1 are reserved for region default group and young regions group respectively.

I think this comment should not be here but at the `gid` member. Also, what is a "region default group"?

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

> 74:   size_t _reclaimable_bytes;
> 75:   double _gc_efficiency;
> 76:   const uint _gid;

Please comment what this is as `gid` is not as obvious as the other members. Also not sure if it isn't better to just write out `_group_id`.

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

> 136: 
> 137:   // Delete all groups from the list. The cardset cleanup for regions within the
> 138:   // groups could have been done elsewhere  (e.g. when adding groups to the

Suggestion:

  // groups could have been done elsewhere (e.g. when adding groups to the

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

> 199:   G1CSetCandidateGroupList _from_marking_groups; // Set of regions selected by concurrent marking.
> 200:   // Set of regions retained due to evacuation failure. Groups added to this list
> 201:   // should contain only one region, making it easier to evacuate retained regions

Suggestion:

  // should contain only one region each, making it easier to evacuate retained regions

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

PR Review: https://git.openjdk.org/jdk/pull/22015#pullrequestreview-2501799826
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1883615763
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1883612275
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1883613041
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1883614098