RFR: 8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers [v2]

[patricio.chilano.mateo at oracle.com]

On 3/11/21 6:10 PM, Daniel D.Daugherty wrote:
> On Tue, 9 Mar 2021 20:32:18 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
>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>>
>>    add asserts for thread state + check_asyncs name
> Patricio and I have bounced this change in behavior back and forth
> in Slack. I'm now convinced that it is a benign change in behavior.
>
> -------------
Great, thanks Dan!
Just for the record on the previous concerns below..

>> 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.
> I think you're missing my point about the equivalence of the old ThreadBlockInVM()
> and the new ThreadBlockInVM(). The old version did this in trans()/transition():
>
>      // Change to transition state and ensure it is seen by the VM thread.
>      thread->set_thread_state_fence((JavaThreadState)(from + 1));
>
>      SafepointMechanism::process_if_requested(thread);
>      thread->set_thread_state(to);
>
> which means that when we were in (`_thread_blocked`+1) we did a
> `process_if_requested(thread)` call before we changed the thread's
> state to `_thread_blocked`. In the new code, we just got straight to
> `_thread_blocked` without processing any pending request. I think the
> only case that matters is that we won't process a pending handshake
> request. As you say, a pending safepoint will be allowed to proceed
> once we've reached `_thread_blocked`.
>
> It could be that this change in behavior doesn't make any real difference,
> but I want to be sure.
When the JT changes its state to blocked it will also be handshake safe, 
so both safepoints and handshakes will be able to continue. Also polling 
in the constructor doesn't really change anything because it could be 
that the poll gets armed right after calling 
SafepointMechanism::process_if_requested(), so we should still be able 
to handle that scenario in the first place.


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