8331341: secondary_super_cache does not scale well: C1 and interpreter #19989
Conversation
|
👋 Welcome back aph! A progress list of the required criteria for merging this PR into |
|
@theRealAph 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 306 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 |
|
@theRealAph 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. |
openjdk
bot
added
hotspot
|
|
|
By the way, to see the performance benefit of this patch run the test case SecondarySuperCacheInterContention.java, like so:
|
|
|
||
| // This next instruction is equivalent to: | ||
| // mov(tmp_reg, (u1)(Klass::SECONDARY_SUPERS_TABLE_SIZE - 1)); | ||
| // sub(temp2, r_array_index, tmp_reg); |
There was a problem hiding this comment.
One minor question about the code comment. Should this line be: // sub(temp2, tmp_reg, slot);?
There was a problem hiding this comment.
Yes, it is. Good spot.
|
@theRealAph Hi, And here is the version for riscv: zifeihan@2c52968 . Do you want to have it in this PR? Or shall I create another one? Thanks. By the way, Already passed tier1-tier3 tests on the linux-riscv64 platform(SOPHON SG2042). |
There's no need to merge with this PR. It should be a simple enough thing to approve. |
|
/integrate |
|
@theRealAph This pull request has not yet been marked as ready for integration. |
|
/integrate |
|
Going to push as commit ead0116.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@theRealAph Pushed as commit ead0116. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This patch expands the use of a hash table for secondary superclasses
to the interpreter, C1, and runtime. It also adds a C2 implementation
of hashed lookup in cases where the superclass isn't known at compile
time.
HotSpot shared runtime
Building hashed secondary tables is now unconditional. It takes very
little time, and now that the shared runtime always has the tables, it
might as well take advantage of them. The shared code is easier to
follow now, I think.
There might be a performance issue with x86-64 in that we build
HotSpot for a default x86-64 target that does not support popcount.
This means that HotSpot C++ runtime on x86 always uses a software
emulation for popcount, even though the vast majority of machines made
for the past 20 years can do popcount in a single instruction. It
wouldn't be terribly hard to do something about that.
Having said that, the software popcount is really not bad.
x86
x86 is rather tricky, because we still support
-XX:-UseSecondarySupersTableand-XX:+UseSecondarySupersCache, aswell as 32- and 64-bit ports. There's some further complication in
that only
RCXcan be used as a shift count, so there's some registershuffling to do. All of this makes the logic in macroAssembler_x86.cpp
rather gnarly, with multiple levels of conditionals at compile time
and runtime.
AArch64
AArch64 is considerably more straightforward. We always have a
popcount instruction and (thankfully) no 32-bit code to worry about.
Generally
I would dearly love simply to rip out the "old" secondary supers cache
support, but I've left it in just in case someone has a performance
regression.
The versions of
MacroAssembler::lookup_secondary_supers_tablethatwork with variable superclasses don't take a fixed set of temp
registers, and neither do they call out to to a slow path subroutine.
Instead, the slow patch is expanded inline.
I don't think this is necessarily bad. Apart from the very rare cases
where C2 can't determine the superclass to search for at compile time,
this code is only used for generating stubs, and it seemed to me
ridiculous to have stubs calling other stubs.
I've followed the guidance from @iwanowww not to obsess too much about
the performance of C1-compiled secondary supers lookups, and to prefer
simplicity over absolute performance. Nonetheless, this is a
complicated patch that touches many areas.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19989/head:pull/19989$ git checkout pull/19989Update a local copy of the PR:
$ git checkout pull/19989$ git pull https://git.openjdk.org/jdk.git pull/19989/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19989View PR using the GUI difftool:
$ git pr show -t 19989Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19989.diff
Using Webrev
Link to Webrev Comment