On Mon, 6 Nov 2023 23:22:03 GMT, Serguei Spitsyn <sspit...@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
