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

[Richard Reingruber]

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

The change is good.
I've only added a few comments (nothing important really).
Thanks, also for giving precedence to me ;)

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

> 1629:     _state->set_pending_step_for_popframe();
> 1630:     _result = JVMTI_ERROR_NONE;
> 1631:   }

I'd suggest to eliminate jt and use java_thread instead. Also because you're using java_thread in line 1626. The assertion should check if `_state->get_thread() == target` then.

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

> 1806:     }
> 1807:     if (java_lang_Class::is_primitive(k_mirror)) {
> 1808:       return JVMTI_ERROR_NONE;

The call of JvmtiSuspendControl::print() seems to be eliminated. Ok for me.

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

> 1452:   _state->set_earlyret_pending();
> 1453:   _state->set_earlyret_oop(ret_ob_h());
> 1454:   _state->set_earlyret_value(_value, _tos);

Good that these updates are done with a handshake now. Maybe I'm missing s.th. but I don't see synchronization in the older version.

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

> 308:                                 GrowableArray<jvmtiMonitorStackDepthInfo*> *owned_monitors_list);
> 309:   static jvmtiError check_top_frame(Thread* current_thread, JavaThread* java_thread,
> 310:                              jvalue value, TosState tos, Handle* ret_ob_h);

Maybe fix indentation?

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

> 1769:     Deoptimization::deoptimize_frame_internal(thread, id, reason);
> 1770:   } else {
> 1771:     VM_DeoptimizeFrame deopt(thread, id, reason);

I guess VM_DeoptimizeFrame can be replaced with a handshake too now.

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

> 535: // cancelled). Returns true if the thread is externally suspended and
> 536: // false otherwise.
> 537: bool JavaThread::is_ext_suspend_completed() {

I'd think `JavaThread::is_ext_suspend_completed` can be removed also (as a separate enhancement). It also duplicates code of the handshake mechanism. Just replace VM_ThreadSuspend with a handshake.

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

Marked as reviewed by rrich (Committer).

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