RFR: 8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers

[patricio.chilano.mateo at oracle.com]

Hi Dan,

Thanks for looking at this.

On 3/9/21 12:53 PM, Daniel D.Daugherty wrote:
> 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:
Fixed.


>> 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.
Fixed. I added an assert() that the thread state is _thread_in_vm in 
TBIVM() and _thread_blocked in ~TBIVM().


> 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.
Calling explicitly SafepointMechanism::process_if_requested() in TBIVM() 
is not needed because we are setting the state to blocked. That will 
already allow any safepoint/handshake to proceed. If when reaching 
~TBIVM() the poll is armed then we will go through 
process_if_requested() there.
The reason I removed OrderAccess::storestore() is because 
thread->set_thread_state() is a release store already.


Thanks,
Patricio
> -------------
>
> Changes requested by dcubed (Reviewer).
>
> PR: https://git.openjdk.java.net/jdk/pull/2880