On Sat, 18 Nov 2023 14:35:58 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 631:
>>
>>> 629: return !jdk_internal_vm_Continuation::done(cont) &&
>>> 630: java_lang_VirtualThread::state(vt) !=
>>> java_lang_VirtualThread::NEW &&
>>> 631: java_lang_VirtualThread::state(vt) !=
>>> java_lang_VirtualThread::TERMINATED;
>>
>> AFAIU `jdk_internal_vm_Continuation::done(cont)` is correct check that
>> vthread is terminated and works for both mounted and unmounted vthreads.
>> Then `java_lang_VirtualThread::state(vt) !=
>> java_lang_VirtualThread::TERMINATED` check is not needed
>
> Good suggestion, thanks!
Added the fix suggested by Alex.
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1989:
>>
>>> 1987: } else {
>>> 1988: Handshake::execute(hs_cl, tlh, target_jt); // delegate to
>>> Handshake implementation
>>> 1989: }
>>
>> Every implementation of JvmtiUnitedHandshakeClosure has to check if the
>> target thread is virtual and call do_vthread manually.
>> I'd suggest to handle this by proxy class, something like
>> Suggestion:
>>
>> class Adapter : public HandshakeClosure {
>> JvmtiUnitedHandshakeClosure* _hs_cl;
>> Handle _target_h;
>> public:
>> Adapter(JvmtiUnitedHandshakeClosure* hs_cl, Handle target_h)
>> : HandshakeClosure(hs_cl->name()), _hs_cl(hs_cl),
>> _target_h(target_h) {}
>> virtual void do_thread(Thread* thread) {
>> if (java_lang_VirtualThread::is_instance(_target_h())) { // virtual
>> thread
>> _hs_cl->do_vthread(_target_h);
>> } else {
>> _hs_cl->do_thread(target);
>> }
>> }
>> } adapter(hs_cl, target_h);
>>
>> if (self) { // target thread is current
>> adapter.do_thread(target_jt); // execute handshake closure callback on
>> current thread directly
>> } else {
>> Handshake::execute(&adapter, tlh, target_jt); // delegate to Handshake
>> implementation
>> }
>
> Thank you for the suggestion! Agreed, this should help to get rid of this
> duplication/ugliness.
Added the fix suggested by Alex.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16460#discussion_r1398324679
PR Review Comment: https://git.openjdk.org/jdk/pull/16460#discussion_r1398324619
