8366688: G1: Rename G1HeapRegionRemSet::is_added_to_cset_group() to has_cset_group()

[tschatzl]

Hi all,

please review this trivial renaming of G1HHRS::is_added_to_cset_group - it's just misnamed, we never add G1HRRS to the cset groups.

Testing: gha, local compilation

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-8366688: G1: Rename G1HeapRegionRemSet::is_added_to_cset_group() to has_cset_group() (Enhancement - P4)

Reviewers

• Albert Mingkun Yang (@albertnetymk - Reviewer)
• Ivan Walulya (@walulyai - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27048

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

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

8366688: G1: Rename G1HeapRegionRemSet::is_added_to_cset_group() to has_cset_group()

Reviewed-by: ayang, 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 52 new commits pushed to the master branch:

• c935d1c: 8366375: Collator example for SECONDARY uses wrong code point
• 48ba8ed: 8366704: Bump timeout on TestInheritFD
• ecf05ca: 8366693: Backout recent JavaLangAccess changes breaking the build
• ... and 49 more: https://git.openjdk.org/jdk/compare/86d6a2e05eb52ea2c603a06bce838a56d5ae507b...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]

@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]

Webrevs

• 00: Full (7697c98e)

[albertnetymk]

I can sort of understand why it's named "added_to", because conceptually, the api answers whether the per-region remset belongs to a cset group, instead of a unique one.

The name "has_cset_group" is odd, IMO, since it implies ownership, which is wrong. The new name makes more sense on a heap-region -- a region possesses a cset group, shared with other regions potentially.

YMMV.

[tschatzl]

I can sort of understand why it's named "added_to", because conceptually, the api answers whether the per-region remset belongs to a cset group, instead of a unique one.

I think there is some confusion about the naming of cset groups. Cset groups are nothing but a card set covering the cards of a set of regions.

Previously the card set has been part of the HRRS ("all the information to evacuate a region", i.e. the region's remset), the change has been that the card set part of the HRRS is shared.

[Unfortunately it would be a bit awkward to have the HRRS itself be shared across regions right now - the problem is the code root set. If g1 shared the code root set, and drop the HRRS, then g1 could never evacuate that region any more as we never rebuild that one. Could be added though.]

The name "has_cset_group" is odd, IMO, since it implies ownership, which is wrong. The new name makes more sense on a heap-region -- a region possesses a cset group, shared with other regions potentially.

I see your point - what if the cset group would be called something like "cardset group" (I have no good name, maybe shared-cardset, but otoh it also contains some information related to evacuation like gc-efficiency of the group of regions, so it is related to collection set candidates), would that make understanding the reason for this name? Then it makes sense that one can ask the question whether the heap region's remset (the hrrs) "has" a card based remembered set (attached).

Maybe there is a better term here, like the suggested has_cset_group_attached/assigned() or so? I think the shorthand suggested in this PR is acceptable.

What do you think?

[tschatzl]

Thanks @walulyai @albertnetymk for your reviews
/integrate

[openjdk]

Going to push as commit 222ae36.
Since your change was applied there have been 80 commits pushed to the master branch:

• 49fd6a0: 8366558: Gtests leave /tmp/cgroups-test* files
• 2527e9e: 8366490: C2 SuperWord: wrong result because CastP2X is missing ctrl and floats over SafePoint creating stale oops
• a03302d: 8366434: THP not working properly with G1 after JDK-8345655
• ... and 77 more: https://git.openjdk.org/jdk/compare/86d6a2e05eb52ea2c603a06bce838a56d5ae507b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tschatzl Pushed as commit 222ae36.

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