On Thu, 20 Nov 2025 20:52:05 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:
>
> Add Alan's comment in VirtualThread
src/hotspot/share/classfile/javaClasses.cpp line 1757:
> 1755: jint* addr =
> java_thread->field_addr<jint>(_VTMS_transition_disable_count_offset);
> 1756: int val = AtomicAccess::load(addr);
> 1757: AtomicAccess::store(addr, val + 1);
Suggestion:
AtomicAccess::inc(addr);
src/hotspot/share/classfile/javaClasses.cpp line 1764:
> 1762: jint* addr =
> java_thread->field_addr<jint>(_VTMS_transition_disable_count_offset);
> 1763: int val = AtomicAccess::load(addr);
> 1764: AtomicAccess::store(addr, val - 1);
Suggestion:
AtomicAccess::dec(addr);
src/hotspot/share/opto/runtime.hpp line 740:
> 738: return vthread_transition_Type();
> 739: }
> 740:
I do not know C2 but this looks really strange - 4 different functions all
return the same thing. ???
src/hotspot/share/runtime/handshake.cpp line 374:
> 372: JavaThread* target = java_lang_Thread::thread(carrier_thread);
> 373: assert(target != nullptr, "");
> 374: // Technically there is need for a ThreadsListHandle since the target
Suggestion:
// Technically there is no need for a ThreadsListHandle since the target
?
src/hotspot/share/runtime/mountUnmountDisabler.cpp line 147:
> 145: MonitorLocker ml(VTMSTransition_lock);
> 146: while (is_start_transition_disabled(current, vth())) {
> 147: ml.wait(200);
I see a lot of timed-waits throughout this code. Is that because we poll rather
than synchronizing properly? All this potential busy-waiting is surely going to
cause performance glitches.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2547864726
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2547863852
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2547884313
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2547900707
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2547963241
