On Tue, 25 Nov 2025 23:10:43 GMT, Coleen Phillimore <[email protected]> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> keep preexisting rebind order for mount
>
> 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.
I dropped VTMS from all names.
> 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.
Added a comment.
> 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?
Because we set them in the intrinsic when trying to start the transition.
Method `MountUnmountDisabler::start_transition` expects them to be false so we
need to clear them in the opto versions.
> 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?
Added a comment for the class and this counter.
> 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?
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2566661560
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2566662105
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2566663466
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2566666104
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2566666238
