8272526: Cleanup ThreadStateTransition class #5128
Conversation
|
|
openjdk
bot
added
serviceability
|
/label add hotspot-runtime |
|
/label remove hotspot |
|
@pchilano |
|
@pchilano |
|
/label remove serviceability |
|
@pchilano |
Webrevs
|
| clear_pending_exception = false; | ||
| } | ||
| } | ||
| ThreadStateTransition::transition_from_native(thread, _thread_in_Java, true /* check_asyncs */); |
| assert(thread->thread_state() == _thread_in_native, "coming from wrong thread state"); | ||
| assert(!thread->has_last_Java_frame() || thread->frame_anchor()->walkable(), "Unwalkable stack in native->vm transition"); |
There was a problem hiding this comment.
We seem to have lost this assertion.
There was a problem hiding this comment.
Unintended, restored.
| } | ||
| ~ThreadInVMfromNative() { | ||
| assert(_thread->thread_state() == _thread_in_vm, "coming from wrong thread state"); |
There was a problem hiding this comment.
Why did you drop this assert? It guards against unexpected/unbalanced intermediate state changes.
There was a problem hiding this comment.
The assert made sense before but now we are just using transition_from_vm() which will already assert the JT is in vm.
| // We are leaving the VM at this point and going directly to native code. | ||
| // Block, if we are in the middle of a safepoint synchronization. |
There was a problem hiding this comment.
Is this comment no longer correct?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| @@ -1644,7 +1646,7 @@ void JavaThread::check_and_handle_async_exceptions() { | |||
| case _thread_in_Java: { | |||
| ThreadInVMfromJava tiv(this); | |||
| JavaThread* THREAD = this; | |||
| Exceptions::throw_unsafe_access_internal_error(THREAD, __FILE__, __LINE__, "a fault occurred in a recent unsafe memory access operation in compiled Java code"); | |||
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So today we always reach the _async_unsafe_access_error case of check_and_handle_async_exceptions() with a state of _thread_in_Java, except when called from jni_check_async_exceptions() where we are in vm, so I don't know why the message refers exclusively to compiled code. I run test InternalErrorTest.java in baseline with just the interpreter("main/othervm -Xint -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI InternalErrorTest") and still passes. Here is the code that sets the pending unsafe access error when the thread receives SIGBUS while in Java: https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp#L249
There was a problem hiding this comment.
If the unsafe access error happens on Unsafe_CopySwapMemory0 like in the test then we will be either in vm or native (depending on src & dst) which will also output the same error message. We can differentiate between this cases if we want by saving the state in SharedRuntime::handle_unsafe_access().
There was a problem hiding this comment.
Sorry for the delay in getting back to this. There are a lot of subtleties around the throwing of an async exception related to "unsafe access errors" and I had to dig around to refresh my memory on a few things. End result: changing the error messages to say the same thing is fine.
There was a problem hiding this comment.
Thanks for the review David!
| assert(!thread->has_last_Java_frame() || thread->frame_anchor()->walkable(), "Unwalkable stack in native->vm transition"); | ||
|
|
||
| // Change to transition state and ensure it is seen by the VM thread. | ||
| thread->set_thread_state_fence(_thread_in_native_trans); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right, but whenever we are in safepoint/handshake code we are already in vm, that's why I actually think directly setting _thread_in_vm here makes more sense than having additional states just to make the poll. In fact using transitional states(along with the code in handle_polling_page_exception) forces us to need yet another transition wrapper for handshakes(ThreadInVMForHandshake) just to switch to vm and back.
Reading now the description of transition states in globalDefinitions.hpp I see they were more useful back then with the old safepoint logic:
"These extra states makes it possible for the safepoint code to handle certain thread_states without having to suspend the thread - making the safepoint code faster."
In SafepointSynchronize::block() there was a faster path for _thread_in_native_trans and _thread_blocked_trans (https://github.com/openjdk/jdk/blame/66aa45649ad36ed41c0241ded767d465248eda3c/src/hotspot/share/runtime/safepoint.cpp#L816).
There was a problem hiding this comment.
Yes things have changed a lot :)
|
Hi David, Thanks for looking at this. Patricio |
|
@pchilano This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 130 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
Great, thanks for the review and extra testing Robbin! Patricio |
There was a problem hiding this comment.
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!
|
|
||
| SafepointMechanism::process_if_requested(thread); | ||
| thread->set_thread_state_fence(_thread_in_vm); | ||
| SafepointMechanism::process_if_requested_with_exit_check(thread, check_asyncs); | ||
| thread->set_thread_state(to); |
There was a problem hiding this comment.
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?
| protected: | ||
| void trans(JavaThreadState from, JavaThreadState to) { transition(_thread, from, to); } | ||
| void trans_from_java(JavaThreadState to) { transition_from_java(_thread, to); } | ||
| void trans_from_native(JavaThreadState to) { transition_from_native(_thread, to); } |
There was a problem hiding this comment.
I'm so happy to see these go away.
Thanks for the review Coleen! Patricio |
|
Are you okay with v2 David? Thanks, |
| @@ -1644,7 +1646,7 @@ void JavaThread::check_and_handle_async_exceptions() { | |||
| case _thread_in_Java: { | |||
| ThreadInVMfromJava tiv(this); | |||
| JavaThread* THREAD = this; | |||
| Exceptions::throw_unsafe_access_internal_error(THREAD, __FILE__, __LINE__, "a fault occurred in a recent unsafe memory access operation in compiled Java code"); | |||
There was a problem hiding this comment.
Sorry for the delay in getting back to this. There are a lot of subtleties around the throwing of an async exception related to "unsafe access errors" and I had to dig around to refresh my memory on a few things. End result: changing the error messages to say the same thing is fine.
|
/integrate |
|
Going to push as commit 7454306.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
Mailing list message from David Holmes on hotspot-runtime-dev: On 21/08/2021 8:05 am, Coleen Phillimore wrote:
We do not even have unconditional release semantics on set_thread_state https://bugs.openjdk.java.net/browse/JDK-8267585 That aside I really object to adding ordering constraints "just in case Cheers, |
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
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5128/head:pull/5128$ git checkout pull/5128Update a local copy of the PR:
$ git checkout pull/5128$ git pull https://git.openjdk.java.net/jdk pull/5128/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5128View PR using the GUI difftool:
$ git pr show -t 5128Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5128.diff