RFR: 8312174: missing JVMTI events from vthreads parked during JVMTI attach [v2]

[Leonid Mesnik]

On Thu, 7 Sep 2023 06:33:29 GMT, Serguei Spitsyn <sspitsyn at 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge
>  - 8312174: missing JVMTI events from vthreads parked during JVMTI attach

Changes requested by lmesnik (Reviewer).

test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java line 86:

> 84:             log("WARNING: test expects at least 8 processors.");
> 85:         }
> 86:         Counter ready1 = new Counter(THREAD_CNT);

I think that CountDownLatch should works fine here and no need to use custom Counter.

test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp line 30:

> 28: #include "jvmti_common.h"
> 29: 
> 30: #ifdef _WIN32

Do we need it here?

test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp line 44:

> 42: 
> 43:   void JNICALL VirtualThreadEnd(jvmtiEnv *jvmti, JNIEnv* jni, jthread virtual_thread) {
> 44:     std::lock_guard<std::mutex> lockGuard(lock);

the atomic would be better for counters. It guarantees that counter is always protected.

test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp line 62:

> 60: 
> 61: void
> 62: check_jvmti_err(jvmtiError err, const char* msg) {

This function could be moved into jvmti_common.h.

-------------

PR Review: https://git.openjdk.org/jdk/pull/15467#pullrequestreview-1616597260
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1319303393
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1319295540
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1319299148
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1319295250