-
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
8309953: Strengthen and optimize oopDesc age methods #14456
Conversation
👋 Welcome back shade! A progress list of the required criteria for merging this PR into |
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.
LGTM.
@shipilev 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 22 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 |
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 as a minor performance optimization. I wouldn't claim that this actually fixes a race: this code is only safe at a safepoint to begin with (might even want to assert). If this were called outside of a safepoint, a competing thread might unlock a stack-lock, and reaching to the displaced header would blow up.
BTW, GC code is full of idioms where we repeatedly load the mark-word, even though a single load would be better. For example if (o->is_forwarded()) { ... o->forwardee() ... } |
I think this issue is overstated as the code is not intended to be thread-safe in the way suggested. So it is just a micro-optimisation, the value of which has not been shown, and which makes the source code somewhat clunky IMO. |
Moving perf observations in a separate comment. Sample generated code for
Note how the method suffix also folds, which saves about 24 bytes in instruction stream. On Linux x86_64, it gives as an edge, for a benchmark that stresses a Serial Full GC:
So, about +1..2% faster Full GC, for a little change :) |
The issue is not whether we can construct a benchmark that demonstrates a gain with this kind of micro-optimisation, but whether the micro-optimisation provides sufficient gain in general to trade-off against the reduction in code readability. I'm dubious about the value but will leave it to the GC folk to make the call. Thanks for the updated info. /label add hotspot-gc |
@dholmes-ora |
Seems like we are good, as I see approvals from 3 GC engineers :) /integrate |
Going to push as commit 4a5475c.
Your commit was automatically rebased without conflicts. |
See the RFE for discussion. Basically, there is little reason to do two loads of mark word, when we can do one.
Additional testing:
tier1 tier2 tier3
tier1 tier2 tier3
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14456/head:pull/14456
$ git checkout pull/14456
Update a local copy of the PR:
$ git checkout pull/14456
$ git pull https://git.openjdk.org/jdk.git pull/14456/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14456
View PR using the GUI difftool:
$ git pr show -t 14456
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14456.diff
Webrev
Link to Webrev Comment