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

Thomas Schatzl tschatzl at openjdk.org
Mon Dec 2 09:31:41 UTC 2024 

On Wed, 20 Nov 2024 19:23:34 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 one additional commit since the last revision:
> 
>   Thomas Review

Sorry for the late reply. Some more comments need update, other than that it seems fine.

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

> 340: //   regions in retained collection set candidates Retained collection set candidates are aged out, ie.
> 341: //   made to regular old regions without remembered sets after a few attempts to save computation costs
> 342: //   of keeping them candidates for very long living pinned regions.

Suggestion:

// The current mechanism for evacuating pinned old regions is as below:
// * pinned regions in the marking collection set candidate list (available during mixed gc) are evacuated like
//   pinned young regions to avoid the complexity of dealing with pinned regions that are part of a
//   collection group sharing a single cardset. These regions will be partially evacuated and added to the
//   retained collection set by the evacuation failure handling mechanism.
// * evacuating pinned regions out of retained collection set candidates would also just take up time
//   with no actual space freed in old gen. Better to concentrate on others. So we skip over pinned
//   regions in retained collection set candidates. Retained collection set candidates are aged out, ie.
//   made to regular old regions without remembered sets after a few attempts to save computation costs
//   of keeping them candidates for very long living pinned regions.

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

> 520:     bool fits_in_remaining_time = predicted_time_ms <= time_remaining_ms;
> 521: 
> 522:     G1CollectionSetCandidateInfo* ci = group->at(0); // we only have one region in the group

Suggestion:

    G1CollectionSetCandidateInfo* ci = group->at(0); // We only have one region in the group.

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

> 63: // All regions in the group share a G1CardSet instance, which tracks remembered set entries for the
> 64: // regions in the group. We do not have track to cross-region references for regions that are in the
> 65: // same group.

Suggestion:

// G1CSetCandidateGroup groups candidate regions that will be selected for evacuation at the same time.
// Grouping occurs both for candidates from marking or regions retained during evacuation failure, but a group
// can not contain regions from both types of regions.
//
// Humongous objects are excluded from the candidate groups because regions associated with these
// objects are never selected for evacuation.
//
// All regions in the group share a G1CardSet instance, which tracks remembered set entries for the
// regions in the group. We do not have track to cross-region references for regions that are in the
// same group saving memory.

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

> 281:           "The maximum number of old CSet regions in a collection group. "  \
> 282:           "These will be evacuated in the same GC pause. The first group "  \
> 283:           "may exceed this limit depending on G1MixedGCCountTarget.")       \

Maybe this is better, not sure. We should explain why the "first group" is special.

Suggestion:

  product(uint, G1OldCSetGroupSize, 5, EXPERIMENTAL,         \
          "The maximum number of old CSet regions in a collection group. "  \
          "All regions in a group will be evacuated in the same GC pause. The first group calculated after marking from marking candidates "  \
          "may exceed this limit as it is calculated based on G1MixedGCCountTarget.")       \

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

Changes requested by tschatzl (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22015#pullrequestreview-2472141691
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1865490259
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1865503030
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1865494143
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1865498063

More information about the hotspot-gc-dev mailing list