-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8366461: Remove obsolete method handle invoke logic #27059
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?
Conversation
👋 Welcome back dlong! A progress list of the required criteria for merging this PR into |
@dean-long 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 1 new commit pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
@dean-long The following labels 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 lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
/label hotspot-compiler hotspot-runtime |
@dean-long The |
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.
Nice cleanup! Looks good.
Thanks @iwanowww ! |
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.
Thank you for cleaning this up, @dean-long. I just have a drive-by comment.
if (senderNm != null) { | ||
// If the sender PC is a deoptimization point, get the original PC. | ||
if (senderNm.isDeoptEntry(getPC()) || | ||
senderNm.isDeoptMhEntry(getPC())) { | ||
if (senderNm.isDeoptEntry(getPC())) { | ||
// DEBUG_ONLY(verifyDeoptriginalPc(senderNm, raw_unextendedSp)); | ||
} | ||
} |
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.
<arch>Frame.java adjustUnextendedSP()
do not seem to do anything? Perhaps these could be cleaned up 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.
Yes, it's tempting to want to clean these up, but I noticed that SA code really tries to mirror the C++ code, so I'm inclined to leave it. Is there a Serviceability expert that would like to see this code cleaned up further? @plummercj , what do you think?
I need one more review for this. |
At one time, JSR292 support needed special logic to save and restore SP across method handle instrinsic calls, but that is no longer the case. The only platform that still does the save/restore is arm32, which is no longer necessary. The save/restore can be removed along with related APIs and logic. Note that the arm32 port is largely based on the x86 port, which stopped doing the save/restore in jdk9 (JDK-8068945).
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27059/head:pull/27059
$ git checkout pull/27059
Update a local copy of the PR:
$ git checkout pull/27059
$ git pull https://git.openjdk.org/jdk.git pull/27059/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27059
View PR using the GUI difftool:
$ git pr show -t 27059
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27059.diff
Using Webrev
Link to Webrev Comment