-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8266497: Remove unnecessary EMCP liveness indication #3852
Conversation
👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into |
/label remove hotspot |
@coleenp |
@coleenp |
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.
@coleenp 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 14 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 |
Thanks Ioi, and thanks for updating the description. |
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.
Hi Coleen,
Seems reasonable. Nice simplification too!
Thanks,
David
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.
Hi Coleen,
This looks good to me in general.
I'm a little bit puzzled why the check emcp_method_count != 0
is needed at the line:
4005 if (emcp_method_count != 0 && obsolete_method_count != 0 &&
Suppose, the emcp_method_count is equal to 0.
Then do we really want to skip marking all the methods of this InstanceKlass as obsolete?
Most likely, I'm missing something.
Thanks,
Serguei
@sspitsyn I think you're right here. If all the newly old methods are obsolete, then we should mark all the methods in the previous versions as obsolete and not skip this. I'll file an issue to investigate this because it's always been this way (and could use a test also). It's too risky to change here. Thanks for the code review! |
Thanks for the code review, David. |
@coleenp Since your change was applied there have been 25 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit a05e8e2. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Thanks, Coleen! I agree, it is better to file a new bug. |
Marking running_emcp for Method* is unnecessary. We can set/clear breakpoints in all old emcp methods because they're not deallocated until none are running. See longer explanation in the CR.
Tested with tier1-6, tiers 7,8 are running and 98% passing.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3852/head:pull/3852
$ git checkout pull/3852
Update a local copy of the PR:
$ git checkout pull/3852
$ git pull https://git.openjdk.java.net/jdk pull/3852/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3852
View PR using the GUI difftool:
$ git pr show -t 3852
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3852.diff