RFR: 8272526: Cleanup ThreadStateTransition class

[David Holmes]

On Mon, 16 Aug 2021 19:04:09 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Hi all,
> 
> Please review the following cleanup of class ThreadStateTransition. I added method transition_from_vm and removed the generic transition method which was covering those transitions. This not only makes the public API more consistent given that we already have transition_from_java and transition_from_native but it also allows for some common code refactoring and it also makes more clear which transitions we exactly need to deal with and what actions need to be executed.
> 
> I adjusted the expected error message in test InternalErrorTest.java to match the _thread_in_vm case in check_and_handle_async_exceptions() because with this patch we transition to Java only after having checked the exit conditions, rather than today where we process safepoint/handshakes, set state to _thread_in_Java and then process the exit conditions. Ideally I would remove the _thread_in_Java case from the switch statement but we still have the handle_polling_page_exception() case which does everything in Java and never transitions (I will address that in another RFE), so I only adjusted the error message there in case we hit that path in this test.
> 
> Tested in mach5 tiers 1-7.
> 
> Thanks,
> Patricio

Hi Patricio,

Generally this looks good but I have a few comments/queries.

Thanks,
David

src/hotspot/share/prims/universalUpcallHandler.cpp line 91:

> 89:   // After this, we are officially in Java Code. This needs to be done before we change any of the thread local
> 90:   // info, since we cannot find oops before the new information is set up completely.
> 91:   ThreadStateTransition::transition_from_native(thread, _thread_in_Java, true /* check_asyncs */);

This is much cleaner!

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.

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?

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.

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?

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

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

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