RFR: 8265327: Remove check_safepoint_and_suspend_for_native_trans() [v2]

[David Holmes]

On Mon, 19 Apr 2021 21:39:18 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Hi,
>> 
>> Please review this small fix. Method check_safepoint_and_suspend_for_native_trans() can just be removed and the code moved directly to check_special_condition_for_native_trans(). The other caller, transition_from_native(), can also just call SafepointMechanism::process_if_requested_with_exit_check() after transitioning to _thread_in_native_trans. Since that removes the only caller of is_suspend_after_native() I also removed it. 
>> I also made a small fix to has_special_runtime_exit_condition(). The compilers were generating three different comparisons for _suspend_flags instead of combining the constant values and doing a single one (I checked this with gcc 10.2 on linux and clang-12 on macos). The extra code was preventing inlining on some cases. For instance, on a Linux release build with gcc 10.2, for the ThreadInVMfromNative constructor, transition_from_native() was being inlined but not process_if_requested_with_exit_check(), so we still required a call for the fast case. The small change to has_special_runtime_exit_condition() fixed that. 
>> Tested in mach5 tiers1-2.
>> 
>> Thanks,
>> 
>> Patricio
>
> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix typo in comment for check_special_condition_for_native_trans

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.

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.

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.

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

Marked as reviewed by dholmes (Reviewer).

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