RFR: 8312174: missing JVMTI events from vthreads parked during JVMTI attach

[Alex Menkov]

On Tue, 29 Aug 2023 10:09:21 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

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