8312174: missing JVMTI events from vthreads parked during JVMTI attach

[sspitsyn]

This update fixes two important issues:

• Issue reported by a bug submitter about missing JVMTI events on virtual threads after an a JVMTI agent dynamic attach
• Known scalability/performance issue: a need to lazily create JvmtiThreadState's for virtual threads

The issue is tricky to fix because the existing mechanism of the JVMTI event management does not support unmounted virtual threads. The JVMTI SetEventNotificationMode() calls the function JvmtiEventControllerPrivate::recompute_enabled()
which inspects a JavaThread's list and for each thread in the list recomputes enabled event bits with the function JvmtiEventControllerPrivate::recompute_thread_enabled(). The JvmtiThreadState of each thread is created but only when it is really needed, eg, if any of the thread filtered events is enabled. There was an initial adjustment of this mechanism for virtual threads which accounted for both carrier and virtual threads when a virtual thread is mounted. However, it does not work for unmounted virtual threads. A temporary work around was to always create JvmtiThreadState for each virtual thread eagerly at a thread starting point.

This fix introduces new function JvmtiExport::get_jvmti_thread_state() which checks if thread is virtual and there is a thread filtered event enabled globally, and if so, forces a creation of the JvmtiThreadState. Another adjustment was needed because the function state_for_while_locked() can be called directly in some contexts. New function JvmtiEventController::recompute_thread_filtered() was introduced to make necessary corrections.

Testing:

• new test from the bug report was adopted: test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest
• ran mach5 tiers 1-6: all are passed

---

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-8312174: missing JVMTI events from vthreads parked during JVMTI attach (Bug - P4)

Reviewers

• Leonid Mesnik (@lmesnik - Reviewer) ⚠️ Review applies to f5a144bc
• Alex Menkov (@alexmenkov - Reviewer) ⚠️ Review applies to 9f9355df

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15467

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@sspitsyn The following labels will be automatically applied to this pull request:

• hotspot
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 06: Full - Incremental (0ce4701a)
• 05: Full - Incremental (600974e0)
• 04: Full - Incremental (9f9355df)
• 03: Full - Incremental (f5a144bc)
• 02: Full - Incremental (6cf97ef9)
• 01: Full - Incremental (dd97dacc)
• 00: Full (dbe3a64a)

[sspitsyn]

I've pushed an update.
It includes:

• addressed review comments on new test test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest (some details are listed below)
• added comments for state_for() and state_for_while_locked() to src/hotspot/share/prims/jvmtiThreadState.hpp as Alex suggested
• moved the call to JvmtiEventController::recompute_thread_filtered(state) from state_for_while_locked() to state_for()
• removed newly added function JavaThread::is_virtual() and replaced its use in jvmtiExport.cpp with is_vthread_mounted()

Some of new test updates:

• Renamed check_jvmti_err() to check_jvmti_error() and moved it to test/lib/jdk/test/lib/jvmti/jvmti_common.h as Leonid suggested
• Removed VARIADICJNI and namespace in the native agent
• Removed std::mutex lock and used atomic counter instead
• Added @requires vm.compMode != "Xcomp" as the test execution time with -Xcomp sometimes is not enough
• I did not replace the test Counter class with the use CountDownLatch because it caused deadlocks while the test never deadlocks with the Counter class. Instead I've renamed Counter to CountDownLatch so that it can be easy to remove this custom implementation of CountDownLatch with the one from the java.util.concorrent. I'm not sure yet why use of original CountDownLatch class causes deadlocks.
• Refactored Java part of the test by introducing methods test1(), test2() and test3()
• Added code to wait for test1() to reach the execution of Thread.sleep(big-timeout) before the agent dynamic attach. One problem is that the thread state in sleep() is WAITING but not TIMED_WAITING (this looks like a bug: will need to follow up. So, there was a need to distinguish if the test1() does not execute the await code. It is why one more CountDownLatch object has been added.

[lmesnik]

Could you please add comment why standard CountDownLatch doesn't work for this test.

[openjdk]

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

8312174: missing JVMTI events from vthreads parked during JVMTI attach

Reviewed-by: lmesnik, amenkov

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

• d0be73a: 8041488: Locale-Dependent List Patterns
• dd214d0: 8315437: Enable parallelism in vmTestbase/nsk/monitoring/stress/classload tests
• 877731d: 8315770: serviceability/sa/TestJmapCoreMetaspace.java should run with -XX:-VerifyDependencies
• d06a564: 8315765: G1: Incorrect use of G1LastPLABAverageOccupancy
• 66b6a5a: 8277954: Replace use of monitors with explicit locks in the JDK LDAP provider implementation
• 4cb4637: 8315970: Big-endian issues after JDK-8310929
• ae08143: 8315611: Open source swing text/html and tree test
• 7b3e697: 8315855: G1: Revise signature of set_humongous_candidate
• 1941290: 8315942: Sort platform enums and definitions after JDK-8304913 follow-ups
• 996b336: 8315781: Reduce the max value of GCDrainStackTargetSize
• ... and 44 more: https://git.openjdk.org/jdk/compare/ab28300a42a0dadda76880a51a4b4ac6f39ad794...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.

[sspitsyn]

Could you please add comment why standard CountDownLatch doesn't work for this test.

Okay. Added comment.

[sspitsyn]

Leonid and Alex, thank you a lot for review and discussions!

[sspitsyn]

/integrate

[openjdk]

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

• d0be73a: 8041488: Locale-Dependent List Patterns
• dd214d0: 8315437: Enable parallelism in vmTestbase/nsk/monitoring/stress/classload tests
• 877731d: 8315770: serviceability/sa/TestJmapCoreMetaspace.java should run with -XX:-VerifyDependencies
• d06a564: 8315765: G1: Incorrect use of G1LastPLABAverageOccupancy
• 66b6a5a: 8277954: Replace use of monitors with explicit locks in the JDK LDAP provider implementation
• 4cb4637: 8315970: Big-endian issues after JDK-8310929
• ae08143: 8315611: Open source swing text/html and tree test
• 7b3e697: 8315855: G1: Revise signature of set_humongous_candidate
• 1941290: 8315942: Sort platform enums and definitions after JDK-8304913 follow-ups
• 996b336: 8315781: Reduce the max value of GCDrainStackTargetSize
• ... and 44 more: https://git.openjdk.org/jdk/compare/ab28300a42a0dadda76880a51a4b4ac6f39ad794...master

Your commit was automatically rebased without conflicts.

[openjdk]

@sspitsyn Pushed as commit fda142f.

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