RFR: 8265327: Remove check_safepoint_and_suspend_for_native_trans()

[patricio.chilano.mateo at oracle.com]

Hi Dan,

Thanks for looking at this.

On 4/19/21 4:45 PM, Daniel D.Daugherty wrote:
> On Fri, 16 Apr 2021 16:24:53 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
> One nit and one question.
>
> Thumbs up.
>
> src/hotspot/share/runtime/thread.cpp line 2150:
>
>> 2148: // pending, when _suspend_flags is non-zero or when we need to process a stack
>> 2149: // watermark. Also check for pending async exceptions (except unsafe access error).
>> 2150: // Note only the native==>Java barriers can call this function and when
> nit typo: s/and when/when/
Fixed.

> src/hotspot/share/runtime/thread.hpp line 1325:
>
>> 1323:     // under SR_lock in java_suspend_self().
>> 1324:     return (_special_runtime_exit_condition != _no_async_condition) ||
>> 1325:            (_suspend_flags & (_external_suspend | _obj_deopt JFR_ONLY(| _trace_flag))) != 0;
> I don't see this change mentioned in the bug report and it's
> not obvious why you're changing this code.
So before I made this change to has_special_runtime_exit_condition() I 
checked the generated code for the ThreadInVMfromNative constructor in 
libjvm.so and noticed we were making a call to 
process_if_requested_with_exit_check(), so it wasn't inlined as I was 
expecting. Then I checked the generated code for 
process_if_requested_with_exit_check() and saw these extra checks for 
_suspend_flags which were most likely making the method too big to be 
inlined. After this small fix I checked again and the call to 
process_if_requested_with_exit_check() was now also being inlined. I 
spotted this issue on a linux machine, compiling with gcc 10.2. I 
checked also on macos(compiling with clang 12) but everything was 
already inlined even without this change to 
has_special_runtime_exit_condition(). I will add this to the bug report.

> Also, I think this
> is going to conflict with @robehn fix for JDK-8257831.
Right, is_external_suspend() will be gone with Robbin's change, so the 
single check will be just for is_trace_suspend() || 
is_obj_deopt_suspend(). I'll check with him to confirm when he will be 
integrating.

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