-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8308479: [s390x] Implement alternative fast-locking scheme #14414
Conversation
👋 Welcome back amitkumar! A progress list of the required criteria for merging this PR into |
@offamitkumar 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. |
Webrevs
|
Hi @RealLucy, @TheRealMDoerr, @reinrich |
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.
There are some comments you may want to consider.
Hi @RealLucy, Please check benchmark result in description. |
Are the benchmark results stable or do they have a large variance? |
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.
Minor suggestions.
I ran
Patch was applied, but for in first case LockingMode was not selected, and in second case (with fast locking) But I will run these again with/without the code change. Thanks for insights. |
After struggling a lot with my environment, I finally was able to run some performance tests. There is the "db" subtest of JVM98 which caught my attention. It reproducibly shows
We need to gain deeper insight into the why before integration. |
Note that the new fast locking (LockingMode 2) doesn't support fast recursive locking. Could this be the problem? LockingMode 2 is fastest on Power10 for "db" benchmark, but that may be due to fewer memory barriers. Interesting finding! |
with fastlockbench:
Dacapo:
Renaissance-jmh (ms/op):
These are the new results I'm getting, seems consistent as well. |
@offamitkumar This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
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 my concerns had been addressed. The performance topic can be investigated later if there's not enough time right now. I think it's good enough for an experimental feature. What's your opinion, @RealLucy?
@offamitkumar 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 1043 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 |
We are considering making Fast Locking on by default for Oracle supported platforms. Have these performance concerns been addressed? |
The performance regression which is observed in some tests is still not fully understood. I will approve the PR despite of that. According to all out testing, it is functionally correct. |
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 functionally.
Performance regression should be further analyzed in a separate task.
Mailing list message from Kennke, Roman on hotspot-dev: FWIW, I found many of the renaissance benchmarks quite noisy. Might be worth watching them closely, and consider ramping up number of iterations *and* forks. (Also, I found using the jmh-wrapped version much easier to deal with, with that you can achieve the increased iterations and forks with simple -i -wi and -f options) Amazon Development Center Germany GmbH |
Lutz, Martin, Thanks for reviews and help. /integrate |
Going to push as commit 3fe6e0f.
Your commit was automatically rebased without conflicts. |
@offamitkumar Pushed as commit 3fe6e0f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This PR implements new fast-locking scheme for s390x. Additionally few parameters have been renamed to be in sync with PPC.
Testing done (for release, fastdebug and slowdebug build):
All
test/jdk/java/util/concurrent
test with parameters:Result is consistently similar to Aarch(MacOS) and PPC, All of 124 tests are passing except
MapLoops.java
because in the 2nd part for this testcase, jvm starts withHeavyMonitors
which conflict withLockingMode=2
BenchMark Result for Renaissance-jmh:
DaCapo Benchmark Result:
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14414/head:pull/14414
$ git checkout pull/14414
Update a local copy of the PR:
$ git checkout pull/14414
$ git pull https://git.openjdk.org/jdk.git pull/14414/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14414
View PR using the GUI difftool:
$ git pr show -t 14414
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14414.diff
Webrev
Link to Webrev Comment