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

[Robbin Ehn]

On Wed, 21 Oct 2020 16:54:48 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> 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
>
> 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 */`.

Fixed

> 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.

Fixed

> 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.

Long story:
Before async handshakes was integrate I had a patch which does the same as this.
This change was accidentally slipped into async handshakes change-set and was integrated.
Richard notice this, I told him he could revert it in his change-set for EB, so he did.
But now we need this change, so here it comes once more!

VM thread is allowed to execute these handshakes, thus when calling check_top_frame() from SetForceEarlyReturn::doit() it's just a Thread*.

> 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 */`.

Fixed

> 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");`

Fixed.

> 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/

Fixed

> 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...

Fixed

> 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 */`.

Fixed

> 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 */`.

Fixed

> 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`??

Fixed

> 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.

I just delete the no longer relevant parts.

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

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