8331341: secondary_super_cache does not scale well: C1 and interpreter #19989

theRealAph · 2024-07-02T14:52:09Z

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, as
well 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 register
shuffling 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 that
work 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

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8331341: secondary_super_cache does not scale well: C1 and interpreter (Enhancement - P4)

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

bridgekeeper · 2024-07-02T14:52:57Z

👋 Welcome back aph! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-07-02T14:53:06Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-07-02T14:53:37Z

@theRealAph The following labels will be automatically applied to this pull request:

core-libs
hotspot

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.

…nto JDK-8331658-work

openjdk · 2024-07-04T14:08:36Z

⚠️ @theRealAph This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

iwanowww · 2024-07-26T21:58:39Z

Yes, I'm in favor of avoiding probing the table when SECONDARY_SUPERS_BITMAP_FULL == bitmap. It doesn't look right when the code treats secondary_supers as a table irrespective of whether it was hashed or not. IMO it unnecessarily complicates things and may continue to be a source of bugs.

Also, you can rearrange fast path checks: probe the home slot bit first, then check for SECONDARY_SUPERS_BITMAP_FULL != bitmap before accessing secondary_supers. It won't help with inlined code size increase, but negative lookups will stay mostly unaffected by the additional check.

iwanowww · 2024-07-27T00:00:25Z

BTW it makes sense to assert the invariant. Here's a patch (accompanied by a minor cleanup):

diff --git a/src/hotspot/share/oops/instanceKlass.cpp b/src/hotspot/share/oops/instanceKlass.cpp
index b050784dfc5..5413e29defb 100644
--- a/src/hotspot/share/oops/instanceKlass.cpp
+++ b/src/hotspot/share/oops/instanceKlass.cpp
@@ -647,7 +647,7 @@ void InstanceKlass::deallocate_contents(ClassLoaderData* loader_data) {
       !secondary_supers()->is_shared()) {
     MetadataFactory::free_array<Klass*>(loader_data, secondary_supers());
   }
-  set_secondary_supers(nullptr);
+  set_secondary_supers(nullptr, SECONDARY_SUPERS_BITMAP_EMPTY);
 
   deallocate_interfaces(loader_data, super(), local_interfaces(), transitive_interfaces());
   set_transitive_interfaces(nullptr);
diff --git a/src/hotspot/share/oops/klass.cpp b/src/hotspot/share/oops/klass.cpp
index 26ec25d1c80..b1012810be4 100644
--- a/src/hotspot/share/oops/klass.cpp
+++ b/src/hotspot/share/oops/klass.cpp
@@ -319,16 +319,16 @@ bool Klass::can_be_primary_super_slow() const {
     return true;
 }
 
-void Klass::set_secondary_supers(Array<Klass*>* secondaries) {
-  assert(!UseSecondarySupersTable || secondaries == nullptr, "");
-  set_secondary_supers(secondaries, SECONDARY_SUPERS_BITMAP_EMPTY);
-}
-
 void Klass::set_secondary_supers(Array<Klass*>* secondaries, uintx bitmap) {
 #ifdef ASSERT
   if (secondaries != nullptr) {
     uintx real_bitmap = compute_secondary_supers_bitmap(secondaries);
     assert(bitmap == real_bitmap, "must be");
+    if (bitmap != SECONDARY_SUPERS_BITMAP_FULL) {
+      assert(((uint)secondaries->length() == population_count(bitmap)), "required");
+    }
+  } else {
+    assert(bitmap == SECONDARY_SUPERS_BITMAP_EMPTY, "");
   }
 #endif
   _secondary_supers_bitmap = bitmap;
diff --git a/src/hotspot/share/oops/klass.hpp b/src/hotspot/share/oops/klass.hpp
index 2f733e11eef..a9e73e7bcbd 100644
--- a/src/hotspot/share/oops/klass.hpp
+++ b/src/hotspot/share/oops/klass.hpp
@@ -236,7 +236,6 @@ class Klass : public Metadata {
   void set_secondary_super_cache(Klass* k) { _secondary_super_cache = k; }
 
   Array<Klass*>* secondary_supers() const { return _secondary_supers; }
-  void set_secondary_supers(Array<Klass*>* k);
   void set_secondary_supers(Array<Klass*>* k, uintx bitmap);
 
   uint8_t hash_slot() const { return _hash_slot; }

AlanBateman · 2024-07-27T06:07:43Z

/label remove core-libs

openjdk · 2024-07-27T06:09:38Z

@AlanBateman
The core-libs label was successfully removed.

iwanowww · 2024-07-27T07:09:51Z

src/hotspot/share/oops/klass.cpp

@@ -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),


Looks like it is redundant since metaspace allocation already initializes memory with zeroes.

theRealAph · 2024-07-29T13:07:48Z

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 SecondarySupersLookup.testPositive03 from 5 cycles/op to 5.5 cycles/op on average. It also adds at least 5 bytes (8 bytes for AArch64) to the inline code size, depending on how you do it.

In contrast, my proposed fix makes the invariant pocount(bitmap) >= secondary_supers.length truly invariant, and changes the full-bitmap test at the start of the slow path thusly to a void a performance regression with a nearly-full bitmap:

--- a/src/hotspot/cpu/x86/macroAssembler_x86.cpp
+++ b/src/hotspot/cpu/x86/macroAssembler_x86.cpp
@@ -5212,8 +5212,8 @@ void MacroAssembler::lookup_secondary_supers_table_slow_path(Register r_super_kl
   // The bitmap is full to bursting.
   // Implicit invariant: BITMAP_FULL implies (length > 0)
   assert(Klass::SECONDARY_SUPERS_BITMAP_FULL == ~uintx(0), "");
-  cmpq(r_bitmap, (int32_t)-1); // sign-extends immediate to 64-bit value
-  jcc(Assembler::equal, L_huge);
+  cmpq(r_array_length, (int32_t)SECONDARY_SUPERS_TABLE_SIZE - 2);
+  jcc(Assembler::greater, L_huge);
 
@@ -344,11 +370,12 @@ uintx Klass::hash_secondary_supers(Array<Klass*>* secondaries, bool rewrite) {
     return uintx(1) << hash_slot;
   }
 
--- a/src/hotspot/share/oops/klass.cpp
+++ b/src/hotspot/share/oops/klass.cpp
@@ -344,11 +370,12 @@ uintx Klass::hash_secondary_supers(Array<Klass*>* secondaries, bool rewrite) {
     return uintx(1) << hash_slot;
   }
 
-  // For performance reasons we don't use a hashed table unless there
-  // are at least two empty slots in it. If there were only one empty
-  // slot it'd take a long time to create the table and the resulting
-  // search would be no faster than linear probing.
-  if (length > SECONDARY_SUPERS_TABLE_SIZE - 2) {
+  // Invariant: _secondary_supers.length >= population_count(_secondary_supers_bitmap)
+
+  // Don't attempt to hash a table that's completely full, because in
+  // the case of an absent interface linear probing would not
+  // terminate.
+  if (length >= SECONDARY_SUPERS_TABLE_SIZE) {
     return SECONDARY_SUPERS_BITMAP_FULL;
   }

So, what I'm suggesting is a bit smaller, a bit faster, and less work for me. On the other hand you say

It doesn't look right when the code treats secondary_supers as a table irrespective of whether it was hashed or not. IMO > it unnecessarily complicates things and may continue to be a source of bugs.

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 pocount(bitmap) >= secondary_supers.length truly invariant.

Your call.

theRealAph · 2024-07-29T13:46:15Z

Adding the full-bitmap test at the start of the fast-path code increases the execution time...

Oh, and I think it'll slow down the slow path a little bit too, but that's much less important.

iwanowww · 2024-07-29T23:09:42Z

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:

as of now, there are 4 platforms which support secondary supers table lookup (so, all of them have to be adjusted if any platform-specific changes are needed);
there are existing implicit assumptions on SECONDARY_SUPERS_BITMAP_FULL (e.g., secondary_supsers_bitmap == SECONDARY_SUPERS_BITMAP_FULL => secondary_supsers->length() > 0.

I thought about use cases for SECONDARY_SUPERS_BITMAP_FULL as a kill switch for table lookups, but don't see anything important (once secondary supers are hashed unconditionally).

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.

…into JDK-8331658-work

Andrew Haley added 18 commits June 4, 2024 16:39

Stage

b814460

Temp

97dec4c

Super

491177c

Merge branch 'clean' into JDK-8331658-work

3db4a14

Inter

4b3aae9

Inter

9232639

Temp

02a4e3c

Ready for x86

db07cbd

Ready for x86

3288967

First stab at x86

4fd1215

Converging

345f22a

UseSecondarySupersTable on by default (temp)

6b286a3

Temp

a027a57

Cleanup

f095fee

Cleanup

de17b44

Merge branch 'clean' into JDK-8331658-work

84a978c

Temp

73ff8fb

Temp

80d33b9

theRealAph marked this pull request as draft July 2, 2024 14:52

openjdk bot added hotspot [email protected] core-libs [email protected] labels Jul 2, 2024

Andrew Haley added 4 commits July 2, 2024 17:22

Temp

e38e086

Fix x86

16f60f9

Fix x86-32

d2771bc

Merge remote-tracking branch 'refs/remotes/origin/JDK-8331658-work' i…

bc5afd0

…nto JDK-8331658-work

Temp

cec26c7

openjdk bot removed the core-libs [email protected] label Jul 27, 2024

iwanowww reviewed Jul 27, 2024

View reviewed changes

Andrew Haley added 3 commits July 29, 2024 11:32

Minor

329f487

Experiment: test bitmap upfront.

193478d

Experiment: test bitmap upfront.

0571877

Andrew Haley added 5 commits July 31, 2024 07:27

Review comments

5cca1cc

Fix AArch64

c33e1bd

Review comments

4c9a081

Merge branch 'JDK-8331658-work' of https://github.com/theRealAph/jdk …

2769d9e

…into JDK-8331658-work

Reorganize x86

24aca9a

openjdk bot removed the rfr Pull request is ready for review label Aug 1, 2024

Untabify

e41f15a

openjdk bot added the rfr Pull request is ready for review label Aug 1, 2024

Andrew Haley added 10 commits August 1, 2024 06:09

use assert rather than guarantee

4c7fad7

Fix shared code

7792ca8

Fix shared code

eb73993

Cleanup

51c68a0

Merge from JDK head.

5b46b38

Merge branch 'clean' into JDK-8331658-work

790939c

Fix merge

77de087

Merge branch 'clean' into JDK-8331658-work

dd42fe9

Use post-incrememnt RegSet operator.

fe754fb

Fix s390

a726b62

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8331341: secondary_super_cache does not scale well: C1 and interpreter #19989

8331341: secondary_super_cache does not scale well: C1 and interpreter #19989

theRealAph commented Jul 2, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

openjdk bot commented Jul 4, 2024

iwanowww commented Jul 26, 2024

iwanowww commented Jul 27, 2024

AlanBateman commented Jul 27, 2024

openjdk bot commented Jul 27, 2024

iwanowww Jul 27, 2024 •

edited

Loading

theRealAph commented Jul 29, 2024

theRealAph commented Jul 29, 2024

iwanowww commented Jul 29, 2024

8331341: secondary_super_cache does not scale well: C1 and interpreter #19989

Are you sure you want to change the base?

8331341: secondary_super_cache does not scale well: C1 and interpreter #19989

Conversation

theRealAph commented Jul 2, 2024 • edited by openjdk bot Loading

HotSpot shared runtime

x86

AArch64

Generally

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

openjdk bot commented Jul 4, 2024

iwanowww commented Jul 26, 2024

iwanowww commented Jul 27, 2024

AlanBateman commented Jul 27, 2024

openjdk bot commented Jul 27, 2024

iwanowww Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

theRealAph commented Jul 29, 2024

theRealAph commented Jul 29, 2024

iwanowww commented Jul 29, 2024

theRealAph commented Jul 2, 2024 •

edited by openjdk bot

Loading

iwanowww Jul 27, 2024 •

edited

Loading