On Wed, 9 Oct 2024 22:58:33 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> This fixes a problem in the VTMS (Virtual Thread Mount State) transition
>> frames hiding mechanism.
>> Please, see a fix description in the first comment.
>>
>> Testing:
>> - Verified with new test `vthread/CheckHiddenFrames`
>> - Mach5 tiers 1-6 are passed
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Disallow NotifyFramePop for
> enter/enter0/VirtualThread.run/VThreadContinuation.run
src/hotspot/share/prims/jvmtiEnvBase.cpp line 692:
> 690: if (jt->is_in_VTMS_transition()) {
> 691: jvf = check_and_skip_hidden_frames(jt->is_in_VTMS_transition(), jvf);
> 692: } else if (is_virtual) { // filter out pure continuations
Not sure I follow the logic here.
As far as I understand yield/yield need to be filtered out only for unmounted
virtual threads. But `is_virtual` here means we have mounted VT?
Looks like almost all callers call this method only if
`jt->is_in_JTMS_transilition()` is true
Exception is a call from `get_vthread_jvf()` when vthread is mounted.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 703:
> 701: if (java_lang_VirtualThread::is_instance(vthread)) { // paranoid check
> for safety
> 702: if (java_lang_Thread::is_in_VTMS_transition(vthread)) {
> 703: jvf =
> check_and_skip_hidden_frames(java_lang_Thread::is_in_VTMS_transition(vthread),
> jvf);
it's just checked that `java_lang_Thread::is_in_VTMS_transition(vthread)` is
true
Suggestion:
jvf = check_and_skip_hidden_frames(true, jvf);
src/hotspot/share/prims/jvmtiEnvBase.cpp line 2025:
> 2023: // - it is a good invariant when a thread's handshake can't be
> impacted by a VTMS transition
> 2024: // - there is no mechanism to disable transitions of a specific
> carrier thread yet
> 2025: JvmtiVTMSTransitionDisabler disabler(is_virtual ? target : nullptr);
> // nullptr is to disable all
We have a number of places with the same issue - `JvmtiVTMSTransitionDisabler
disabler(target)` when target thread can be virtual or platform.
I think they need to be fixed all together (and most likely as a separate
issue).
Maybe it would be better to fix disabler itself (check if the thread provided
is platform and disable transitions for all threads in the case). Then there is
no need to update all this places when (if) disabling for single platform
thread is implemented
src/hotspot/share/prims/jvmtiExport.cpp line 1814:
> 1812: }
> 1813: // pure continuations have no VTMS transitions but they use methods
> annotated with JvmtiMountTransition
> 1814: if ((mh->jvmti_mount_transition() && state->is_virtual()) ||
> thread->is_in_any_VTMS_transition()) {
Could you please explain the change (and other similar changes in
jvmtiExport.cpp)
Before events were not posted for any `@JvmtiMountTransition` methods, and now
only for virtual threads? I.e. events for `@JvmtiMountTransition` methods of
carrier threads are posted?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801933054
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801953309
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1801992736
PR Review Comment: https://git.openjdk.org/jdk/pull/21397#discussion_r1802034030
