RFR: 8319244: implement JVMTI handshakes support for virtual threads [v4]

[Patricio Chilano Mateo]

On Mon, 6 Nov 2023 23:22:03 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> The handshakes support for virtual threads is needed to simplify the JVMTI implementation for virtual threads. There is a significant duplication in the JVMTI code to differentiate code intended to support platform, virtual threads or both. The handshakes are unified, so it is enough to define just one handshake for both platform and virtual thread.
>> At the low level, the JVMTI code supporting platform and virtual threads still can be different.
>> This implementation is based on the `JvmtiVTMSTransitionDisabler` class.
>> 
>> The internal API includes two new classes:
>>   - `JvmtiHandshake` and `JvmtiUnifiedHandshakeClosure`
>>   
>>   The `JvmtiUnifiedHandshakeClosure` defines two different callback functions: `do_thread()` and `do_vthread()`.
>> 
>> The first JVMTI functions are picked first to be converted to use the `JvmtiHandshake`:
>>  - `GetStackTrace`, `GetFrameCount`, `GetFrameLocation`, `NotifyFramePop`
>> 
>> To get the test results clean, the update also fixes the test issue:
>> [8318631](https://bugs.openjdk.org/browse/JDK-8318631): GetStackTraceSuspendedStressTest.java failed with "check_jvmti_status: JVMTI function returned error: JVMTI_ERROR_THREAD_NOT_ALIVE (15)" 
>> 
>> Testing:
>>  - the mach5 tiers 1-6 are all passed
>
> Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review: get rid of the VM_HandshakeUnmountedVirtualThread

Hi Serguei,


Looks good to me, nice code consolidation.

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

> 1972: 
> 1973:   if (java_lang_VirtualThread::is_instance(target_h())) { // virtual thread
> 1974:     if (!JvmtiEnvBase::is_vthread_alive(target_h())) {

There is only one issue I see in how this check is implemented and the removal of the VM_op for unmounted vthreads. The change of state to TERMINATED happens after notifyJvmtiUnmount(), i.e we can see that this vthread is alive here but a check later can return is not. This might hit the assert in JvmtiEnvBase::get_vthread_jvf() (maybe this the issue you saw on your first prototype). We can either change that order at the Java level, or maybe better change this function to read the state and add a case where if the state is RUNNING check whether the continuation is done or not (jdk_internal_vm_Continuation::done(cont)).

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

> 1976:     }
> 1977:     if (target_jt == nullptr) {    // unmounted virtual  thread
> 1978:       hs_cl->do_vthread(target_h); // execute handshake closure callback on current thread directly

I think comment should be: s/current thread/unmounted vthread

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

> 2414:   if (!JvmtiEnvBase::is_vthread_alive(_target_h())) {
> 2415:     return; // JVMTI_ERROR_THREAD_NOT_ALIVE (default)
> 2416:   }

Don't we have this check already in JvmtiHandshake::execute()? Same with the other converted functions.

src/hotspot/share/prims/jvmtiEnvBase.hpp line 490:

> 488: class JvmtiHandshake : public Handshake {
> 489:  protected:
> 490:   static bool is_vthread_handshake_safe(JavaThread* thread, oop vt);

Not defined, leftover?

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

PR Review: https://git.openjdk.org/jdk/pull/16460#pullrequestreview-1717815943
PR Review Comment: https://git.openjdk.org/jdk/pull/16460#discussion_r1385033726
PR Review Comment: https://git.openjdk.org/jdk/pull/16460#discussion_r1384994419
PR Review Comment: https://git.openjdk.org/jdk/pull/16460#discussion_r1384999063
PR Review Comment: https://git.openjdk.org/jdk/pull/16460#discussion_r1385002852