On Wed, 29 Mar 2023 18:02:38 GMT, Serguei Spitsyn <[email protected]> wrote:
>> The fix is to enable virtual threads support for late binding JVMTI agents.
>> The fix includes:
>> - New function `JvmtiEnvBase::enable_virtual_threads_notify_jvmti()` which
>> does enabling JVMTI VTMS transition notifications in case of agent loaded
>> into running VM. This function executes a VM operation counting VTMS
>> transition bits in all `JavaThread`'s to correctly set the static counter
>> `_VTMS_transition_count` needed for VTMS transition protocol.
>> - New function `JvmtiEnvBase::disable_virtual_threads_notify_jvmti()` which
>> is needed for testing. It is used by the `WhiteBox` API.
>> - New WhiteBox function `WB_SetVirtualThreadsNotifyJvmtiMode(JNIEnv* env,
>> jobject wb, jboolean enable)` needed for testing of this update.
>> - New regression test: `serviceability/jvmti/vthread/ToggleNotifyJvmtiTest`
>>
>> Testing:
>> - New test: `serviceability/jvmti/vthread/ToggleNotifyJvmtiTest`
>> - The originally failed tests are expected to pass now:
>> `runtime/vthread/RedefineClass.java`
>> `runtime/vthread/TestObjectAllocationSampleEvent.java`
>> - In progress: Run the tiers 1-6 to make sure there are no regression.
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: updated correction of jt->jvmti_thread_state() links in
> VM_SetNotifyJvmtiEventsMode
Hi Serguei,
I took a look at the patch and looks good to me. I have a couple of comments
though.
Thanks,
Patricio
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1554:
> 1552: }
> 1553: // Correct jt->jvmti_thread_state() and jt->jvmti_vthread() if
> necessary.
> 1554: // It was not maintained while notifyJvmti was disabled.
While trying to understand which exact situation we are trying to guard against
with this code, I run the test without the sleeps and without this restore code
and I got a crash when deleting a JvmtiThreadState (null dereference of _thread
in the ~()). Probably the same crash you mentioned you had. But when debugging
the crash I see that the problem is that the assumption that disabling the flag
is done when no virtual threads are running is not guaranteed (see my comment
there). So I think we are trying to address a case that shouldn't happen in the
first place. Also not sure if applying this restore in all cases will be
correct, since we might be somewhere at a transition. For example, a thread
could have blocked right in the return from notifyJvmtiUnmount() in
yieldContinuation(). It will looked like virtual because unmount() was not
executed yet, and the jvmti_thread_state should be that of the platform thread
because we never changed it when mounting. We should leave the state a
s is but in here we would change it to the virtual thread's jvmti state. The
only case I think it makes sense to do this restore steps when enabling the
flag is for those threads that are outside a transition with a mounted virtual
thread, since we want to adjust the jvmti_thread_state so that it looks right
on the next unmount.
But in any case this is also only needed when using the WhiteBox methods right?
In the intended case (no WhiteBox method used), after we execute this operation
to enable the events, we will create the JvmtiThreadStates later in
JvmtiExport::get_jvmti_interface() and the correct jvmti_thread_state and
jvmti_vthread will be already set for each JavaThread. In that case can we only
execute this restore code when using the WhiteBox API?
test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java
line 142:
> 140: TestedThread thread = threads[i];
> 141: if (thread == null) {
> 142: break;
Bailing out here means we could later disable the flag while there are virtual
threads running. If I comment out the first two sleeps in the main thread I can
see that issue happening. To avoid relying on timing I suggest using a
semaphore to wait at the beginning of finishThreads(), and signal at the end of
startThreads().
-------------
PR Review: https://git.openjdk.org/jdk/pull/13133#pullrequestreview-1362140557
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1152508618
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1152514856
