-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8268052: [JVMCI] non-default installed code must be marked as in_use #4283
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
Conversation
👋 Welcome back never! A progress list of the required criteria for merging this PR into |
@@ -1784,6 +1785,8 @@ JVMCI::CodeInstallResult JVMCIRuntime::register_method(JVMCIEnv* JVMCIENV, | |||
MutexLocker ml(CompiledMethod_lock, Mutex::_no_safepoint_check_flag); | |||
if (nm->make_in_use()) { | |||
method->set_code(method, nm); | |||
} else { | |||
result = JVMCI::nmethod_reclaimed; |
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.
Is it really possible for the make_in_use call to fail if the nmethod is nmethodLocker'ed? The copy of this logic in ciEnv.cpp doesn't treat a failure of this call to be a compilation failure but it certainly seems like it should. It would be better if it weren't possible for this call to fail.
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.
The issue is that the nmethod is published to Method code pointers before it is made in_use. That means a concurrent JavaThread may enter it and make the nmethod not_entrant, before it is made in_use. Making it in_use after it became not_entrant would cause a non-monotonic state transition which isn't great and comes with problems.
I agree it feels like there is room for improvement here, but at least that is the reason why make_in_use can fail; to prevent non-monotonic state transitions in this race.
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.
Ok. I see the intent. In that case it seems like the reordering of the in_use and set_code calls was the important part of JDK-8226705. It can't actually fail here since it hasn't been published until the following line, right? Or is there some other way the nmethod is published? Why couldn't the in_use state be set at the end of the nmethod factory method?
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 think it's published in the code cache and we had trouble with code walkers using only the CodeCache_lock to shoot down nmethods, but not the Compile_lock. If the walkers used the Compile_lock too then they wouldn't pick these up until they are fully installed. Plenty of room for improvements...
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.
Yes, directly scanning the code cache seemed like the only other path. It's unfortunate that the nmethod lifecycle seems to have gotten more complicated instead of less so. So it could really fail in some exceptionally weird circumstances.
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.
Yes, directly scanning the code cache seemed like the only other path. It's unfortunate that the nmethod lifecycle seems to have gotten more complicated instead of less so. So it could really fail in some exceptionally weird circumstances.
Yes indeed. This is what motivated my work on https://openjdk.java.net/jeps/8221828
In my prototype, after removing inline caches, replacing it with an optimized table driven approach to method invocations, I could reduce the nmethod lifecycle down to a boolean: is_in_use(), which is true or false. No zombies, no unloaded, no unloading, no is_alive, no transient installation state, no separate "nmethod locking mechanism". I hope it hits mainline one day, because it reduces so much risk in our code base, by removing a huge amount of state machinery.
@tkrodriguez 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. |
Could I get some review for this? |
LGTM |
@tkrodriguez 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 138 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 |
@dougxc You need to go to 'File changed' tab and press 'Review changes' and 'Approve' for your review to be counted. |
/integrate |
@tkrodriguez Since your change was applied there have been 174 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit db45ff0. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4283/head:pull/4283
$ git checkout pull/4283
Update a local copy of the PR:
$ git checkout pull/4283
$ git pull https://git.openjdk.java.net/jdk pull/4283/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4283
View PR using the GUI difftool:
$ git pr show -t 4283
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4283.diff