On Mon, 11 Sep 2023 21:22:18 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 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
