8370807: G1: Improve region attribute table method naming #28038

tschatzl · 2025-10-29T07:50:53Z

Hi all,

please review this cleanup of the region attribute registration methods.

Testing: gha

Thanks,
Thomas

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-8370807: G1: Improve region attribute table method naming (Enhancement - P4)

Reviewers

Stefan Johansson (@kstefanj - Reviewer) Review applies to 162f3875
Ivan Walulya (@walulyai - Reviewer) Review applies to 162f3875
Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28038

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Hi all, please review this cleanup of the region attribute registration methods. Testing: gha Thanks, Thomas

bridgekeeper · 2025-10-29T07:51:52Z

👋 Welcome back tschatzl! 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 · 2025-10-29T07:52:48Z

@tschatzl 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:

8370807: G1: Improve region attribute table method naming Reviewed-by: ayang, sjohanss, iwalulya

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 4 new commits pushed to the master branch:

6080ccd: 8370797: Test runtime/ErrorHandling/AccessZeroNKlassHitsProtectionZone.java failed on macos 26
436dc68: 8367059: DTLS: loss of NewSessionTicket message results in handshake failure
28f2591: 8370568: Refer to Thread.interrupted as "interrupted status" consistently
... and 1 more: https://git.openjdk.org/jdk/compare/2c07214d7c075da5dd4a4e872aef29f58cef2bae...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 · 2025-10-29T07:53:42Z

@tschatzl 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 · 2025-10-29T07:57:32Z

Webrevs

kstefanj

Change looks good, but the bug-title could be updated to reflect that this is not just a change to a comment.

albertnetymk · 2025-10-29T10:29:04Z

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

 }

-void G1CollectedHeap::register_old_region_with_region_attr(G1HeapRegion* r) {
+void G1CollectedHeap::register_old_collection_set_region_with_region_attr(G1HeapRegion* r) {


Why "collection_set" in the name? Is it a precondition that r is in cset?

It must be a region that is currently being added to the collection set. I.e. registered to the attribute table to be in the collection set.

Then in_cset is a postcondition, not a precondition. The new name seems to suggest the arg (r) is old + in_cset, as a precondition.

Looking at the impl, one can't see anything related to "collection_set" in this context. I'd suggest restoring the original name, but YMMV.

The set_in_old call is the giveaway - in the region attribute table you only set a region to "Old" if it is

static const region_type_t Old = 1; // The region is in the collection set and an old region.

Old regions not in the collection set are just NotInCset.

The "Young" and "NewSurvivor" do not have the collection_set tagging because they are always in the collection set (i.e. it's obvious).

I thought it would be good to make it clear in the name that for old regions, we only add (old) regions that are part of the collection set.

The attribute table is an accelerator table - it only mirrors information from elsewhere used during GC. So the given region is already(*) in the collection set, but we can't check easily.

(*) Formally, from the code flow it isn't yet - the cardset group this region is contained in (which determines whether the related regions are in) will also be added during the G1CollectionSet::add_group_to_collection_set() call. This would just require some reordering of the calls, but logically it already is. If you really want I can do that in this change too.

So the given region is already(*) in the collection set

If that's indeed precondition, the new name makes sense.

There is assert(!hr->in_collection_set(), in the caller, which is conflicting with this semantic.

If you really want I can do that in this change too.

I'd prefer doing that (new name + new precondition) in another PR, as the current title imply naming changes only, but up to you.

There is assert(!hr->in_collection_set(), in the caller, which is conflicting with this semantic.

The reason is that that assert only checks the accelerator table, which must not indicate that we already added it. There is no long form of that in-collections-set check any more, we removed it quite some time ago because we thought it was superfluous.

So reordering the calls does not change the situation, i.e. there is no long-form check for the condition (i.e. the region is in the collection set already and we are just updating the attribute table).

tschatzl · 2025-10-30T07:33:12Z

Thanks @kstefanj @walulyai @albertnetymk for your reviews
/integrate

openjdk · 2025-10-30T07:35:27Z

Going to push as commit 17fd801.
Since your change was applied there have been 7 commits pushed to the master branch:

87a4772: 8366968: Exhaustive switch expression rejected by for not covering all possible values
f3dfdfa: 8369013: Shenandoah: passive mode should support enabling ShenandoahCardBarrier
d62553d: 8316274: javax/swing/ButtonGroup/TestButtonGroupFocusTraversal.java fails in Ubuntu 23.10 with Motif LAF
... and 4 more: https://git.openjdk.org/jdk/compare/2c07214d7c075da5dd4a4e872aef29f58cef2bae...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-10-30T07:35:37Z

@tschatzl Pushed as commit 17fd801.

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

8370807

162f387

Hi all, please review this cleanup of the region attribute registration methods. Testing: gha Thanks, Thomas

openjdk bot changed the title ~~8370807~~ 8370807: G1: Fix comment about not selecting pinned regions into collection set Oct 29, 2025

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Oct 29, 2025

openjdk bot added the rfr Pull request is ready for review label Oct 29, 2025

tschatzl mentioned this pull request Oct 29, 2025

8370804: G1: Make G1HeapRegionAttr::remset_is_tracked() conform to coding style #28039

Closed

3 tasks

kstefanj approved these changes Oct 29, 2025

View reviewed changes

walulyai approved these changes Oct 29, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 29, 2025

tschatzl changed the title ~~8370807: G1: Fix comment about not selecting pinned regions into collection set~~ 8370807: G1: Improve region attribute table method naming Oct 29, 2025

albertnetymk reviewed Oct 29, 2025

View reviewed changes

* ayang review

818c353

openjdk bot removed the ready Pull request is ready to be integrated label Oct 29, 2025

tschatzl added 3 commits October 29, 2025 14:49

Merge branch 'master' into 8370807-pinned-region-flag-not-propagated

f2491bf

* ayang review2

e946b78

* add more verification before changing the attribute table

e432694

albertnetymk approved these changes Oct 29, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 29, 2025

openjdk bot added the integrated Pull request has been integrated label Oct 30, 2025

openjdk bot closed this Oct 30, 2025

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

tschatzl deleted the submit/8370807-pinned-region-flag-not-propagated branch October 30, 2025 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8370807: G1: Improve region attribute table method naming #28038

8370807: G1: Improve region attribute table method naming #28038

tschatzl commented Oct 29, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 29, 2025

openjdk bot commented Oct 29, 2025 •

edited

Loading

openjdk bot commented Oct 29, 2025 •

edited

Loading

mlbridge bot commented Oct 29, 2025 •

edited

Loading

kstefanj left a comment

albertnetymk Oct 29, 2025

tschatzl Oct 29, 2025 •

edited

Loading

albertnetymk Oct 29, 2025

tschatzl Oct 29, 2025 •

edited

Loading

tschatzl Oct 29, 2025

tschatzl Oct 29, 2025

albertnetymk Oct 29, 2025

tschatzl Oct 29, 2025

tschatzl Oct 29, 2025 •

edited

Loading

tschatzl commented Oct 30, 2025

openjdk bot commented Oct 30, 2025

openjdk bot commented Oct 30, 2025

Labels

4 participants

8370807: G1: Improve region attribute table method naming #28038

8370807: G1: Improve region attribute table method naming #28038

Conversation

tschatzl commented Oct 29, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 29, 2025

openjdk bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

openjdk bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mlbridge bot commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

kstefanj left a comment

Choose a reason for hiding this comment

albertnetymk Oct 29, 2025

Choose a reason for hiding this comment

tschatzl Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

albertnetymk Oct 29, 2025

Choose a reason for hiding this comment

tschatzl Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

tschatzl Oct 29, 2025

Choose a reason for hiding this comment

tschatzl Oct 29, 2025

Choose a reason for hiding this comment

albertnetymk Oct 29, 2025

Choose a reason for hiding this comment

tschatzl Oct 29, 2025

Choose a reason for hiding this comment

tschatzl Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

tschatzl commented Oct 30, 2025

openjdk bot commented Oct 30, 2025

openjdk bot commented Oct 30, 2025

Labels

4 participants

tschatzl commented Oct 29, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 29, 2025 •

edited

Loading

openjdk bot commented Oct 29, 2025 •

edited

Loading

mlbridge bot commented Oct 29, 2025 •

edited

Loading

tschatzl Oct 29, 2025 •

edited

Loading

tschatzl Oct 29, 2025 •

edited

Loading

tschatzl Oct 29, 2025 •

edited

Loading