On Wed, 21 Oct 2020 13:46:14 GMT, Richard Reingruber <rr...@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 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.
It's not clear to me why the `JvmtiSuspendControl::print()` is being
eliminated. Please explain. The `TraceJVMTICalls` support is so that
someone can diagnose what JVM/TI calls are being made, including
context in some cases, so it seems wrong to delete this call.
> 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.
Agreed. @sspitsyn - This makes me wonder if the lack of
synchronization is the cause of some instability in the
JVM/TI ForceEarlyReturn() testing.
Update: The old code only made the updates if the thread was fully
suspended so you won't have a race between the requesting thread
and the target thread in that case.
> 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.
Especially if an assert() is added above on L1543.
> 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.
`is_ext_suspend_completed()` includes code that detects that a thread
that is in `_thread_in_native_trans` and does not yet have a walkable
stack has not completed suspension and we will do some retries in
this function until the target thread gets stable. We have to make sure
that the handshake mechanism has a similar stability guarantee or a
stack walker may fail intermittently.
-------------
PR: https://git.openjdk.java.net/jdk/pull/729
