On Mon, 6 Jun 2022 07:17:15 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> 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
>
> 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.
Having `Handshake::execute()` handle the current-thread case will certainly
allow us to make the code consistent in all the callers of
`Handshake::execute()`.
> 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. ??
Because we're pushing the special case handling for current-thread down
into the three parameter version of `Handshake::execute()`, we'll also
directly execute the closure's `do_thread()` function in other calls to the
three parameter version of `Handshake::execute()` where we didn't change
the calling code site in this patch:
- src/hotspot/share/classfile/javaClasses.cpp: async_get_stack_trace()
- src/hotspot/share/prims/jvmtiExtensions.cpp: GetCarrierThread()
- src/hotspot/share/prims/whitebox.cpp: WB_HandshakeReadMonitors(),
WB_HandshakeWalkStack()
- src/hotspot/share/runtime/handshake.cpp: execute(HandshakeClosure* hs_cl,
JavaThread* target)
Of course, since the two parameter version of `Handshake::execute()` is
now a changed code path, that means that all callers to the two parameter
version of `Handshake::execute()` are also affected. No, I'm not going to
list all those call sites.
This is a change in behavior and I'm not saying that this is wrong, but it's
not clear to me that the repercussions are understood and discussed in
this PR.
What I'm mumbling about here might be the same thing that @dholmes-ora is
worried about, but I'm just being more verbose about it. :-)
-------------
PR: https://git.openjdk.java.net/jdk/pull/8992
