On Tue, 2 Dec 2025 20:12:47 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 with a new target base > due to a merge or a rebase. The incremental webrev excludes the unrelated > changes brought in by the merge/rebase. The pull request contains 13 > additional commits since the last revision: > > - Merge branch 'master' into JDK-8364343 > - rebind in end_transition only for mount case > - Fix comment in inline_native_vthread_start_transition > - missing to initialize _is_disabler_at_start > - More changes from Coleen's review > - Drop VTMS from names > - keep preexisting rebind order for mount > - Remove INCLUDE_JVMTI macro > - David's comments > - Rename VM methods for endFirstTransition/startFinalTransition > - ... and 3 more: https://git.openjdk.org/jdk/compare/4b22489c...1f1cd594 Marked as reviewed by sspitsyn (Reviewer). Yes, this is a great work! src/hotspot/share/runtime/mountUnmountDisabler.cpp line 87: > 85: } > 86: DEBUG_ONLY(bool is_virtual = > java_lang_VirtualThread::is_instance(_current->jvmti_vthread())); > 87: assert((_is_mount && is_virtual) || (!_is_mount && !is_virtual), > "wrong identity"); Nit: Thank you for update! This check can be simplified with: assert(_is_mount == is_virtual), "wrong identity"); But it becomes more obscure though. Feel free to ignore this comment. ------------- PR Review: https://git.openjdk.org/jdk/pull/28361#pullrequestreview-3531985682 PR Comment: https://git.openjdk.org/jdk/pull/28361#issuecomment-3603842122 PR Review Comment: https://git.openjdk.org/jdk/pull/28361#discussion_r2582681780
