On Mon, 19 Oct 2020 09:59:34 GMT, Robbin Ehn <r...@openjdk.org> wrote:
> The main point of this change-set is to make it easier to implement S/R on
> top of handshakes.
> Which is a prerequisite for removing _suspend_flag (which duplicates the
> handshake functionality).
> But we also remove some complicated S/R methods.
>
> We basically just put in everything in the handshake closure, so the diff
> just looks much worse than what it is.
>
> TraceSuspendDebugBits have an ifdef, but in both cases it now just returns.
> But I was unsure if I should remove now or when is_ext_suspend_completed() is
> removed.
>
> Passes multiple t1-5 runs, locally it passes many
> jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs.
Hi Robbin,
IIUC the "waiting" part of `wait_for_ext_suspend_completion` is now implicitly
handled in the handshake - correct?
Overall this seems like a great simplification.
A few minor comments below.
Thanks,
David
src/hotspot/share/runtime/thread.cpp line 579:
> 577: // That trace is very chatty.
> 578: return;
> 579: #else
Without the !is_wait check none of the code below line 583 is reachable now. I
would remove this now.
src/hotspot/share/prims/jvmtiEnv.cpp line 1648:
> 1646: op.doit(java_thread, true);
> 1647: } else {
> 1648: Handshake::execute(&op, java_thread);
This pattern is repeated a lot - we should be able to incorporate it into the
op itself by passing in `java_thread`.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1525:
> 1523: Thread* current_thread = Thread::current();
> 1524: HandleMark hm(current_thread);
> 1525: JavaThread* java_thread = target->as_Java_thread();
Contrast with the same three lines at L1390 - we should use the same
boilerplate in each `doit`. And ideally refactor
into some shared code somewhere (future RFE).
-------------
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/729
