-
Notifications
You must be signed in to change notification settings - Fork 112
8359064: Expose reason for marking nmethod non-entrant to JVMCI client #335
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
base: master
Are you sure you want to change the base?
8359064: Expose reason for marking nmethod non-entrant to JVMCI client #335
Conversation
|
👋 Welcome back cslucas! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
This backport pull request has now been updated with issue from the original commit. |
Reviewed-by: aph, shade (cherry picked from commit ff54a6493a63cfbcaab7ec90c7db0135e98a7f0c)
|
/issue add JDK-8359064 |
|
@JohnTortugo This issue is referenced in the PR title - it will now be updated. |
|
/issue add JDK-8360049 |
|
@JohnTortugo |
|
/approval request This PR updates JVMCI interface to receive/send to the external compiler the reason why a nmethod is being invalidated. Most of the touched files in the PR are just hit by collateral effect due to constants' name renaming, though. Note that a second commit/issue was added to this PR to address a bug on the original patch. These original changes have been integrated in JDK mainline about 4 months ago. Besides GHA the current patch was also tested on my internal CI. |
|
@JohnTortugo |
shipilev
left a comment
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 backport looks fine.
I do notice JDK-8360168 is still open, and there is JDK-8360169 problemlist addition. @RealFYang, are you OK with this going into 25u, or are you fixing the issue in mainline, or do you want to pick up problemlist patch as well?
Hi, Thanks for the ping. I think we need the problemlist patch for now. |
|
If everyone agree I can just problem list the test as part of this PR. |
I say let @JohnTortugo integrate this one, and then to a RISC-V-specific followup, if you need one. Or you just fix the mainline and backport a fix meanwhile. |
OK. I will try and propose a followup change as needed. |
|
The request for JDK-8359064 is still pending but JDK-8360049 was approved. Is there any concern regarding JDK-8359064? |
|
Note for @GoeLin, and a little status update: This one allows downstream Graal/Truffle to fix important performance potholes. Plus, it limits the divergence between mainline and JDK 25, which simplify the maintenance. (We have already picked it up in Corretto 25 for a few months.) So we would like to have it in 25u, pretty please :) |
|
Hi @JohnTortugo , I'll approve this, but can you please mention the two JBS bugids in your pull request comment? |
|
Hi @GoeLin , I updated the PR description with more details. Please, let me know if anything else is missing. |
Hi all, this pull request contains:
These changes updates JVMCI interface to receive/send to the external compiler the reason why a nmethod is being invalidated, which will let that compiler make better decisions about future recompilations of the method. Twelve of the files touched in this PR are just hit by collateral effect due to enum type/entries renaming. The remaining of the changes are in the JVMCI interface itself, which shouldn't affect HotSpot in anyway if not running with JVMCI enabled. A few changes are in tests.
Note that commit
ff54a64was added to this PR to address a bug on the original patch. These original changes have been integrated in JDK mainline over 5 months ago. Besides GHA the current patch was also tested on my internal CI.Thanks!
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk25u.git pull/335/head:pull/335$ git checkout pull/335Update a local copy of the PR:
$ git checkout pull/335$ git pull https://git.openjdk.org/jdk25u.git pull/335/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 335View PR using the GUI difftool:
$ git pr show -t 335Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk25u/pull/335.diff
Using Webrev
Link to Webrev Comment