On Fri, 3 Jun 2022 09:45:31 GMT, Johan Sjölén <d...@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
