8358003: KlassTrainingData initializer reads garbage holder #25623

veresov · 2025-06-03T17:36:13Z

Simplify KlassTrainingData constructor. The lines in question come from the old pre-CDS world. They are not needed anymore.

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-8358003: KlassTrainingData initializer reads garbage holder (Bug - P3)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer) Review applies to f9b133fe
Vladimir Ivanov (@iwanowww - Reviewer) Review applies to f9b133fe
Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25623

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

Using diff file

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

Using Webrev

Link to Webrev Comment

veresov · 2025-06-03T17:36:44Z

mach5 testing in progress, will report back once it's done.

bridgekeeper · 2025-06-03T17:40:02Z

👋 Welcome back iveresov! 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-06-03T17:45:47Z

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

8358003: KlassTrainingData initializer reads garbage holder

Reviewed-by: coleenp, shade, vlivanov

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

7838321: 8358496: Concurrent reading from Socket with timeout executes sequentially
42f48a3: 8350689: Turn on timestamp and thread metadata by default for java.security.debug
cd16b68: 8357155: [asan] ZGC does not work (x86_64 and ppc64)
... and 57 more: https://git.openjdk.org/jdk/compare/a7671e7360ce23df9577110266e66a1c1a790196...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.

shipilev

Makes sense. I was dumbfounded what was "previous handle", when we are in constructor. I suspected it was something about placement-new code somewhere.

openjdk · 2025-06-03T18:04:08Z

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

hotspot

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-06-03T18:07:25Z

Webrevs

iwanowww

Looks good.

veresov · 2025-06-03T21:08:56Z

It seems like we don't need these release_stores either since the constructor is always run under a lock.
I'll run some testing.

coleenp · 2025-06-03T22:37:54Z

src/hotspot/share/oops/trainingData.cpp

-  Atomic::release_store(&_holder, const_cast<InstanceKlass*>(klass));
+  assert(klass != nullptr, "");
+  Handle hm(JavaThread::current(), klass->java_mirror());
+  jobject hmj = JNIHandles::make_global(hm);


Why don't you use OopStorage for this?

Are there any advantages?

Ok, transitioned to OopStrage. Please take a look if correctly. I'll be back when the testing is done.

The advantage of OopStorage is that jni handles aren't trusted because they come from outside jni calls so have some safefetch code, but OopStorage are trusted so presumably faster.

veresov · 2025-06-03T22:45:38Z

Testing is ok

veresov · 2025-06-04T08:43:04Z

Ok, testing was clean. Please take another look.

shipilev

Right off the bat, before I look at the rest of it: I don't think there is a need to introduce another OopStorage class just for these handles. We already see it would probably require touchups in other code that enumerates OopStorages. So instead, use VM Global one? I.e. do:

 handle = OopHandle(Universe::vm_global(), obj);

Also I cannot spot where we clean these. Note that for OopHandle-s, you have to explicitly call .release, likely in KlassTrainingData destructor.

veresov · 2025-06-04T09:49:33Z

We don't need to release them. KTDs are never destroyed. They just die with the process.
As for OopStorage @coleenp wants it. I gives a bit of an advantage that we can remove the handle field from KTD (since again, we don't ever need to free them).

veresov · 2025-06-04T09:56:20Z

I kind of need to push it today before the fork. Let's try making changes to this minimal. I'm also fine reverting back to before @coleenp suggested OopStorage. And we can address the remaining concerns later.

shipilev · 2025-06-04T09:56:42Z

I kind of need to push it today before the fork. Let's try making changes to this minimal. I'm also fine reverting back to before @coleenp suggested OopStorage. And we can address the remaining concerns later.

Yeah, let's do OopStorage rewrite as the followup.

veresov · 2025-06-04T10:12:07Z

Ok, I reverted this to before the OopStorage changes. And filed https://bugs.openjdk.org/browse/JDK-8358580 to rethink it later. @coleenp are you ok with that?

coleenp

I'm fine with this and the follow-up issue.

coleenp · 2025-06-04T11:32:52Z

src/hotspot/share/oops/trainingData.cpp

-  Atomic::release_store(&_holder, const_cast<InstanceKlass*>(klass));
+  assert(klass != nullptr, "");
+  Handle hm(JavaThread::current(), klass->java_mirror());
+  jobject hmj = JNIHandles::make_global(hm);


The advantage of OopStorage is that jni handles aren't trusted because they come from outside jni calls so have some safefetch code, but OopStorage are trusted so presumably faster.

veresov · 2025-06-04T14:07:12Z

/integrate

openjdk · 2025-06-04T14:07:50Z

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

0352477: 8357660: [JVMCI] Add support for retrieving all BootstrapMethodInvocations directly from ConstantPool
a653ff4: 8358536: jdk/jfr/api/consumer/TestRecordingFileWrite.java times out
248341d: 8344942: Template-Based Testing Framework
... and 62 more: https://git.openjdk.org/jdk/compare/a7671e7360ce23df9577110266e66a1c1a790196...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-06-04T14:07:57Z

@veresov Pushed as commit ae1892f.

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

Cleanup with KlassTrainingData constructor

f9b133f

openjdk bot added the rfr Pull request is ready for review label Jun 3, 2025

shipilev approved these changes Jun 3, 2025

View reviewed changes

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jun 3, 2025

openjdk bot added the ready Pull request is ready to be integrated label Jun 3, 2025

iwanowww approved these changes Jun 3, 2025

View reviewed changes

No need for release_store()

85a7161

openjdk bot removed the ready Pull request is ready to be integrated label Jun 3, 2025

coleenp reviewed Jun 3, 2025

View reviewed changes

veresov added 3 commits June 3, 2025 16:59

Use dedicated OopStorage

639026b

More changes

f8a9b4a

More changes

5a7b128

veresov requested review from coleenp, iwanowww and shipilev June 4, 2025 08:43

shipilev reviewed Jun 4, 2025

View reviewed changes

Undo OopStorage changes

a5693d6

coleenp approved these changes Jun 4, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 4, 2025

openjdk bot added the integrated Pull request has been integrated label Jun 4, 2025

openjdk bot closed this Jun 4, 2025

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

8358003: KlassTrainingData initializer reads garbage holder #25623

8358003: KlassTrainingData initializer reads garbage holder #25623

Uh oh!

Conversation

veresov commented Jun 3, 2025 • 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!

veresov commented Jun 3, 2025

Uh oh!

bridgekeeper bot commented Jun 3, 2025

Uh oh!

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

Uh oh!

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 3, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

veresov commented Jun 3, 2025

Uh oh!

coleenp Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

veresov Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

veresov Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

veresov commented Jun 3, 2025

Uh oh!

veresov commented Jun 4, 2025

Uh oh!

shipilev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veresov commented Jun 4, 2025

Uh oh!

veresov commented Jun 4, 2025

Uh oh!

shipilev commented Jun 4, 2025

Uh oh!

veresov commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

veresov commented Jun 4, 2025

Uh oh!

openjdk bot commented Jun 4, 2025

Uh oh!

openjdk bot commented Jun 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

veresov commented Jun 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 3, 2025 •

edited

Loading

mlbridge bot commented Jun 3, 2025 •

edited

Loading

shipilev left a comment •

edited

Loading

veresov commented Jun 4, 2025 •

edited

Loading