On Thu, 7 Sep 2023 06:33:29 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 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
