On Thu, 20 Nov 2025 23:10:48 GMT, Patricio Chilano Mateo
<[email protected]> wrote:
>> When `ThreadSnapshotFactory::get_thread_snapshot()` captures a snapshot of a
>> virtual thread, it uses `JvmtiVTMSTransitionDisabler` class to disable
>> mount/unmount transitions. However, this only works if a JVMTI agent has
>> attached to the VM, otherwise virtual threads don’t honor the disable
>> request. Since this snapshot mechanism is used by `jcmd Thread.dump_to_file`
>> and `HotSpotDiagnosticMXBean` which don’t require a JVMTI agent to be
>> present, getting the snapshot of a virtual thread in such cases can lead to
>> crashes.
>>
>> This patch moves the transition-disabling mechanism out of JVMTI and into a
>> new class, `MountUnmountDisabler`. The code has been updated so that
>> transitions can be disabled independently of JVMTI, making JVMTI just one
>> user of the API rather than the owner of the mechanism. Here is a summary of
>> the key changes:
>>
>> - Currently when a virtual thread starts a mount/unmount transition we only
>> need to check if `_VTMS_notify_jvmti_events` is set to decide if we need to
>> go to the slow path. With these changes, JVMTI is now only one user of the
>> API, so we instead check the actual transition disabling counters, i.e the
>> per-vthread counter and the global counter. Since these can be set at any
>> time (unlike `_VTMS_notify_jvmti_events` which is only set at startup or
>> during a safepoint in case of late binding agents), we follow the classic
>> Dekker pattern for the required synchronization. That is, the virtual thread
>> sets the “in transition” bits for the carrier and vthread *before* reading
>> the “transition disabled” counters. The thread requesting to disable
>> transitions increments the “transition disabled” counter *before* reading
>> the “in transition” bits.
>> An alternative that avoids the extra fence in `startTransition` would be to
>> place extra overhead on the thread requesting to disable transitions (e.g.
>> using safepoint, handshake-all, or UseSystemMemoryBarrier). Performance
>> analysis show no difference with current mainline so for now I kept this
>> simpler version.
>>
>> - Ending the transition doesn’t need to check if transitions are disabled
>> (equivalent to not need to poll when transitioning from unsafe to safe
>> safepoint state). But we still require to go through the slow path if there
>> is a JVMTI agent present, since we need to check for event posting and JVMTI
>> state rebinding. As such, the end transition follows the same pattern that
>> we have today of only needing to check `_VTMS_notify_jvmti_events`.
>>
>> - The code was previously structured in t...
>
> Patricio Chilano Mateo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Rename VM methods for endFirstTransition/startFinalTransition
src/hotspot/share/runtime/javaThread.cpp line 1173:
> 1171: bool JavaThread::java_suspend(bool register_vthread_SR) {
> 1172: #if INCLUDE_JVMTI
> 1173: // Suspending a JavaThread in VTMS transition or disabling VTMS
> transitions can cause deadlocks.
Q: I wonder if the `#if INCLUDE_JVMTI` and `#endif` can be removed here.
src/hotspot/share/runtime/mountUnmountDisabler.cpp line 126:
> 124: || global_start_transition_disable_count() > base_disable_count
> 125: JVMTI_ONLY(||
> (JvmtiVTSuspender::is_vthread_suspended(java_lang_Thread::thread_id(vthread))
> || thread->is_suspended()));
> 126: }
I like this approach with the JVMTIStartTransition and JVMTIEndTransition
helper classes. It is a nice way to decouple the JVMTI part of the protocol.
Introducing the `is_start_transition_disabled()` function was also long
desired. Also, I like the functions `start_transition()` and `end_transition()`
became pretty simple and clean!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2552502964
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2552624330
