RFR: 8287281: adjust guarantee in Handshake::execute for the case of target thread being current [v4]

[David Holmes]

On Fri, 3 Jun 2022 09:45:31 GMT, Johan Sjölén <duke at openjdk.java.net> wrote:

>> Please review this PR for fixing JDK-8287281.
>> 
>> If a thread is handshake safe we immediately execute the closure, instead of going through the regular Handshake process. 
>> 
>> Finally:  Should `VirtualThreadGetThreadClosure` and its `do_thread()` body be inlined instead? We can do this in this PR, imho, but I'm hoping to get some input on this.
>> 
>> 
>> Passes tier1. Running tier2-5.
>
> Johan Sjölén has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - do_thread(target) not self
>  - Remove checks for is_handshake_for, instead call Handshake::execute
>  - Switch order of handshake check

Hi Johan,

I like the idea of this, but am not clear on all the details for all possible cases - see below.

It also makes me wonder about the async case, where `Handshake::execute(AsyncHandshakeClosure*, ...)` never processes the handshake directly even if it is for the current thread. The async case seems to be a two phase protocol:
1. Install async op on yourself
2. At some later handshake state poll discover the op you previously installed.
??

There are a few minor nits/suggestions below as well.

Thanks.

src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 399:

> 397:       // so get current location with direct handshake.
> 398:       GetCurrentLocationClosure op;
> 399:       Thread *current = Thread::current();

This appears unused now.

src/hotspot/share/prims/jvmtiEnvThreadState.cpp line 401:

> 399:       Thread *current = Thread::current();
> 400:       Handshake::execute(&op, thread);
> 401:       guarantee(op.completed(), "Handshake failed. Target thread is not alive?");

I much prefer that the current-thread case is internalised by `Handshake::execute` now. The code creating the handshake op shouldn't have to worry about current thread or not.

src/hotspot/share/prims/jvmtiEventController.cpp line 370:

> 368:   }
> 369:   EnterInterpOnlyModeClosure hs;
> 370:   assert(state->get_thread() != NULL, "sanity check");

Existing: We have already performed this check. We set `target` above and returned if it was `NULL`.

src/hotspot/share/runtime/handshake.cpp line 357:

> 355:   if (target->is_handshake_safe_for(self)) {
> 356:     hs_cl->do_thread(target);
> 357:     return;

I like the idea of doing this, but I can't quite convince myself that it will always be safe when the target is not the current thread. ??

src/hotspot/share/runtime/handshake.cpp line 360:

> 358:   }
> 359: 
> 360:   HandshakeOperation op(hs_cl, target, Thread::current());

Existing nit: this should pass `self` not re-invoke `Thread::current()`.

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

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8992