RFR: 8265327: Remove check_safepoint_and_suspend_for_native_trans() [v2]

[patricio.chilano.mateo at oracle.com]

Hi David,

Thanks for looking at this.

On 4/20/21 1:49 AM, David Holmes wrote:
> On Mon, 19 Apr 2021 21:39:18 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:
> Hi Patricio,
>
> Given `check_safepoint_and_suspend_for_native_trans` is a simple wrapper around `SafepointMechanism::process_if_requested_with_exit_check`, this "inlining" seems reasonable.
>
> I have a couple of minor comments/suggestions below but nothing blocking.
>
> Note that thread.cpp line 649 still refers to the now deleted method.
Yes, with Robbin's change that method will be gone so I'll wait for him 
to push first.

> Thanks,
> David
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 122:
>
>> 120:     // to the runtime from native code because the runtime is not set
>> 121:     // up to handle exceptions floating around at arbitrary points.
>> 122:     SafepointMechanism::process_if_requested_with_exit_check(thread, false /* check asyncs */);
> So previously we would check to see if there was a handshake/safepoint pending, or a suspend request, and if so process it. Now we batch the checks and processing together and actually check for additional conditions ie. deopt_suspend (which I'm guessing can't be set when returning from native?). So functionally this seems okay, but from a performance perspective there would seem to be a little more overhead in the case where there is nothing to do.
Today we are also checking for _obj_deopt since that could be set when 
coming back from native. The only extra check that will be done when 
using process_if_requested_with_exit_check() instead of 
'SafepointMechanism::should_process(thread) || 
thread->is_suspend_after_native()' is for async exceptions. (Note that 
today we are actually doing an extra check when calling 
SafepointMechanism::should_process() instead of just calling 
SafepointMechanism::local_poll_armed() as 
process_if_requested_with_exit_check() does, but that's a different 
issue and I filed 8265453 to fix that). If doing that extra check for 
async exceptions is a problem, instead of restoring the check 
'SafepointMechanism::should_process(thread) || 
thread->is_suspend_after_native()' I think a better thing to do would be 
to just have the boolean 'check_asyncs' in 
has_special_runtime_exit_condition() instead of 
handle_special_runtime_exit_condition(), since it makes sense to filter 
that already in the check rather than afterwards in the slow path when 
processing. That boolean is always known at compile time so the check 
for async exceptions will be removed for these cases. That will also fix 
all other places were we check for all conditions but then call 
handle_special_runtime_exit_condition() and filter async exceptions.

> An existing nit with process_if_requested_with_exit_check - the "exit check" pertains to calling handle_special_runtime_exit_condition, but here we are entering the runtime, not exiting it. I think the usage and naming of some of these methods is diverging. Possible future cleanup RFE.
Yes, maybe process_if_requested_with_special_condition_check()? The 
names are starting to get long. : )  I can do it in a different RFE then.

Thanks!
Patricio
> -------------
>
> Marked as reviewed by dholmes (Reviewer).
>
> PR: https://git.openjdk.java.net/jdk/pull/3548