RFR: 8223312: Utilize handshakes instead of is_thread_fully_suspended [v3]

Wed Oct 21 17:48:28 UTC 2020

On Wed, 21 Oct 2020 08:40:47 GMT, Robbin Ehn <rehn at 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.
>
> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits:
> 
>  - Fixed merge miss
>  - Merge branch 'master' into 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
>  - Merge fix from Richard
>  - Merge branch 'master' into 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
>  - Removed TraceSuspendDebugBits
>  - Removed unused method is_ext_suspend_completed_with_lock
>  - Utilize handshakes instead of is_thread_fully_suspended

I don't think I have any "must fix" comments here.

I'm going to assume that my confusion about why there
is code from @reinrich's EscapeBarrier work here is
because of the merging of conflicts...

src/hotspot/share/prims/jvmtiEnv.cpp line 1646:

> 1644: // java_thread - pre-checked
> 1645: jvmtiError
> 1646: JvmtiEnv::PopFrame(JavaThread* java_thread) {

So I'm a bit confused why I'm seeing PopFrame() changes here that are
related to @reinrich's EscapeBarrier work. I've seen mention of picking
up a patch during this review from @reinrich so may that's why. I don't
see anything wrong with the changes, but I am confused why they are
here in this review.

src/hotspot/share/prims/jvmtiEnv.cpp line 1715:

> 1713:   }
> 1714: 
> 1715:   SetFramePopClosure op(this, state, depth);

The new closure is `SetFramePopClosure`, but the function
we are in is `NotifyFramePop()` so that seems like a mismatch.
Update: Okay, that just a move of existing code so this "mismatch"
is pre-existing.

src/hotspot/share/prims/jvmtiEnv.cpp line 1718:

> 1716:   MutexLocker mu(JvmtiThreadState_lock);
> 1717:   if (java_thread == JavaThread::current()) {
> 1718:     op.doit(java_thread, true);

Please add a comment after the `true` parameter to
indicate the name of the doit() function's parameter,
e.g., `true /* self */`.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 56:

> 54: #include "runtime/threadSMR.hpp"
> 55: #include "runtime/vframe.hpp"
> 56: #include "runtime/vframe.inline.hpp"

When you add `foo.inline.hpp` you delete `foo.hpp` because
the `foo.inline.hpp` file always includes the `foo.hpp` file.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 1311:

> 1309: // It is to keep a ret_ob_h handle alive after return to the caller.
> 1310: jvmtiError
> 1311: JvmtiEnvBase::check_top_frame(Thread* current_thread, JavaThread* java_thread,

Again, it is not clear why these changes to `check_top_frame` are here since they
appear to be related to @reinrich's work.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 1398:

> 1396:   SetForceEarlyReturn op(state, value, tos);
> 1397:   if (java_thread == current_thread) {
> 1398:     op.doit(java_thread, true);

Please add a comment after the true parameter to
indicate the name of the doit() function's parameter,
e.g., `true /* self */`.

src/hotspot/share/prims/jvmtiEnvBase.cpp line 1570:

> 1568: 
> 1569:   ResourceMark rm(current_thread);
> 1570:   // Check if there are more than one Java frame in this thread, that the top two frames

typo: s/are more/is more/

src/hotspot/share/prims/jvmtiEnvBase.cpp line 1543:

> 1541:   HandleMark hm(current_thread);
> 1542:   JavaThread* java_thread = target->as_Java_thread();
> 1543: 

This would be useful here:

`assert(_state->get_thread() == java_thread, "Must be");`

src/hotspot/share/prims/jvmtiEnvBase.cpp line 1642:

> 1640: 
> 1641:   if (!self) {
> 1642:     if (!java_thread->is_external_suspend()) {

You could join these two if-statements with `&&` and have
one less indenting level...

src/hotspot/share/prims/jvmtiEnvBase.cpp line 1661:

> 1659:   assert(vf->frame_pointer() != NULL, "frame pointer mustn't be NULL");
> 1660:   if (java_thread->is_exiting() || java_thread->threadObj() == NULL) {
> 1661:     return;

What's the `_result` value if this `return` executes?

src/hotspot/share/prims/jvmtiEnvBase.hpp line 361:

> 359:      _tos(tos) {}
> 360:   void do_thread(Thread *target) {
> 361:     doit(target, false);

Please add a comment after the true parameter to
indicate the name of the doit() function's parameter,
e.g., `false /* self */`.

src/hotspot/share/prims/jvmtiEnvBase.hpp line 395:

> 393:       _depth(depth) {}
> 394:   void do_thread(Thread *target) {
> 395:     doit(target, false);

Please add a comment after the true parameter to
indicate the name of the doit() function's parameter,
e.g., `false /* self */`.

src/hotspot/share/runtime/deoptimization.cpp line 1755:

> 1753:          thread->is_handshake_safe_for(Thread::current()) ||
> 1754:          SafepointSynchronize::is_at_safepoint(),
> 1755:          "can only deoptimize other thread at a safepoint");

Should that now be: `safepoint/handshake`??

src/hotspot/share/runtime/thread.cpp line 567:

> 565:     //
> 566:     //   _thread_in_native -> _thread_in_native_trans -> _thread_blocked
> 567:     //

This code should not be needed with the much simpler suspension mechanism.
(My agreement may come back to bite me if we have to change a suspend/resume
bug in the near future. :-) )

src/hotspot/share/runtime/thread.cpp line 698:

> 696:   RememberProcessedThread rpt(this);
> 697:   oops_do_no_frames(f, cf);
> 698:   oops_do_frames(f, cf);

In the comment above:
      // ... If we were
      // called by wait_for_ext_suspend_completion(), then it
      // will be doing the retries so we don't have to.
`wait_for_ext_suspend_completion()` has been deleted so the
comment needs work.

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

Marked as reviewed by dcubed (Reviewer).

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