RFR: 8272526: Cleanup ThreadStateTransition class [v2]

[Patricio Chilano Mateo]

On Tue, 17 Aug 2021 07:00:48 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   restore assert in transition_from_native()
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 116:
> 
>> 114:     assert((to & 1) == 0, "odd numbers are transitions states");
>> 115:     assert(thread->thread_state() == _thread_in_native, "coming from wrong thread state");
>> 116:     assert(!thread->has_last_Java_frame() || thread->frame_anchor()->walkable(), "Unwalkable stack in native->vm transition");
> 
> We seem to have lost this assertion.

Unintended, restored.

> 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.

> 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.

> 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).

> 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"?

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

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