8343782: G1: Use one G1CardSet instance for multiple old gen regions #22015

walulyai · 2024-11-11T13:58:36Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8343782: G1: Use one G1CardSet instance for multiple old gen regions (Enhancement - P4)

Reviewers

Thomas Schatzl (@tschatzl - Reviewer) Review applies to ff5e9e04
Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22015/head:pull/22015
$ git checkout pull/22015

Update a local copy of the PR:
$ git checkout pull/22015
$ git pull https://git.openjdk.org/jdk.git pull/22015/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22015

View PR using the GUI difftool:
$ git pr show -t 22015

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22015.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-11T13:59:16Z

👋 Welcome back iwalulya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-11-11T14:00:02Z

@walulyai This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8343782: G1: Use one G1CardSet instance for multiple old gen regions

Reviewed-by: ayang, tschatzl

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 18 new commits pushed to the master branch:

6b994cd: 8333697: C2: Hit MemLimit in PhaseCFG::global_code_motion
2ff8440: 8349344: Clarify documentation of Arena.ofConfined
19399d2: 8348572: C2 compilation asserts due to unexpected irreducible loop
6146588: 8348190: Framework for tracing makefile inclusion and parsing
66a3898: 8348659: AArch64: IR rule failure with compiler/loopopts/superword/TestSplitPacks.java
40603a5: 8349214: Improve size optimization flags for MSVC builds
0926949: 8347629: Test FailOverDirectExecutionControlTest.java fails with -Xcomp
a51e669: 8349200: [JMH] time.format.ZonedDateTimeFormatterBenchmark fails
9d23de5: 8184352: Remove Sun provider information from KeyPairGenerator javadoc
d222c18: 8349107: Remove RMI finalizers
... and 8 more: https://git.openjdk.org/jdk/compare/beae8843b9b2433af5e9fbe420b17e663cbdb960...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-11T14:00:55Z

@walulyai The following label will be automatically applied to this pull request:

hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-11-11T15:35:29Z

Webrevs

tschatzl

Initial set of comments.

tschatzl · 2024-11-14T13:36:09Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

+  // Limit to the number regions in a collection group. We make an exception
+  // for the first collection group to be as large as G1Policy::calc_min_old_cset_length
+  // because we are certain that these regions have to be collected together.
+  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)

tschatzl · 2024-11-19T09:23:55Z

src/hotspot/share/gc/g1/g1CollectionSet.cpp

+  size_t bytes_to_copy = 0;
  double predicted_eden_time = _policy->predict_young_region_other_time_ms(eden_region_length) +
-                               _policy->predict_eden_copy_time_ms(eden_region_length);
+                               _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.

tschatzl · 2024-11-19T09:25:03Z

src/hotspot/share/gc/g1/g1CollectionSet.cpp

 //   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.
 void G1CollectionSet::finalize_old_part(double time_remaining_ms) {


The comment above is out of date.

tschatzl · 2024-11-19T09:25:41Z

src/hotspot/share/gc/g1/g1CollectionSet.cpp

-  const uint max_optional_regions = max_old_cset_length - min_old_cset_length;
+  uint min_old_cset_length = _policy->calc_min_old_cset_length(candidates()->last_marking_candidates_length());
+  uint max_old_cset_length = MAX2(min_old_cset_length, _policy->calc_max_old_cset_length());
+  uint max_optional_regions = max_old_cset_length - min_old_cset_length;


tschatzl · 2024-11-19T09:26:14Z

src/hotspot/share/gc/g1/g1CollectionSet.cpp

                            "Min %u regions, max %u regions, available %u regions"
                            "time remaining %1.2fms, optional threshold %1.2fms",
-                            min_old_cset_length, max_old_cset_length, marking_list->length(), time_remaining_ms, optional_threshold_ms);
+                            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.

tschatzl · 2024-11-19T09:43:09Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

-
-  G1CollectionCandidateListIterator& operator++();
-  G1CollectionSetCandidateInfo* operator*();
+class G1CSetCandidateGroup : public CHeapObj<mtGCCardSet>{


Document requirements of regions in group.

tschatzl · 2024-11-19T09:44:24Z

src/hotspot/share/gc/g1/g1Policy.cpp

-    predicted_region_evac_time_ms += predict_region_total_time_ms(ci->_r, false /* for_young_only_phase */);
-    min_marking_candidates--;
+    predicted_region_evac_time_ms += gr->predict_group_total_time_ms();
+    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.

tschatzl · 2024-11-19T09:45:19Z

src/hotspot/share/gc/g1/g1Policy.cpp

 }

 double G1Policy::predict_region_merge_scan_time(G1HeapRegion* hr, bool for_young_only_phase) const {
+  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.

tschatzl · 2024-11-19T09:46:39Z

src/hotspot/share/gc/g1/g1RemSet.cpp


+        g1h->collection_set()->merge_cardsets_for_collection_groups(g1h, merge, worker_id, _num_workers);
+
        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.

tschatzl · 2024-11-19T09:47:41Z

src/hotspot/share/gc/g1/g1YoungCollector.cpp

      // Sample card set sizes for humongous before GC: this makes the policy to give
      // back memory to the OS keep the most recent amount of memory for these regions.
      if (hr->is_starts_humongous()) {
+        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.

tschatzl

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

src/hotspot/share/gc/g1/g1CollectionSet.cpp

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

src/hotspot/share/gc/g1/g1_globals.hpp

src/hotspot/share/gc/g1/g1CollectionSet.cpp

Co-authored-by: Thomas Schatzl <59967451+tschatzl@users.noreply.github.com>

albertnetymk · 2024-12-03T12:26:48Z

src/hotspot/share/gc/g1/g1_globals.hpp

+  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.")       \
+          range(1, 256)                                                     \
+                                                                            \


Better wrap text to align \.

albertnetymk · 2024-12-03T12:29:01Z

src/hotspot/share/gc/g1/g1CardSet.cpp

+#ifdef ASSERT
+  uint region_idx = card_region >> config()->log2_card_regions_per_heap_region();
+  G1HeapRegion* r = G1CollectedHeap::heap()->region_at(region_idx);
+  assert(r->rem_set()->card_set() != this, "must be");
+#endif


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

It's possible, but I have not seen this done in the hotspot code.

With a quick search, I can find some in runtime code, though not universal. Regardless, it's better encapsulation, IMO.

albertnetymk · 2024-12-03T12:41:48Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

+  G1CardSet _card_set;

-  GrowableArray<G1CollectionSetCandidateInfo> _candidates;
+  //


Missing comment?

albertnetymk · 2024-12-03T13:12:12Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp

+  uint num_added_to_group = 0;
+  G1CSetCandidateGroup* current = nullptr;
+
+  current = new G1CSetCandidateGroup(G1CollectedHeap::heap()->card_set_config());
+
  for (uint i = 0; i < num_infos; i++) {
    G1HeapRegion* r = candidate_infos[i]._r;
    assert(!contains(r), "must not contain region %u", r->hrm_index());
    _contains_map[r->hrm_index()] = CandidateOrigin::Marking;
+
+    if (num_added_to_group == group_limit) {
+      if (group_limit != G1OldCSetGroupSize) {
+        group_limit = G1OldCSetGroupSize;
+      }
+
+      _from_marking_groups.append(current);
+
+      current = new G1CSetCandidateGroup(G1CollectedHeap::heap()->card_set_config());
+      num_added_to_group = 0;
+    }
+    current->add(candidate_infos[i]);
+    num_added_to_group++;
  }
+
+  _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;

Suggestions are welcome, I failed to find a way to handle the corner case.

albertnetymk · 2024-12-03T13:24:55Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

+  void clear();
+
+  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.

albertnetymk · 2024-12-03T13:43:04Z

src/hotspot/share/gc/g1/g1CollectionSet.cpp

-    // in that list must have been pinned for at least G1NumCollectionsKeepPinned
-    // GCs and hence are considered "long lived".
-    drop_pinned_retained_regions(&pinned_retained_regions);
+    uint num_optional_regions = _optional_groups.num_regions();


Seems unused.

albertnetymk · 2024-12-03T13:51:09Z

src/hotspot/share/gc/g1/g1CollectionSet.cpp

+  G1CSetCandidateGroupList groups_to_abandon;
+
+  for (G1CSetCandidateGroup* group : *retained_groups) {
+    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?

albertnetymk · 2024-12-03T13:52:46Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

+  void iterate(Func&& f) const;
 };

 // Tracks all collection set candidates, i.e. regions that could/should be evacuated soon.


Seems outdated now that fields are group list.

albertnetymk · 2024-12-06T12:22:02Z

src/hotspot/share/gc/g1/g1HeapRegion.cpp

+  if (is_young() || is_free()) {
+    return -1.0;
+  }


I don't get why young-regions are treated specially. Also, it's weird that "free" region needs to have a gc-efficiency.

albertnetymk · 2024-12-06T12:43:05Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp


-  GrowableArrayFromArray<G1CollectionSetCandidateInfo> a(candidate_infos, (int)num_infos);
-  _candidates.appendAll(&a);
+void G1CSetCandidateGroup::add(G1HeapRegion* hr) {


I believe this method is only for retained regions; if so, one can make that explicit by naming it sth like add_region_region.

(Probably add_retained_region was meant here?)

Currently, we are using the method for adding young regions too

albertnetymk · 2024-12-06T12:46:38Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp

-}
+void G1CSetCandidateGroup::add(G1CollectionSetCandidateInfo& hr_info) {
+  G1HeapRegion* hr = hr_info._r;
+  assert(!hr->is_young(), "should be flagged as survivor region");


Can one assert region is Old here?

albertnetymk · 2024-12-06T13:02:25Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp

-  G1CollectionCandidateRegionList other_retained_regions;
+  G1CSetCandidateGroupList other_marking_groups;
+  G1CSetCandidateGroupList other_retained_groups;
+


Extra blank line.

albertnetymk · 2024-12-06T13:59:11Z

src/hotspot/share/gc/g1/g1CollectionSet.inline.hpp

+#include "gc/g1/g1HeapRegionRemSet.hpp"
+
+template <class CardOrRangeVisitor>
+inline void G1CollectionSet::merge_cardsets_for_collection_groups(G1CollectedHeap* g1h, CardOrRangeVisitor& cl, uint worker_id, uint num_workers) {


The first arg seems unused.

albertnetymk · 2024-12-06T21:46:08Z

src/hotspot/share/gc/g1/g1CollectionSet.cpp

+  for (G1CollectionSetCandidateInfo ci : *gr) {
+    G1HeapRegion* r = ci._r;
+    r->uninstall_group_cardset();
+    r->rem_set()->set_state_complete();


Why changing the remset state here? I'd expect it's already complete; otherwise, how can it be added to cset?

Maybe change to assert?

albertnetymk · 2024-12-06T22:54:53Z

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

  CodeCache::arm_all_nmethods();
 }

 void G1CollectedHeap::prepare_group_cardsets_for_scan () {


Pre-existing: extra space.

albertnetymk · 2024-12-06T22:57:33Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp

+
+void G1CSetCandidateGroupList::prepare_for_scan() {
+  for (G1CSetCandidateGroup* gr : _groups) {
+    gr->card_set()->reset_table_scanner();


This is a group card set, so why not calling reset_table_scanner_for_groups?

albertnetymk · 2024-12-09T09:04:39Z

src/hotspot/share/gc/g1/g1CardSet.cpp

+#ifdef ASSERT
+  uint region_idx = card_region >> config()->log2_card_regions_per_heap_region();
+  G1HeapRegion* r = G1CollectedHeap::heap()->region_at(region_idx);
+  assert(r->rem_set()->card_set() != this, "must be");
+#endif


With a quick search, I can find some in runtime code, though not universal. Regardless, it's better encapsulation, IMO.

tschatzl · 2024-12-13T09:14:56Z

src/hotspot/share/gc/g1/g1CollectionSet.cpp

+  for (G1CollectionSetCandidateInfo ci : *gr) {
+    G1HeapRegion* r = ci._r;
+    r->uninstall_group_cardset();
+    r->rem_set()->set_state_complete();


Maybe change to assert?

tschatzl · 2024-12-13T09:17:53Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

+  size_t _reclaimable_bytes;
  double _gc_efficiency;
-
+  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.

tschatzl · 2024-12-13T09:18:27Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

-
-  void abandon();
+  // Delete all groups from the list. The cardset cleanup for regions within the
+  // groups could have been done elsewhere  (e.g. when adding groups to the


Suggested change

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

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

tschatzl · 2024-12-13T09:19:14Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

  G1CSetCandidateGroupList _from_marking_groups; // Set of regions selected by concurrent marking.
-  G1CSetCandidateGroupList _retained_groups; // Set of regions retained due to evacuation failure.
+  // Set of regions retained due to evacuation failure. Groups added to this list
+  // should contain only one region, making it easier to evacuate retained regions


Suggested change

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

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

tschatzl · 2024-12-13T09:20:35Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp

-  _from_marking_groups.abandon();
+  _retained_groups.clear(true /* uninstall_group_cardset */);
+  _from_marking_groups.clear(true /* uninstall_group_cardset */);
  for (uint i = 0; i < _max_regions; i++) {


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

tschatzl · 2024-12-13T09:24:07Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp


-  GrowableArrayFromArray<G1CollectionSetCandidateInfo> a(candidate_infos, (int)num_infos);
-  _candidates.appendAll(&a);
+void G1CSetCandidateGroup::add(G1HeapRegion* hr) {


(Probably add_retained_region was meant here?)

albertnetymk · 2024-12-17T09:34:08Z

src/hotspot/share/gc/g1/g1HeapRegionRemSet.hpp

+  // The collection set groups to which the region owning this RSet is assigned.
+  // We maintain a _default_cset_group to handle special cases, such as humongous regions,
+  // which are never added to collection set groups. This approach allows us to avoid using
+  // nullptr guards before every use of _cset_group.
+  G1CSetCandidateGroup* _default_cset_group;
+  G1CSetCandidateGroup* _cset_group;


As I understand it, only one of these two fields contains the real group. I don't get why we need null-checks if only _cset_group is there. Whenever we work with _cset_group, we should know whether it's null or not already depending on the call-site.

Refactored to guarantee that all call-sites are aware of this detail, then removed the _default_cset_group.

albertnetymk · 2024-12-17T09:47:24Z

src/hotspot/share/gc/g1/g1HeapRegion.cpp

+  // If the region will be collected as part of a group, then we cannot
+  // rely on the predition for this region.
+  if (_rem_set->is_added_to_cset_group() && _rem_set->cset_group()->length() > 1) {
+    return -1.0;


I believe all special cases logic (returning -1) in this method belong to the caller, G1PrintRegionLivenessInfoClosure, where we branch use if(gc_eff < 0) {`.

albertnetymk · 2024-12-17T09:56:48Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

-
 struct G1CollectionSetCandidateInfo {
  G1HeapRegion* _r;
  double _gc_efficiency;


Seems that this field has become unused.

albertnetymk · 2024-12-20T10:08:58Z

src/hotspot/share/gc/g1/g1CardSet.cpp

  uint card_within_region;
  split_card(card, card_region, card_within_region);

+


Extra blank line.

albertnetymk · 2024-12-20T10:09:42Z

src/hotspot/share/gc/g1/g1HeapRegionRemSet.hpp

-  // The set of cards in the Java heap
-  G1CardSet* _card_set;
-  G1CardSet* _saved_card_set;
+  // The collection set groups to which the region owning this RSet is assigned.


Should be singular, "group", right?

albertnetymk · 2024-12-20T10:41:09Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp


-void G1CollectionCandidateList::set(G1CollectionSetCandidateInfo* candidate_infos, uint num_infos) {
-  assert(_candidates.is_empty(), "must be");
+G1CSetCandidateGroup::G1CSetCandidateGroup(G1CardSetConfiguration* config, uint group_id) :


AFAICT, all callers use the same config from g1heap. I wonder if we reduce arg-list to just group_id.

albertnetymk · 2024-12-21T00:34:26Z

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp

+
+  uint num_added_to_group = 0;
+
+  uint group_id = 2;


Should move this magical constant to where const uint _group_id; is.

albertnetymk · 2024-12-21T00:45:18Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

+  if (r->rem_set()->is_added_to_cset_group()) {
+    if (r->rem_set()->cset_group()->length() == 1) {
+      gc_eff = r->rem_set()->cset_group()->gc_efficiency();
+    }


Why is gc_eff set only for length == 1?

if the group has more than one region, then the gc_eff is associated with the entire group and not just the single region. However, if we have just one region in the group, then we can go ahead and print the gc_eff details.

albertnetymk · 2024-12-21T00:47:50Z

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

+                          "-",
+                          size_t(0), young_only_cset_group->card_set()->mem_size());
+
+  for (G1CSetCandidateGroup* group : g1h->policy()->candidates()->from_marking_groups()) {


This would skip retained groups, right? Is that intentional?

Yes, retained regions are in "single region" groups, so all details should be added to the log when we call do_heap_region

I see; however, this would print the same gc_eff twice if young-gen contains a single region, right? Since this method is about cset-groups, I think it's more natural to visit all groups (regardless their size) here. With this PR, there is no gc_eff associated with individual region, do_heap_region can just skip gc_eff.

fixed, creating another issue; now we don't print details on humongous regions. I ask we fix that in a follow up.

bridgekeeper · 2025-01-20T22:56:55Z

@walulyai This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

walulyai · 2025-02-07T10:20:27Z

Thanks @albertnetymk and @tschatzl for the reviews!

/integrate

openjdk · 2025-02-07T10:22:23Z

Going to push as commit 86cec4e.
Since your change was applied there have been 51 commits pushed to the master branch:

006ed5c: 8349375: Cleanup AIX special file build settings
3989a19: 8344925: translet-name ignored when package-name is also set
1eb54e4: 8346049: jdk/test/lib/security/timestamp/TsaServer.java warnings
a0c7f66: 8349508: runtime/cds/appcds/TestParallelGCWithCDS.java should not check for specific output
1079147: 8348570: CTW: Expose the code hidden by uncommon traps
1a74ee6: 8349092: File.getFreeSpace violates specification if quotas are in effect (win)
0181030: 8349006: File.getCanonicalPath should remove "(on UNIX platforms)" from its specification
ed8945a: 8347377: Add validation checks for ICC_Profile header fields
1ab1c1d: 8349058: 'internal proprietary API' warnings make javac warnings unusable
eb84702: 8349513: Remove unused BUILD_JDK_JTREG_LIBRARIES_JDK_LIBS_libTracePinnedThreads
... and 41 more: https://git.openjdk.org/jdk/compare/beae8843b9b2433af5e9fbe420b17e663cbdb960...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-07T10:22:30Z

@walulyai Pushed as commit 86cec4e.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

walulyai added 8 commits October 23, 2024 11:55

revamp with retained regions added to groups

4f56a90

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

e6b1596

remove MarkingSkipEvents

834c598

cleanup

7a01f29

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

e92e0be

more cleanups

87ee3ba

add logging

b489c1d

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

9447a45

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Nov 11, 2024

walulyai marked this pull request as ready for review November 11, 2024 15:31

openjdk bot added the rfr Pull request is ready for review label Nov 11, 2024

tschatzl suggested changes Nov 19, 2024

View reviewed changes

Thomas Review

4aa4d6b

tschatzl suggested changes Dec 2, 2024

View reviewed changes

walulyai and others added 4 commits December 2, 2024 13:02

Update src/hotspot/share/gc/g1/g1CollectionSet.cpp

c3d7dda

Co-authored-by: Thomas Schatzl <59967451+tschatzl@users.noreply.github.com>

Update src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp

e901227

Co-authored-by: Thomas Schatzl <59967451+tschatzl@users.noreply.github.com>

Update src/hotspot/share/gc/g1/g1_globals.hpp

4eec225

Co-authored-by: Thomas Schatzl <59967451+tschatzl@users.noreply.github.com>

Update src/hotspot/share/gc/g1/g1CollectionSet.cpp

fbff7d7

Co-authored-by: Thomas Schatzl <59967451+tschatzl@users.noreply.github.com>

tschatzl approved these changes Dec 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 3, 2024

albertnetymk reviewed Dec 3, 2024

View reviewed changes

Albert Review

e687b0c

openjdk bot removed the ready Pull request is ready to be integrated label Dec 3, 2024

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

28e92c7

albertnetymk reviewed Dec 9, 2024

View reviewed changes

Albert Review 2

65e9b21

walulyai requested a review from tschatzl December 11, 2024 17:28

tschatzl suggested changes Dec 13, 2024

View reviewed changes

walulyai added 5 commits December 13, 2024 10:48

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

ee59e0b

Thomas Review

e573b82

assert

633e3a2

cleanup

f0dce79

fix space issues

ff5e9e0

tschatzl approved these changes Dec 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 13, 2024

walulyai added 2 commits December 17, 2024 10:42

fix type

08f5d3f

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

6194442

openjdk bot removed the ready Pull request is ready to be integrated label Dec 17, 2024

albertnetymk reviewed Dec 17, 2024

View reviewed changes

walulyai added 3 commits December 19, 2024 21:27

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

1a27e5d

Albert review

c4887ed

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

6a8039d

albertnetymk reviewed Dec 23, 2024

View reviewed changes

walulyai added 3 commits February 4, 2025 13:20

Merge remote-tracking branch 'upstream/master' into OldGenRemsetGroupsV1

403aad4

Revise Print Rememberedset info

d50457e

space

5b43fdb

albertnetymk approved these changes Feb 5, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 5, 2025

openjdk bot added the integrated Pull request has been integrated label Feb 7, 2025

openjdk bot closed this Feb 7, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 7, 2025


		g1h->collection_set()->merge_cardsets_for_collection_groups(g1h, merge, worker_id, _num_workers);

		g1h->collection_set_iterate_increment_from(&combined, nullptr, worker_id);

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

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

		uint card_within_region;
		split_card(card, card_region, card_within_region);

8343782: G1: Use one G1CardSet instance for multiple old gen regions #22015

8343782: G1: Use one G1CardSet instance for multiple old gen regions #22015

Uh oh!

Conversation

walulyai commented Nov 11, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 11, 2024

Uh oh!

openjdk bot commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 11, 2024

Uh oh!

mlbridge bot commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walulyai commented Nov 11, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 11, 2024 •

edited

Loading

mlbridge bot commented Nov 11, 2024 •

edited

Loading