On Tue, 29 Aug 2023 10:09:21 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
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 91:
> 89:
> 90: try (ExecutorService executorService =
> Executors.newVirtualThreadPerTaskExecutor()) {
> 91: for (int tCnt = 0; tCnt < TCNT1; tCnt++) {
Could you please add a comment before each test group creation block about
expected state
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 100:
> 98: mready.await();
> 99: try {
> 100: // timeout is big enough to keep mounted untill
> interrupted
The comment is misleading. 1st group of threads are expected to be unmounted
during attach and mounted after the threads are interrupted.
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 136:
> 134: ready1.await();
> 135: mready.decr();
> 136: VirtualMachine vm =
> VirtualMachine.attach(String.valueOf(ProcessHandle.current().pid()));
I think sleep is needed here so threads which should be unmounted have time to
unmount before attach.
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 141:
> 139: log("main: completedNo: " + completedNo);
> 140: attached = true;
> 141: for (Thread t : threads) {
AFAIU threads in 3rd group (TCNT3) should be unmounted (with
LockSupport.parkNanos) before they are interrupted.
Then we need sleep here
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
line 149:
> 147: for (int sleepNo = 0; sleepNo < 10 && threadEndCount() <
> THREAD_CNT; sleepNo++) {
> 148: log("main: wait iter: " + sleepNo);
> 149: Thread.sleep(100);
sleep(1000)? (comment before the loop tells about 10 secs)
test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp
line 37:
> 35:
> 36: namespace {
> 37: std::mutex lock;
This mutex is only to make access to counters atomic.
It would be clearer to make counters std::atomic<int> and remove the mutex
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317795256
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317794334
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317796891
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317811234
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317804389
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1317802305
