-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8331341: secondary_super_cache does not scale well: C1 and interpreter #19989
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back aph! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
@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. |
|
Yes, I'm in favor of avoiding probing the table when Also, you can rearrange fast path checks: probe the home slot bit first, then check for |
BTW it makes sense to assert the invariant. Here's a patch (accompanied by a minor cleanup):
|
/label remove core-libs |
@AlanBateman |
src/hotspot/share/oops/klass.cpp
Outdated
@@ -260,7 +282,8 @@ Klass::Klass() : _kind(UnknownKlassKind) { | |||
// The constructor is also used from CppVtableCloner, | |||
// which doesn't zero out the memory before calling the constructor. | |||
Klass::Klass(KlassKind kind) : _kind(kind), | |||
_shared_class_path_index(-1) { | |||
_secondary_supers_bitmap(SECONDARY_SUPERS_BITMAP_EMPTY), |
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 like it is redundant since metaspace allocation already initializes memory with zeroes.
I promise that if you say you really want this change I will do it, but there is a cost I want to make clear. Adding the full-bitmap test at the start of the fast-path code increases the execution time in the case of In contrast, my proposed fix makes the invariant
So, what I'm suggesting is a bit smaller, a bit faster, and less work for me. On the other hand you say
I agree about the "It doesn't look right" part, but I'm not sure I agree about the cause of the bug. IMO, that was the failure to make the Your call. |
Oh, and I think it'll slow down the slow path a little bit too, but that's much less important. |
Thanks for the numbers, Andrew. I agree that the fix you propose is simple and conservative which makes it very appealing. First of all, does the bug have to be addressed separately? It affects 23, so we need to backport the fix anyway. Also, I took a closer look at the implementation. A couple of observations:
I thought about use cases for Let's do the fix you propose for now. We can reconsider the decision later if any interesting use cases show up. The downside is that there'll be more platform-specific code to touch, but that looks like a fair price to pay. |
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:-UseSecondarySupersTable
and-XX:+UseSecondarySupersCache
, aswell as 32- and 64-bit ports. There's some further complication in
that only
RCX
can 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_table
thatwork 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
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19989/head:pull/19989
$ git checkout pull/19989
Update a local copy of the PR:
$ git checkout pull/19989
$ git pull https://git.openjdk.org/jdk.git pull/19989/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19989
View PR using the GUI difftool:
$ git pr show -t 19989
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19989.diff
Webrev
Link to Webrev Comment