RFR: 8293613: need to properly handle and hide tmp VTMS transitions [v3]

[Chris Plummer]

On Mon, 19 Sep 2022 19:18:57 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> There are several places in VirtualThread class implementation where virtual threads are being mounted or unmounted, so there is a transition of the JavaThread identity from carrier thread to virtual thread and back. The execution state in such transitions is inconsistent, and so, has to be invisible to JVM TI agents. Such transitions are named as VTMS (virtual thread mount state) transitions, and there is a JVM TI mechanism which supports them. Besides normal VTMS transitions there are also a couple of performance-critical contexts (in `VirtualThread` methods: `scheduleUnpark()`, `cancel()` and `unpark()`) which can be named as temporary VTMS transitions. Execution state of such temporary VTMS transitions has to be also invisible to the JVM TI agent which is implemented in the current update.
>> 
>> There are a couple of details of this fix to highlight:
>> -  A JavaThread which is in temporary VTMS transition is marked with a special bit `_is_in_tmp_VTMS_transition`. It is done with the native notification method `toggleJvmtiTmpVTMSTrans()`.
>> - A couple of lambda form classes can be created in context of temporary VTMS transitions, so their `ClassLoad`, `ClassPrepare` and `CFLH` events are ignored.
>> - Suspending threads in temporary transition is allowed.
>> 
>> The fix was tested in Loom repository by using `JTREG_MAIN_WRAPPER=Virtual` mode of execution which forces all main threads to be run as virtual. All `JVM TI` and `JDI` tests were run on all platforms. It includes `Kitchensink` and `JCK` tests. Additionally, the fix was tested in the jdk 20 repository. It includes `JVM TI` and `JDI` tests and tiers 1-6.
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fixed typo in VirtualThread.c

src/hotspot/share/prims/jvmtiEnvBase.cpp line 713:

> 711:   if (!jt->is_in_tmp_VTMS_transition()) {
> 712:     jvf = check_and_skip_hidden_frames(jt, jvf);
> 713:   }

I think this comment needs some help. It's hard to match it up with what the code is doing. I think you are saying you want to avoid skipping hidden frames when in transition, although if that's the case, it's not clear to me why not skipping is ok.

Also is skipping (or not skipping) ok regardless of the JvmtiEnvBase::is_cthread_with_continuation() result?

src/hotspot/share/prims/jvmtiExport.cpp line 1055:

> 1053:   if (JavaThread::current()->is_in_tmp_VTMS_transition()) {
> 1054:     return false;
> 1055:   }

You mentioned this in the PR description. However, it's not clear to me why this is ok.  Also, it should be commented.

Same thing for changes in JvmtiExport::post_class_load() and JvmtiExport::post_class_prepare().

src/hotspot/share/runtime/javaThread.cpp line 1174:

> 1172: #if INCLUDE_JVMTI
> 1173:   // Suspending a JavaThread in VTMS transition or disabling VTMS transitions can cause deadlocks.
> 1174:   assert(!is_in_non_tmp_VTMS_transition(), "no suspend allowed in VTMS transition");

IMHO, a non tmp VTMS transition should be considered a type of VTMS transition, so the assert check here does not really match the assert text. Also see my related comments below on naming and use of these flags and APIs.

src/hotspot/share/runtime/javaThread.hpp line 650:

> 648:   void set_is_in_VTMS_transition(bool val);
> 649:   bool is_in_tmp_VTMS_transition() const         { return _is_in_tmp_VTMS_transition; }
> 650:   void toggle_is_in_tmp_VTMS_transition()        { _is_in_tmp_VTMS_transition = !_is_in_tmp_VTMS_transition; };

The naming of the flags does not match up with the naming of the APIs, which is confusing.  An API named  is_in_VTMS_transition() should return _is_in_VTMS_transition. I think part of problem is that _is_in_VTMS_transition is not a superset of _is_in_tmp_VTMS_transition. The flags are never both set true, although both can be false. Maybe this should be changed to make it an invariant that if _is_in_tmp_VTMS_transit is true, then _is_in_tmp_VTMS_transition is true.

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

PR: https://git.openjdk.org/jdk/pull/10321