RFR: 8364343: Virtual Thread transition management needs to be independent of JVM TI [v4]

[Patricio Chilano Mateo]

On Fri, 21 Nov 2025 00:42:32 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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/mountUnmountDisabler.cpp line 277:
> 
>> 275: 
>> 276:   // Start of the critical region. Prevent future memory
>> 277:   // operations to be ordered before we read the transition flag.
> 
> Does this refer to `java_lang_Thread::is_in_VTMS_transition(_vthread())`? If so perhaps that should internally perform the `load_acquire`?

Yes, but that would also call for doing the same with `JavaThread::_is_in_VTMS_transition` for the `VTMS_transition_disable_for_all` case, and also have the pairing release stores by the virtual thread in `end_transition` on those same addresses, otherwise it would be confusing. And same with the other side, i.e doing load_acquire by the virtual thread of `_VTMS_transition_disable_count` and `_global_start_transition_disable_count` on `start_transition` and release store by the disabler when enabling transitions again. But I wanted to avoid unnecessary barriers in the virtual thread transition side, so I kept them as plain load/stores with separate memory barriers when necessary.

> src/hotspot/share/runtime/mountUnmountDisabler.cpp line 307:
> 
>> 305:   // Block while some mount/unmount transitions are in progress.
>> 306:   // Debug version fails and prints diagnostic information.
>> 307:   for (JavaThreadIteratorWithHandle jtiwh; JavaThread *jt = jtiwh.next(); ) {
> 
> This looks very odd, having an assignment in the loop condition check and no actual loop-update expression.

Yes, from what I see this same construct is used in many places. Seems this is valid because a pointer used in a boolean context evaluates to false if nullptr and true if non-null. :)

> src/hotspot/share/runtime/mountUnmountDisabler.cpp line 316:
> 
>> 314:   // operations to be ordered before we read the transition flags.
>> 315:   // This matches the release fence in end_transition().
>> 316:   OrderAccess::acquire();
> 
> Surely the use of the iterator already provides the necessary ordering guarantee here as well. ?

We still need it because we need to prevent reordering of loads from the critical section with loads of `jt->is_in_VTMS_transition()`.

> src/hotspot/share/runtime/mountUnmountDisabler.cpp line 327:
> 
>> 325:   // End of the critical section. Prevent previous memory operations to
>> 326:   // be ordered after we clear the clear the disable transition flag.
>> 327:   // This matches the equivalent acquire fence in start_transition().
> 
> Suggestion:
> 
>   // This pairs with the acquire in start_transition().
> 
> I just realized you are using "fence" to describe release and acquire memory barrier semantics. Given we have an operation `fence` I find this confusing for the reader - especially when we also have a `release_store_fence` operation which might be confused with "release fence".

Right, I changed it now to use the terms acquire and release barrier respectively.

> src/hotspot/share/runtime/mountUnmountDisabler.cpp line 370:
> 
>> 368:   assert(VTMSTransition_lock->owned_by_self() || SafepointSynchronize::is_at_safepoint(), "Must be locked");
>> 369:   assert(_global_start_transition_disable_count >= 0, "");
>> 370:   AtomicAccess::store(&_global_start_transition_disable_count, _global_start_transition_disable_count + 1);
> 
> Suggestion:
> 
>   AtomicAccess::inc(&_global_start_transition_disable_count);

I’d prefer to leave it as a plain store to avoid the unnecessary extra fence.

> src/hotspot/share/runtime/mountUnmountDisabler.cpp line 376:
> 
>> 374:   assert(VTMSTransition_lock->owned_by_self() || SafepointSynchronize::is_at_safepoint(), "Must be locked");
>> 375:   assert(_global_start_transition_disable_count > 0, "");
>> 376:   AtomicAccess::store(&_global_start_transition_disable_count, _global_start_transition_disable_count - 1);
> 
> Suggestion:
> 
>   AtomicAccess::dec(&_global_start_transition_disable_count);

Same here.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561208616
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561210899
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561216984
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561219344
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561219842
PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2561220292