-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8365190: Remove LockingMode related code from share #27041
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
8365190: Remove LockingMode related code from share #27041
Conversation
👋 Welcome back fbredberg! A progress list of the required criteria for merging this PR into |
@fbredber 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 96 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 |
@fbredber 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
|
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.
A few comments and suggestions for your next RFE.
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.
Nothing obvious seems to be missing from the removal. And the changes look correct.
As @coleenp already mentioned there is even more code now that is effectively unused. Mostly to do with legacy + loom interactions. But I think it is fine to remove that in a follow up RFE.
Similarly there are some nomenclature that should be updated, but I know you have expressed wanting to do that in a follow up RFE as well.
I think it the main refactoring that are left are cleaning up the Synchronizer APIs, unifying some functions etc.
As for unifying LightweightSynchronizer with the ObjectSynchronizer, there might be an opportunity to let ObjectSynchronizer define the general API used by the rest of the VM to interact with the locking subsystem. And let LightweightSynchronizer contain all of the implementation. This could including moving the locking specific implementation details of relocking, deopting etc. behind an interface, decoupling them, and avoiding leaking implementation.
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.
svc part looks good.
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 good. Great cleanup! A couple of nits/suggestions.
Thanks
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 great!!
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.
Compiler changes look good, thanks!
Thank you all for the reviews. /integrate |
Going to push as commit a272696.
Your commit was automatically rebased without conflicts. |
Since the integration of JDK-8359437 the
LockingMode
flag can no longer be set by the user. After that, a number of PRs has been integrated which has removed allLockingMode
related code from all platforms (except from zero, which is done in this PR).This PR removes
LockingMode
related code from the shared (non-platform specific) files. It also removes theLockingMode
variable itself.Passes tier1-tier7 with no added problems.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27041/head:pull/27041
$ git checkout pull/27041
Update a local copy of the PR:
$ git checkout pull/27041
$ git pull https://git.openjdk.org/jdk.git pull/27041/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27041
View PR using the GUI difftool:
$ git pr show -t 27041
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27041.diff
Using Webrev
Link to Webrev Comment