RFR: 8343782: G1: Use one G1CardSet instance for multiple old gen regions
Thomas Schatzl
tschatzl at openjdk.org
Tue Nov 19 09:51:17 UTC 2024
On Mon, 11 Nov 2024 13:58:36 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
>
> 
> 
> 
> 
Initial set of comments.
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 314:
> 312: size_t bytes_to_copy = 0;
> 313: double predicted_eden_time = _policy->predict_young_region_other_time_ms(eden_region_length) +
> 314: _policy->predict_eden_copy_time_ms(eden_region_length, &bytes_to_copy);
`bytes_to_copy` is never used afterwards, so it can be removed (and `predict...` does not need to get it passed.
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 347:
> 345: // without remembered sets after a few attempts to save computation costs of keeping
> 346: // them candidates for very long living pinned regions.
> 347: void G1CollectionSet::finalize_old_part(double time_remaining_ms) {
The comment above is out of date.
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 399:
> 397: uint min_old_cset_length = _policy->calc_min_old_cset_length(candidates()->last_marking_candidates_length());
> 398: uint max_old_cset_length = MAX2(min_old_cset_length, _policy->calc_max_old_cset_length());
> 399: uint max_optional_regions = max_old_cset_length - min_old_cset_length;
Unused.
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 407:
> 405: "Min %u regions, max %u regions, available %u regions"
> 406: "time remaining %1.2fms, optional threshold %1.2fms",
> 407: min_old_cset_length, max_old_cset_length, from_marking_groups->num_regions(), time_remaining_ms, optional_threshold_ms);
Most of these debug messages are regions, it might be useful to add information about groups too. Otherwise it is impossible to understand any grouping issues.
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 423:
> 421: // Add regions to old set until we reach the minimum amount
> 422: if (num_inital_regions < min_old_cset_length) {
> 423:
Suggestion:
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 452:
> 450:
> 451: predicted_initial_time_ms += predicted_time_ms;
> 452:
Suggestion:
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 467:
> 465:
> 466: // Remove selected groups from list of candidate groups.
> 467: if (num_initial_groups > 0) {
Imo it would be clearer to read if this check would be done inside `remove`; it knows if the number of groups is zero. Maybe then the `num_initial_groups` local could go away then too.
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 483:
> 481: "predicted initial time: %1.2fms, predicted optional time: %1.2fms, time remaining: %1.2fms",
> 482: num_inital_regions, num_optional_regions,
> 483: predicted_initial_time_ms, predicted_optional_time_ms, time_remaining_ms);
Again, no information about group selection.
src/hotspot/share/gc/g1/g1CollectionSet.cpp line 620:
> 618: log_debug(gc, ergo, cset) ("Completed with groups, selected %u", num_regions_selected);
> 619: // Remove selected groups from candidate list.
> 620: if (num_groups_selected > 0) {
Maybe the check could be done in `remove`, removing this local.
src/hotspot/share/gc/g1/g1CollectionSet.hpp line 147:
> 145: uint* _collection_set_regions;
> 146: volatile uint _collection_set_cur_length;
> 147: uint _collection_set_max_length;
Maybe there is an existing wrapper somewhere that wraps array/counter/max value somehow. To me it, whenever I read these three, I believe this is the x'th time. Maybe something for another time to factor out.
src/hotspot/share/gc/g1/g1CollectionSet.hpp line 149:
> 147: uint _collection_set_max_length;
> 148:
> 149: // Old gen groups selected for evacuation.
(This is related to the big comment at the top of the file): the comment needs update.
src/hotspot/share/gc/g1/g1CollectionSet.hpp line 187:
> 185:
> 186: void add_group_to_collection_set(G1CSetCandidateGroup* gr);
> 187:
Suggestion:
src/hotspot/share/gc/g1/g1CollectionSet.hpp line 191:
> 189:
> 190: double select_candidates_from_marking(double time_remaining_ms);
> 191:
Suggestion:
src/hotspot/share/gc/g1/g1CollectionSet.hpp line 193:
> 191:
> 192: void select_candidates_from_retained(double time_remaining_ms);
> 193:
Suggestion:
src/hotspot/share/gc/g1/g1CollectionSet.hpp line 195:
> 193:
> 194: // Select regions for evacuation from the optional candidates given the remaining time
> 195: // and return the number of actually selected regions.
Suggestion:
// Select regions for evacuation from the optional candidates given the remaining time
// and return the number of actually selected regions.
src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 56:
> 54: using G1CSetCandidateGroupIterator = GrowableArrayIterator<G1CollectionSetCandidateInfo>;
> 55:
> 56: class G1CSetCandidateGroup : public CHeapObj<mtGCCardSet>{
Document requirements of regions in group.
src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 116:
> 114: // for the first collection group to be as large as G1Policy::calc_min_old_cset_length
> 115: // because we are certain that these regions have to be collected together.
> 116: static const int GROUP_SIZE = 5;
Please make this a diagnostic flag to allow changing this to (essentially) 1 if needed, i.e. if with a low pause time goal and low amount of worker threads one can select between accuracy of keeping pause time goal and efficiency.
With the current logging we can diagnose such a problem (well, at least consider it, see my comments about logging lacking group information), but we can't give a quick solution.
If the user sets a value of "0" may even mean group sizes of 1 including the first group, but that is not necessary imo (maybe as a way to keep the current test though, but that is not very important because the size of that group can be changed with `G1MixedGCCount` or similar)
src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 143:
> 141: void remove_selected(uint count, uint num_regions);
> 142:
> 143: void remove(G1CSetCandidateGroupList* other);
This should add some comments about preconditions, particularly that the regions in the `other` list must be sorted the same way as this list (the original method indicates that, and I do not see a change that allows this now).
I.e. this is not a general purpose `remove` method.
src/hotspot/share/gc/g1/g1Policy.cpp line 499:
> 497: }
> 498: predicted_region_evac_time_ms += gr->predict_group_total_time_ms();
> 499: min_marking_candidates = min_marking_candidates > gr->length() ? (min_marking_candidates - gr->length()) : 0;
Instead of this somewhat complex way of saturating subtraction, maybe it is more clear to have an extra counter summing up already added regions and compare that against the (constant) min_marking_candidates.
src/hotspot/share/gc/g1/g1Policy.cpp line 1135:
> 1133:
> 1134: double G1Policy::predict_region_merge_scan_time(G1HeapRegion* hr, bool for_young_only_phase) const {
> 1135: assert(!hr->is_young(), "Sanity Check!");
Debug code? At least change "Sanity Check" to "must be"; would be nice to at least add region index to the message too.
src/hotspot/share/gc/g1/g1RemSet.cpp line 1398:
> 1396: g1h->collection_set()->merge_cardsets_for_collection_groups(g1h, merge, worker_id, _num_workers);
> 1397:
> 1398: g1h->collection_set_iterate_increment_from(&combined, nullptr, worker_id);
I think it is not necessary any more to use the `combined` closure here. It can be removed afaict, and closure instantiation scoped.
src/hotspot/share/gc/g1/g1YoungCollector.cpp line 300:
> 298: // back memory to the OS keep the most recent amount of memory for these regions.
> 299: if (hr->is_starts_humongous()) {
> 300: guarantee(!hr->rem_set()->has_group_cardset(), "double adding");
"must be" or "humongous regions should not be grouped/do not have group card sets" would be better. Generally the group card set description does not mention that humongous regions do not use the grouping; obvious in hindsight, but it would be nice to document.
-------------
Changes requested by tschatzl (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/22015#pullrequestreview-2436104998
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847968444
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847970300
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847971290
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847972121
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847974605
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847975428
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847977384
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847978186
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847980203
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847983636
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847981291
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847985842
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847986016
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847986437
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847986722
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1848000765
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1842233718
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1847998050
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1848003022
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1848004407
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1848006630
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1848008310
More information about the hotspot-gc-dev
mailing list