RFR: 8272526: Cleanup ThreadStateTransition class [v2]

[David Holmes]

On Tue, 17 Aug 2021 16:47:35 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 119:
>> 
>>> 117: 
>>> 118:     // Change to transition state and ensure it is seen by the VM thread.
>>> 119:     thread->set_thread_state_fence(_thread_in_native_trans);
>> 
>> Why do we no longer need _thread_in_native_trans? If it is truly not needed here then it seems to no longer be used?
>
> We just need to transition to an unsafe state before polling to avoid the issue of a safepoint/handshake starting right after we checked but before we transition to the unsafe state. Since we are in the vm we can use _thread_in_vm instead which is already unsafe. We still use the _thread_in_native_trans state on the native wrappers when transitioning out of native back to Java, but I will try to address that in another RFE.

I kind of liked the trans states as indicators of unsafe states as it made the logic consistent and simple:
- advance to current state + 1 (trans state)
- check for safepoint
- advance to final state

and you didn't have to think too much about what the initial state was.

But I can see that logically any unsafe state works.

>> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 207:
>> 
>>> 205:   }
>>> 206:   ~ThreadInVMfromNative() {
>>> 207:     assert(_thread->thread_state() == _thread_in_vm, "coming from wrong thread state");
>> 
>> Why did you drop this assert? It guards against unexpected/unbalanced intermediate state changes.
>
> The assert made sense before but now we are just using transition_from_vm() which will already assert the JT is in vm.

Ah! I  missed that.

>> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 223:
>> 
>>> 221:   ThreadToNativeFromVM(JavaThread *thread) : ThreadStateTransition(thread) {
>>> 222:     // We are leaving the VM at this point and going directly to native code.
>>> 223:     // Block, if we are in the middle of a safepoint synchronization.
>> 
>> Is this comment no longer correct?
>
> So the sentence "We are leaving the VM at this point and going directly to native code." is correct but I'm not sure what's the point of adding it. Otherwise we would have to add it for all transitions. As for "Block, if we are in the middle of a safepoint synchronization." it was true before when we used transition(), but now we just go directly to native since the safepoint will be able to progress without the need to block (equivalent to what we do in ~ThreadInVMfromNative).

I hadn't realized we had such asymmetry with the two different VM->native transitions. Though the circumstances of the two are somewhat different.

But I can't conjure up any bad scenarios here given any safepoint/handshake is racing with this transition anyway.

>> src/hotspot/share/runtime/thread.cpp line 1647:
>> 
>>> 1645:       ThreadInVMfromJava tiv(this);
>>> 1646:       JavaThread* THREAD = this;
>>> 1647:       Exceptions::throw_unsafe_access_internal_error(THREAD, __FILE__, __LINE__, "a fault occurred in a recent unsafe memory access operation in compiled Java code");
>> 
>> This wording is somewhat deliberate as the unsafe access if not detected synchronously with the actual memory operation - hence it is a "recent unsafe memory access operation".
>
> I see, maybe I should instead change the error message for the _thread_in_vm case to be "a fault occurred in a recent unsafe memory access operation"?

IIRC it was only the _thread_in_Java case that was special because that was where JIT'd code could have a series of unsafe memory accesses, and the exception won't be thrown until we hit a specific async exception check point.

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

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