-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8329748: Change default value of AssertWXAtThreadSync to true #19102
Conversation
👋 Welcome back tholenstein! A progress list of the required criteria for merging this PR into |
@tobiasholenstein This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. 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 128 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. ➡️ To integrate this PR with the above commit message to the |
@tobiasholenstein The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good.
@@ -74,6 +74,7 @@ void* JfrIntrinsicSupport::write_checkpoint(JavaThread* jt) { | |||
|
|||
void* JfrIntrinsicSupport::return_lease(JavaThread* jt) { | |||
DEBUG_ONLY(assert_precondition(jt);) | |||
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this could be moved down. It doesn't seem to be needed for the Java --> native transition. Is it needed for the JfrJavaEventWriter::flush() call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is only needed for the native --> Java transition below, why don't we do it lazily? The interpreter and compilers already do this by calling check_special_condition_for_native_trans() only if a safepoint is detected.
Normally we would want to be in the WXExec state when executing in _thread_in_native.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WXWrite
is needed for
JfrIntrinsicSupport::return_lease ->
ThreadStateTransition::transition_from_native ->
SafepointMechanism::process_if_requested_with_exit_check ->
SafepointMechanism::process_if_requested ->
JavaThread::check_possible_safepoint ->
assert_wx_state(WXWrite)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally we would want to be in the WXExec state when executing in _thread_in_native.
I agree. So we would need to aquireWXWrite
twice just forThreadStateTransition::transition_from_java
and again forThreadStateTransition::transition_from_native
. I think its a bit unfortune thatWXWrite
is needed for the state transition..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need WXWrite for transition_from_java. I don't know how useful AssertWXAtThreadSync is if it forces us to make unnecessary transitions. It seems to go in the opposite direction from the more lazy approaches discussed in JDK-8307817.
@@ -74,6 +74,7 @@ void* JfrIntrinsicSupport::write_checkpoint(JavaThread* jt) { | |||
|
|||
void* JfrIntrinsicSupport::return_lease(JavaThread* jt) { | |||
DEBUG_ONLY(assert_precondition(jt);) | |||
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, jt)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is only needed for the native --> Java transition below, why don't we do it lazily? The interpreter and compilers already do this by calling check_special_condition_for_native_trans() only if a safepoint is detected.
Normally we would want to be in the WXExec state when executing in _thread_in_native.
FWIW, I decided to look into WXExec as default (JDK-8328306), and in my draft so far I have removedAssertWXAtThreadSync completely, and I suspect that a successful implementation of exec-by-default will make JDK-8307817 no longer needed as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me.
Thanks, Richard.
Thanks for looking at JDK-8328306. Sounds like an interesting approach that could simplify things with WXExec/WXWrite. |
Sure, fine with me! |
Thanks for the reviews @vnkozlov , @reinrich and @dean-long /integrate |
Going to push as commit 3d511ff.
Your commit was automatically rebased without conflicts. |
@tobiasholenstein Pushed as commit 3d511ff. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
The debug flag
-XX:+AssertWXAtThreadSync
conservatively checks for correct W^X thread state at possible safepoints or handshake. The flag is useful to detect missingMACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, thread));
. Since the check is cheap and it is aAARCH64_ONLY(develop(..))
only flag it makes sense to enable the flag by default.There was one missing
MACOS_AARCH64_ONLY(ThreadWXEnable __wx(WXWrite, thread));
to make all tests (tier1-7) pass.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19102/head:pull/19102
$ git checkout pull/19102
Update a local copy of the PR:
$ git checkout pull/19102
$ git pull https://git.openjdk.org/jdk.git pull/19102/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19102
View PR using the GUI difftool:
$ git pr show -t 19102
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19102.diff
Webrev
Link to Webrev Comment