RFR: 8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers

[Daniel D.Daugherty]

On Mon, 8 Mar 2021 19:33:06 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Hi,
> 
> Please review the following small patch. ThreadInVMfromJavaNoAsyncException is exactly the same as ThreadInVMfromJava except that in the destructor we avoid checking for asynchronous exceptions. Similarly ThreadBlockInVMWithDeadlockCheck is equal to ThreadBlockInVM except that in the destructor we might need to release a Mutex in case we need to stop for a safepoint or handshake.
> We can consolidate these two wrappers with the existing ones to avoid code duplication and minimize the number of transition wrappers.
> 
> Tested in mach5 tiers 1-3.
> 
> Thanks,
> Patricio

I think there are non-trivial differences between the old ThreadBlockInVM
and the new ThreadBlockInVM. Maybe I'm wrong, but they do not look
equivalent to me.

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 155:

> 153:   bool _check_asyncs;
> 154:  public:
> 155:   ThreadInVMfromJava(JavaThread* thread, bool check_async = true) : ThreadStateTransition(thread), _check_asyncs(check_async) {

Field is `_check_asyncs` and parameter is `check_async`.
Either add an `s` to the parameter or delete the `s` from the field.

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 237:

> 235: 
> 236:  public:
> 237:   ThreadBlockInVM(JavaThread* thread, Mutex** in_flight_mutex_addr = NULL)

The new ThreadBlockInVM is not equivalent to the old one.
The old one used `trans(_thread_in_vm, _thread_blocked)` which
asserted that the "from" state is equal to `_thread_in_vm`. This
new code does not do that. The trans() call also resulted in a
`SafepointMechanism::process_if_requested(thread);` call after
we transitioned the thread to `_thread_in_vm`+1 and before we
set the new thread state to `_thread_blocked`.

src/hotspot/share/runtime/interfaceSupport.inline.hpp line 242:

> 240:     // Once we are blocked vm expects stack to be walkable
> 241:     thread->frame_anchor()->make_walkable(thread);
> 242:     thread->set_thread_state(_thread_blocked);

If you're going to set the thread_state directly here, I think you need
to keep the `OrderAccess::storestore()` call. However, this new ThreadBlockInVM
is also missing a `SafepointMechanism::process_if_requested(thread)` call that
happened before setting the `_thread_blocked` state.

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

Changes requested by dcubed (Reviewer).

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