RFR: 8272526: Cleanup ThreadStateTransition class [v2]

[David Holmes]

On 21/08/2021 8:05 am, Coleen Phillimore wrote:
> On Tue, 17 Aug 2021 16:50:50 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
>>
>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>>
>>    restore assert in transition_from_native()
> 
> So this simplifies things a lot and consolidates the code when you transition from the _thread_in_vm. Essentially the real states one has to think about are _thread_in_native, _thread_in_vm and _thread_in_Java, so there's a function for each.  Really nice cleanup!
> 
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 87:
> 
>> 85:     thread->set_thread_state_fence(_thread_in_vm);
>> 86:     SafepointMechanism::process_if_requested_with_exit_check(thread, check_asyncs);
>> 87:     thread->set_thread_state(to);
> 
> I always wonder why all the set_thread_states don't have a fence.  Is it really that bad for performance to not have this unconditional, to avoid any transitions potentially needing but missing the fence?

We do not even have unconditional release semantics on set_thread_state 
because it was deemed too much of a performance risk on x86. I'm not so 
sure that is really true, but a fence is a lot more heavyweight than 
release semantics. See related discussion in

https://bugs.openjdk.java.net/browse/JDK-8267585

That aside I really object to adding ordering constraints "just in case 
we needed it but we missed it". Understanding races and ordering 
constraints is hard enough when you can see the participating code, and 
nigh impossible when there are unnecessary constraints applied. Anywhere 
there is a full fence we should have a comment describing exactly why a 
full fence is needed at that point IMO.

Cheers,
David
-----

> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 131:
> 
>> 129:    void trans(JavaThreadState from, JavaThreadState to)  { transition(_thread, from, to); }
>> 130:    void trans_from_java(JavaThreadState to)              { transition_from_java(_thread, to); }
>> 131:    void trans_from_native(JavaThreadState to)            { transition_from_native(_thread, to); }
> 
> I'm so happy to see these go away.
> 
> -------------
> 
> Marked as reviewed by coleenp (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/5128
>