-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8372285: G1: Micro-optimize x86 barrier code #28446
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
|
Sample experiments show this saves ~1.6% of code: |
|
👋 Welcome back shade! A progress list of the required criteria for merging this PR into |
|
@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 2 new commits 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 |
Webrevs
|
tschatzl
left a comment
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, thanks.
I assume that the jmh writebarrier micros were run just in case. Fwiw, also the GHA failures earlier looked like infra issues.
|
GHA failure is due to #28445. |
As expected, I see no real impact on EPYC machine, as we realistically only touch gc-active and/or slow-paths: |
|
/cc hotspot-gc |
|
@albertnetymk |
vnkozlov
left a comment
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.
Comments.
| thread, pre_val, tmp); | ||
| __ jmp(done); | ||
| __ testptr(pre_val, pre_val); | ||
| __ jccb(Assembler::equal, L_null); |
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 know that this short jump will be fused to one instruction with testptr on modern x86. But you will have jump-to-jump sequence. So you may win size wise but "throughput" could be worser. Especially if it is "fast" path.
Can you check performance of these changes vs using jcc(Assembler::equal, L_done); here.
|
|
||
| void G1BarrierSetAssembler::gen_write_ref_array_post_barrier(MacroAssembler* masm, DecoratorSet decorators, | ||
| Register addr, Register count, Register tmp) { | ||
| Label done; |
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.
Since you are touching this code can you add L_ to labels in this code?
This is our usual practice for labels to clear see them.
|
|
||
| Register thread = r15_thread; | ||
|
|
||
| Label done; |
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.
Please use L_done.
We know from JDK-8372284 that G1 C2 stubs can take ~10% of total instructions. So minor optimizations in hand-written assembly pay off for code density. This PR does a little x86-specific polishing:
testptrwhere possible, short forward branches where possible. I rewired some code to make it abundantly clear the branches in question are short. It also makes clear that lots of the affected methods are essentially fall-through.The patch is deliberately on simpler side, so we can backport it to 25u, if need arises.
Additional testing:
tier1allProgress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28446/head:pull/28446$ git checkout pull/28446Update a local copy of the PR:
$ git checkout pull/28446$ git pull https://git.openjdk.org/jdk.git pull/28446/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28446View PR using the GUI difftool:
$ git pr show -t 28446Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28446.diff
Using Webrev
Link to Webrev Comment