8308603: Removing do_pending_ref/enclosing_ref from MetaspaceClosure

[iklam]

The problem

MetaspaceClosure is used to traverse all pointers in the metaspace objects. It works recursively. A pointer is represented by the MetaspaceClosure::Ref type. For example, if you have

InstanceKlass* a = ..., *b = ...;
a->_super = b;

When we are traversing the pointer a->_super, we create a Ref that records the address of b and &a->super. However, we currently don't remember the address of a in the Ref. As a result, when processing the Ref inside a push(Ref* ref) function, we need to call MetaspaceClosure::enclousing_ref() to get to a, so that we can mark pointers inside it.

The reason fordo_pending_ref is even more convoluted. Please see JDK-8308603 for more details.

The fix

The code can be much more readable if we simply remember a in the Ref.

More info

A more in-depth discussion of the use of the MetaspaceClosure API in CDS can be found at:

• https://github.com/iklam/tools/blob/main/NOTES/cds_status_2023.md#33-copying-of-class-metadata

---

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-8308603: Removing do_pending_ref/enclosing_ref from MetaspaceClosure (Enhancement - P4)

Reviewers

• Calvin Cheung (@calvinccheung - Reviewer) ⚠️ Review applies to fecc2a42

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14093

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back iklam! 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]

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

• hotspot-runtime

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

• 01: Full - Incremental (75024c46)
• 00: Full (fecc2a42)

[coleenp]

Are 'obj' oops ? We almost always refer to oops as obj but not other things. Is 'object' an oop? This operates on metadata right? I realize this terminology is pre-existing. I suggest changing obj and object in comments at least in the new code to either metadata() or ptr() even, metaptr() ? Unless this does move oops. I don't know this code.

[iklam]

All the "objects" in the code related to MetaspaceClosure refer to instances of MetaspaceObj. This code rarely interacts with code that work on oops, so there isn't much confusion in practice.

I'd rather not change part of the code or comments to use a different name than "object". I think that will be even more confusing, causing the reader to think that "metadata", for example, is materially different than "object".

Also, "metadata" is unfortunately not a good name, because not every MetaspaceObj is a Metadata.

If renaming is desirable, that should be done in a separate RFE.

[openjdk]

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

8308603: Removing do_pending_ref/enclosing_ref from MetaspaceClosure

Reviewed-by: ccheung

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

[iklam]

Thanks @calvinccheung for the review.
/integrate

[openjdk]

Going to push as commit 16c3d53.
Since your change was applied there have been 4 commits pushed to the master branch:

• b94b679: 8309627: Incorrect sorting of DirtyCardQueue buffers
• aace3dc: 8309760: ProblemList serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java#default with ZGC
• 80edd5c: 8294985: SSLEngine throws IAE during parsing of X500Principal
• bdd81b3: 8304885: Reuse stale data to improve DNS resolver resiliency

Your commit was automatically rebased without conflicts.

[openjdk]

@iklam Pushed as commit 16c3d53.

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