On Tue, 25 Nov 2025 22:59:59 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:
>
> keep preexisting rebind order for mount
First read through, mostly questions and plea for comments. This is a nice
refactoring and cleanup of some very difficult code. You don't have to do the
renaming that I requested now if you don't want to.
src/hotspot/share/classfile/javaClasses.cpp line 1688:
> 1686: int java_lang_Thread::_jvmti_thread_state_offset;
> 1687: int java_lang_Thread::_VTMS_transition_disable_count_offset;
> 1688: int java_lang_Thread::_is_in_VTMS_transition_offset;
Since you're renaming these anyway, can we drop the VTMS part? Just call it
vthread_transition_disable_count_offset and is_in_vthread_transition_offset?
There are other VTMS named things that aren't these flags but they can stay.
Maybe migrate other names at some future point.
src/hotspot/share/opto/library_call.cpp line 3046:
> 3044: }
> 3045:
> 3046: bool LibraryCallKit::inline_native_vthread_start_transition(address
> funcAddr, const char* funcName, bool is_final_transition) {
Would it be helpful to add a comment above this to say what this does? This is
supposed to match some non-intrinsic code and might be helpful if you
referenced that here.
src/hotspot/share/prims/jvm.cpp line 3671:
> 3669:
> 3670: JVM_ENTRY(void, JVM_VirtualThreadStartFinalTransition(JNIEnv* env,
> jobject vthread))
> 3671: oop vt = JNIHandles::resolve_external_guard(vthread);
Why do the opto runtime versions set is_in_VTMTS_transition in both the
java.lang.Thread and JavaThread and these don't?
src/hotspot/share/prims/jvmtiEnv.cpp line 1827:
> 1825: JvmtiEnv::ClearAllFramePops(jthread thread) {
> 1826: ResourceMark rm;
> 1827: MountUnmountDisabler disabler(thread);
Not for this change but I thought JVMTI had some xml code that generated
prefixes for these functions. This seems like something that could be unified
somewhere tbd.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1772:
> 1770:
> 1771: assert(java_thread != nullptr, "sanity check");
> 1772: assert(!java_thread->is_in_VTMS_transition(), "sanity check");
Why don't you need these asserts anymore?
src/hotspot/share/runtime/javaThread.cpp line 1152:
> 1150: bool JavaThread::is_in_VTMS_transition() const {
> 1151: return AtomicAccess::load(&_is_in_VTMS_transition);
> 1152: }
Is the JavaThread version always the same as the
java_lang_Thread::is_in_VTMS_transition(threadOop()) value?
src/hotspot/share/runtime/mountUnmountDisabler.hpp line 34:
> 32:
> 33: class MountUnmountDisabler : public AnyObj {
> 34: static volatile int _global_start_transition_disable_count;
Can you describe this variable - when is it set and why is there a global
disabler? What does it mean to have 'n' active disablers?
A comment at the beginning of MountUnmountDisabler to say something of the
effect that during virtual thread mounting and unmounting, JVMTI and operations
that need to examine thread state need to be disabled. Or is it the converse?
During JVMTI and operations that examine the state of threads, virtual thread
mounting and unmounting must wait until these operations are complete. This
class is for the latter right?
src/hotspot/share/runtime/mutexLocker.cpp line 52:
> 50: Mutex* JvmtiThreadState_lock = nullptr;
> 51: Monitor* EscapeBarrier_lock = nullptr;
> 52: Monitor* VTMSTransition_lock = nullptr;
oh you could drop the name VTMS and call it VThreadTransitionLock can't you?
-------------
PR Review: https://git.openjdk.org/jdk/pull/28361#pullrequestreview-3507302896
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561864174
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561876549
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561897865
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561904709
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561910057
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561926510
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561943253
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561945493
