On Mon, 19 Sep 2022 19:18:57 GMT, Serguei Spitsyn <sspit...@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
