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

[Alex Menkov]

On Mon, 11 Sep 2023 21:22:18 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 incrementally with one additional commit since the last revision:
> 
>   addressed second round of review comments on VThreadEventTest.java

Marked as reviewed by amenkov (Reviewer).

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

> 43:  * The test uses custom implementation of the CountDownLatch class.
> 44:  * The reason is we want the state of tested thread to be predictable.
> 45:  * With original CountDownLatch it is not clear what thread state is expected.

"original CountDownLatch" -> "java.util.concurrent.CountDownLatch"

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

> 104:         ready1.countDown(); // to guaranty state is not State.WAITING after await(mready)
> 105:         try {
> 106:             Thread.sleep(20000); // big timeout to keep unmounted untill interrupted

untill -> until

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

> 130:             // keep mounted
> 131:         }
> 132:         LockSupport.parkNanos(10 * TIMEOUT_BASE); // will cause extra mount and unmount

I don't see much sense in TIMEOUT_BASE constant (it's used only here and multiplied by 10)
I think it would be clearer to
Suggestion:

        // park for 10ms; causes extra unmount and mount
        LockSupport.parkNanos(10_000_000L);

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

> 22:  */
> 23: 
> 24: #include <cstdlib>

I suppose this include was needed for abort() only and not needed anymore

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

> 25: #include <cstring>
> 26: #include <jvmti.h>
> 27: #include <mutex>

not needed

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

PR Review: https://git.openjdk.org/jdk/pull/15467#pullrequestreview-1620911716
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322125143
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322125314
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322134900
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322138828
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322135837