8352256: ObjectSynchronizer::quick_notify misses JFR event notification path

[shipilev]

Noticed this while removing OM counters. When doing JDK-8351187, I apparently forgot to cover a path from ObjectSynchronizer::quick_notify, probably due to concurrent renames. I think we can cover this gap while still simplifying the code: let ObjectMonitor handle all events, DTrace, JFR, OM stats.

Additional testing:

• Linux x86_64 server fastdebug, jdk_jfr

---

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-8352256: ObjectSynchronizer::quick_notify misses JFR event notification path (Enhancement - P4)

Reviewers

• Coleen Phillimore (@coleenp - Reviewer) Review applies to d82b3da2
• David Holmes (@dholmes-ora - Reviewer)
• Markus Grönlund (@mgronlun - Reviewer) Review applies to 50b17f0a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24097

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8352256: ObjectSynchronizer::quick_notify misses JFR event notification path

Reviewed-by: dholmes, coleenp, mgronlun

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

• 9844c1c: 8066583: DeflaterInput/OutputStream and InflaterInput/OutputStream should explain responsibility for freeing resources
• fb955bc: 8353267: jmod create finds the wrong set of packages when class file are in non-package location
• 80ff7b9: 8353695: RISC-V: compiler/cpuflags/TestAESIntrinsicsOnUnsupportedConfig.java is failing with Zvkn
• ... and 30 more: https://git.openjdk.org/jdk/compare/d63b561fffd42d76f14771c47951dd1d08efe3a7...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]

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

• 03: Full (2ffd2d82)
• 02: Full (0c259e58)
• 01: Full - Incremental (144636f9)
• 00: Full (d82b3da2)

[coleenp]

This looks good. At one point, because of this lack of encapsulation, I wanted us to get rid of quick_notify because I didn't think it was quicker. This improves the encapsulation too and we can figure out if it's useful later.

[shipilev]

This looks good. At one point, because of this lack of encapsulation, I wanted us to get rid of quick_notify because I didn't think it was quicker. This improves the encapsulation too and we can figure out if it's useful later.

Yeah, I wondered about these "quick" functions myself when doing this. I guess there is a papercut performance optimization for not re-checking the owner and waitset, which is why I opted to do ObjectMonitor::quick_* entries that bypass those checks as well. But I am doubtful those checks matter.

[dholmes-ora]

The use of the quick variants is to avoid the checks that could lead to throwing an exception, as they would require that the thread be _thread_in_vm not _thread_in_java.

That said it is not clear to me that posting the event doesn't violate the requirements that the quick version must maintain: no safepoints, no blocking, no state transitions. ??

[shipilev]

That said it is not clear to me that posting the event doesn't violate the requirements that the quick version must maintain: no safepoints, no blocking, no state transitions. ??

Huh, that's a good question. There is a NoSafepointVerifier at the beginning ObjectSynchronizer::quick_notify, so at least the mechanical runtime checks did not fail. I'll run all tests with monitor notification event enabled.

I clicked through the JFR event commit code, and I don't think there are safepoints, indefinite blocking, or state transitions. Can we rely on this, @egahlin, @mgronlun?

[shipilev]

I'll run all tests with monitor notification event enabled.

I see no new failures. There are some pre-existing bugs with JFR enabled, but nothing new. So with that, and another sweep through the code, I think we are fine.

[shipilev]

(friendly reminder)

[mgronlun]

From a JFR perspective, we need threads in state VM or Java for writing events that refer to artifacts; both states are equivalent for epoch management. You can also have threads in native writing JFR events, but only if the event fields are scalars, not foreign keys. However, the thread cannot capture a stack trace.

[mgronlun]

A JavaThread in state VM or Java running the JFR commit code will not perform a transition or block for a safepoint. It is not waitfree (yet) however because of the JfrStacktrace_lock.

[mgronlun]

Are we keeping the NoSafepointVerifier in the quick entries?

[shipilev]

Are we keeping the NoSafepointVerifier in the quick entries?

Yes we do.

[shipilev]

Thanks @mgronlun! @dholmes-ora, are you happy with our conclusions?

[dholmes-ora]

I clicked through the JFR event commit code, and I don't think there are safepoints, indefinite blocking, or state transitions. Can we rely on this, @egahlin, @mgronlun?

I do not see a clear answer to this.

Again just for clarity the quick versions are for use when you 100% guarantee the monitor is owned. They do not need to, and cannot, check the owner and potentially throw IMSE.

[shipilev]

Again just for clarity the quick versions are for use when you 100% guarantee the monitor is owned. They do not need to, and cannot, check the owner and potentially throw IMSE.

I don't think new quick versions do. We only assert(check_owner(...)), but I think that is "safe" in the sense we fail hard if monitor is not owned.

[mgronlun]

That said it is not clear to me that posting the event doesn't violate the requirements that the quick version must maintain: no safepoints, no blocking, no state transitions. ??

Huh, that's a good question. There is a NoSafepointVerifier at the beginning ObjectSynchronizer::quick_notify, so at least the mechanical runtime checks did not fail. I'll run all tests with monitor notification event enabled.

I clicked through the JFR event commit code, and I don't think there are safepoints, indefinite blocking, or state transitions. Can we rely on this, @egahlin, @mgronlun?

A JavaThread in state VM or Java running the JFR commit code will not perform a transition or block for a safepoint. It is not waitfree (yet) however because of the JfrStacktrace_lock.

[dholmes-ora]

Looks good - thanks for the updates.

Lets see how this works in practice.

[shipilev]

Thanks for reviews!

/integrate

[openjdk]

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

• 9844c1c: 8066583: DeflaterInput/OutputStream and InflaterInput/OutputStream should explain responsibility for freeing resources
• fb955bc: 8353267: jmod create finds the wrong set of packages when class file are in non-package location
• 80ff7b9: 8353695: RISC-V: compiler/cpuflags/TestAESIntrinsicsOnUnsupportedConfig.java is failing with Zvkn
• ... and 30 more: https://git.openjdk.org/jdk/compare/d63b561fffd42d76f14771c47951dd1d08efe3a7...master

Your commit was automatically rebased without conflicts.

[openjdk]

@shipilev Pushed as commit ea07e71.

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