On Mon, 11 Sep 2023 09:08:26 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> 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 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > removed JavaThread::is_virtual() 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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/15467#issuecomment-1714362970
