RFR: 8265934: Cleanup _suspend_flags and _special_runtime_exit_condition

[Patricio Chilano Mateo]

On Tue, 11 May 2021 09:49:57 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Hi,
>> 
>> Please review the following patch which contains the following cleanups:
>> 
>> - Move _suspend_flags from Thread class to JavaThread
>> 
>> - Rename _special_runtime_exit_condition to _async_exception_condition. The name has been mixed up with the methods has_special_runtime_exit_condition() and handle_special_runtime_exit_condition() which check both async exception conditions and _suspend_flags. Also rename related methods:  clear_special_runtime_exit_condition() -> clear_async_exception_condition(), has_async_condition() -> has_async_exception_condition(). I also added set_async_exception_condition() just for completeness which is now called by set_pending_unsafe_access_error() and set_pending_async_exception().
>> 
>> - The _has_async_exception enum value in SuspendFlags creates a coupling between _suspend_flags and asynchronous exceptions. This allows us in the transition from native to Java wrappers to do a single load and comparison against the _suspend_flags field instead of having to do one for _suspend_flags and one for asynchronous exceptions. Since this is just an optimization I removed methods has_async_exception(), set_has_async_exception() and clear_has_async_exception() associated with _suspend_flags since they create confusion as to who actually manages asynchronous exceptions. Methods associated with _async_exception_condition should be used instead. I added a comment in set_pending_async_exception() as to why we use _suspend_flags. 
>> 
>> - Remove boolean parameter check_unsafe_error from check_and_handle_async_exceptions(). When we check for async exceptions we always process any async condition except in the transition from native->Java where we skip unsafe access error ones. I moved the boolean parameter to has_async_exception_condition() instead and fixed check_special_condition_for_native_trans(). Method check_and_handle_async_exceptions() could use some further cleanup but I'll do that in another RFR.
>> 
>> Tested in mach5 tiers 1-6.
>> 
>> Thanks,
>> Patricio
>
> src/hotspot/share/runtime/thread.cpp line 1621:
> 
>> 1619:     // ok because the call we a returning from already collides
>> 1620:     // with exception handling registers and so there is no issue.
>> 1621:     // (The exception handling path kills call result registers but
> 
> I'm not clear how you got rid of the check_unsafe_error. They are a special kind of async exception and the scope of checking for them was supposed to be very narrow.

Hi David,

> I'm not clear how you got rid of the check_unsafe_error. They are a special kind of async exception and the scope of checking for them was supposed to be very narrow.

The only time we call check_and_handle_async_exceptions() with check_unsafe_error=false is in check_special_condition_for_native_trans(), in the transition from native to Java, otherwise that argument is always true. I only moved the check to has_async_exception_condition() so for that particular case we only call check_and_handle_async_exceptions() for non _async_unsafe_access_error cases. 
That boolean is actually redundant today because in  check_special_condition_for_native_trans() we call has_async_exception() first which checks the _suspend_flag bit instead of _special_runtime_exit_condition, and that already only gets set for non _async_unsafe_access_error cases. Then in check_and_handle_async_exceptions() we always check for non _async_unsafe_access_error cases first.

Thanks,
Patricio

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

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