RFR: 8255384: Remove special_runtime_exit_condition() check from SS::block() [v4]

[Daniel D.Daugherty]

On Thu, 5 Nov 2020 19:44:16 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Hi all,
>> 
>> Please review the following patch that removes the call to handle_special_runtime_exit_condition() from SS::block(). This avoids recursive calls when transitioning and stopping for safepoints and also makes the code simpler to read since it is not trivial to deduce why we need to execute the check for certain states but not others, i.e. what exact scenarios we are trying to guard against.
>> 
>> Method handle_special_runtime_exit_condition() checks for external suspends, object deoptimization, async exceptions and JFR suspends. All these need to be checked when transitioning to java and when transitioning out of native (except for async exceptions when transitioning to thread_in_vm). In SS::block() this check is executed for the _thread_new_trans, _thread_in_native_trans and thread_in_Java cases. For _thread_new_trans, we know the JT will have to go through JavaCallWrapper() the first time it transitions to Java and that already has a check for handle_special_runtime_exit_condition(). For _thread_in_native_trans, transitioning out of native already has checks for external suspends, object deoptimization and JFR suspends in check_safepoint_and_suspend_for_native_trans() which is called from ThreadStateTransition::transition_from_native()(called either directly or through the ThreadStateTransition wrappers) and check_special_condition_for_native_trans (for native wrappers c
 ase). So that leaves the thread_in_Java case.
>> Careful analysis shows the handle_special_runtime_exit_condition() call in SS::block() prevents JTs transitioning back to Java from escaping after being externally suspended. This can happen when calling SafepointMechanism::process_if_requested() while transitiong back to java without a later check for external suspend. Looking at the callers of SafepointMechanism::process_if_requested() we see that this can happen from handle_polling_page_exception(), java_suspend_self_with_safepoint_check() and check_safepoint_and_suspend_for_native_trans(). An example of this can be shown for the handle_polling_page_exception() case:
>>     - JT hits a poll exception while executing nmethod.
>>     - JT calls handle_polling_page_exception() ( which doesn't use ThreadStateTransition wrappers) and calls SafepointMechanism::process_if_requested()
>>     - Stops for a safepoint due to a VM_ThreadSuspend request
>>     - Upon unblocking from the safepoint, unless we have the check in SS::block() the JT will transition back to java without actually suspending
>> 
>> The "escape from suspend" scenarios for the other callers of SafepointMechanism::process_if_requested() are described in the comments of the bug as well as the proper fixes.
>> 
>> I have tested the patch several times in mach5 tiers1-7 and saw no issues. Let me know if you think I should run any other special tests.
>> 
>> Thanks,
>> Patricio
>
> Patricio Chilano Mateo has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Merge branch 'master' into 8255384-SSBlock
>  - Add SafepointMechanism::process_if_requested_with_exit_check()
>  - Merge branch 'master' into 8255384-SSBlock
>  - Add explicit bool arg
>  - Make direct calls instead of using transition wrappers
>  - v1

Okay. It took me some time to be convinced of the equivalence,
but I'm on-board at this point. The new code is much more clear
and easy to reason about (once you convince yourself that the
new, simpler code is equivalent to the older code).

As @dholmes-ora likes to say: the proof is in the testing. I'll add
that the proof will be in the stress testing.

src/hotspot/share/runtime/safepoint.cpp line 961:

> 959:     // as otherwise we may never deliver it.
> 960:     if (self->has_async_condition()) {
> 961:       ThreadInVMfromJavaNoAsyncException __tiv(self);

It's not clear to me why this if-block is deleted.
You change to call SafepointMechanism::process_if_requested_with_exit_check(),
but I don't see equivalent code there.

Update: I found it in JavaThread::handle_special_runtime_exit_condition().

src/hotspot/share/runtime/safepoint.cpp line 982:

> 980:       }
> 981:     }
> 982:   }

It's not clear to me why this if-block is deleted.
You change to call SafepointMechanism::process_if_requested_with_exit_check(),
but I don't see equivalent code there.

Update: I found it in JavaThread::handle_special_runtime_exit_condition().

src/hotspot/share/runtime/safepoint.cpp line 952:

> 950:     // We never deliver an async exception at a polling point as the
> 951:     // compiler may not have an exception handler for it. The polling
> 952:     // code will notice the async and deoptimize and the exception will

Perhaps a little rewording:

    code will notice the pending async exception, deoptimize and the exception will

and then reformat the block comment a bit.

src/hotspot/share/runtime/safepointMechanism.cpp line 83:

> 81:     // Otherwise we might load an old safepoint counter (for example).
> 82:     OrderAccess::loadload();
> 83:     SafepointSynchronize::block(thread);

Wasn't that comment just added recently? :-)

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

> 2508: // Also check for pending async exception (not including unsafe access error).
> 2509: // Note only the native==>VM/Java barriers can call this function and when
> 2510: // thread state is _thread_in_native_trans.

It's not clear to me why this if-block is deleted.
You change to call SafepointMechanism::process_if_requested_with_exit_check(),
but I don't see equivalent code there.

Update: I found it in JavaThread::handle_special_runtime_exit_condition().

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

> 2510: // thread state is _thread_in_native_trans.
> 2511: void JavaThread::check_special_condition_for_native_trans(JavaThread *thread) {
> 2512:   check_safepoint_and_suspend_for_native_trans(thread);

Where's the equivalent of this JFR code?

Update: I found it in JavaThread::handle_special_runtime_exit_condition().

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

> 2501:   SafepointMechanism::process_if_requested_with_exit_check(thread, false /* check asyncs */);
> 2502: }
> 2503: 

I found the equivalent of this part of the if-statement in
JavaThread::handle_special_runtime_exit_condition().

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

Marked as reviewed by dcubed (Reviewer).

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