jdk 8331341: secondary_super_cache does not scale well: C1 and interpreter

trafficstars

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)
[x] Change must not contain extraneous whitespace
[x] Commit message must refer to an issue

Issue

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

Reviewers

Vladimir Ivanov (@iwanowww - Reviewer) 🔄 Re-review required (review applies to cf71c835)
Vladimir Kozlov (@vnkozlov - Reviewer) 🔄 Re-review required (review applies to d5a29592)
Dean Long (@dean-long - Reviewer) 🔄 Re-review required (review applies to cf71c835)

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

Jul 02 '24 14:07 theRealAph

:wave: 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.

Jul 02 '24 14:07 bridgekeeper[bot]

@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:

8331341: secondary_super_cache does not scale well: C1 and interpreter

Reviewed-by: vlivanov, kvn, dlong

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 master branch:

714472d8a5b3d16b870bc272ce8664cd62733857: 8341798: Fix ExceptionOccurred in jdk.jdwp.agent
825ceb16b2e2347a4d9c1977d9a3a2da1296d5fe: 8341796: Fix ExceptionOccurred in jdk.hotspot.agent
8b4749713c63a08e502845ed5d0a0236822018cd: 8331682: Slow networks/Impatient clients can potentially send unencrypted TLSv1.3 alerts that won't parse on the server
0668e181c8234df3fab478748b88365510a7343f: 8343234: (bf) Move java/nio/Buffer/LimitDirectMemory.java from ProblemList.txt to ProblemList-Virtual.txt
774de278f77817e4494dc73bfee9257f145600fc: 8343497: Missing DEF_STATIC_JNI_OnLoad in libjimage and libsaproc native libraries
1cc3586c5328e7b792498707a08952e760b3511f: 8343551: Missing copyright header update in Charset-X-Coder.java.template
1c4483473d878bb4c41e66f27fea84ef81ede8c9: 8333582: Update CLDR to Version 46.0
23fa1a33274d279a53fa6dde683900450561957b: 8343484: Remove unnecessary @SuppressWarnings annotations (nio)
7f8450cc511e22e3183092edfa3a37b39f133cff: 8343473: Update copyright year of AddmodsOption.java
b74652b5f4424419b36888514730ac7550f42a6b: 8343167: Unnecessary define checks in InterpreterRuntime after JDK-8199809
... and 296 more: https://git.openjdk.org/jdk/compare/1581508988141bfb420d97759138203f30926b35...master

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 master branch, type /integrate in a new comment.

Jul 02 '24 14:07 openjdk[bot]

@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.

Jul 02 '24 14:07 openjdk[bot]

⚠️ @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).

Jul 04 '24 14:07 openjdk[bot]

Webrevs

Jul 05 '24 13:07 mlbridge[bot]

All done apart from the questions of:

Should Klass::linear_search_secondary_supers() const call set_secondary_super_cache()? (Strong no from me. It's UB.)
Should we use a straight linear search for secondary C++ supers in the runtime, i.e.not changing it for now?

Jul 22 '24 17:07 theRealAph

@theRealAph this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8331658-work
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Jul 22 '24 17:07 openjdk[bot]

FYI I did a merge with mainline and submitted the patch for testing.

Jul 23 '24 19:07 iwanowww

My take on the questions you raised.

Should Klass::linear_search_secondary_supers() const call set_secondary_super_cache()? (Strong no from me. It's UB.)

Agree. I'm fine with addressing that separately (as I mentioned earlier).

Should we use a straight linear search for secondary C++ supers in the runtime, i.e.not changing it for now?

Slightly in favor of keeping Klass::is_subtype_of() simple, but I'm fine with it either way.

Jul 23 '24 19:07 iwanowww

OK! I think that's everything.

Are we ready for a second pair of eyes now?

Jul 25 '24 16:07 theRealAph

Thanks! The patch looks good, except there was one failure observed during testing with the latest patch [1]. It does look related to the latest changes you did in 54050a5 about bitmap == SECONDARY_SUPERS_BITMAP_FULL check.

[1]

#  Internal Error (.../src/hotspot/share/oops/array.hpp:126), pid=1225147, tid=1273512
#  assert(i >= 0 && i< _length) failed: oob: 0 <= 63 < 63

V  [libjvm.so+0x7f7eab]  oopDesc::is_a(Klass*) const+0x21b  (array.hpp:126)
V  [libjvm.so+0xe9805e]  java_lang_Throwable::fill_in_stack_trace(Handle, methodHandle const&, JavaThread*)+0x158e  (javaClasses.cpp:2677)
V  [libjvm.so+0xe982cb]  java_lang_Throwable::fill_in_stack_trace(Handle, methodHandle const&)+0x6b  (javaClasses.cpp:2719)
V  [libjvm.so+0xfe045c]  JVM_FillInStackTrace+0x9c  (jvm.cpp:515)
....

RBX=0x00000000b446bcf0 is a pointer to class: 
javasoft.sqe.tests.lang.clss029.clss02902.e67 {0x00000000b446bcf0}
...
 - name:              'javasoft/sqe/tests/lang/clss029/clss02902/e67'
 - super:             'javasoft/sqe/tests/lang/clss029/clss02902/e66'
 - sub:               'javasoft/sqe/tests/lang/clss029/clss02902/e68'   
...
 - secondary supers: Array<T>(0x00007faff68b3058)
 - hash_slot:         39
 - secondary bitmap: 0xffffffffffffffff


R12=0x00000000b446ba80 is a pointer to class: 
javasoft.sqe.tests.lang.clss029.clss02902.e66 {0x00000000b446ba80}
...
 - name:              'javasoft/sqe/tests/lang/clss029/clss02902/e66'
 - super:             'javasoft/sqe/tests/lang/clss029/clss02902/e65'
 - sub:               'javasoft/sqe/tests/lang/clss029/clss02902/e67'   
...
 - secondary supers: Array<T>(0x00007faff68b2c90)
 - hash_slot:         63
 - secondary bitmap: 0xfffffffffffefffd

Jul 25 '24 23:07 iwanowww

Thanks! The patch looks good, except there was one failure observed during testing with the latest patch [1]. It does look related to the latest changes you did in 54050a5 about bitmap == SECONDARY_SUPERS_BITMAP_FULL check.

Wow! Whoever wrote that test case deserves a bouquet of roses.

It's a super-edge case. If the hash slot of an interface is 63, and the secondaries array length of the Klass we're probing is 63, then the initial probe is at Offset 63, one past the array bounds.

This bug happens because of an "optimization" created during the first round of reviews. If the secondaries array length is >= 62 (not >= 63), we set the secondaries bitmap to SECONDARY_SUPERS_BITMAP_FULL. So, the initial probe sees a full bitmap, popcount returns 63, and we look at secondary_supers[63]. Bang.

We never noticed this problem before because there's no bounds checking in the hand-coded assembly language implementations.

The root cause of this bug is that we're not maintaining the invariant popcount(bitmap) == secondary_supers()->length(). There are a couple of ways to fix this. We could check secondary_supers()->length() before doing any probe. I'm very reluctant to add a memory load to the super-hot path for this edge case, though. It's better to take some pain in the case of an almost-full secondaries array, because those are very rare, and will never occur in most Java programs. So, i've corrected the bitmap at the point the hash table is constructed.

diff --git a/src/hotspot/share/oops/klass.cpp b/src/hotspot/share/oops/klass.cpp
index bdbcae857d2..c87380fc8de 100644
--- a/src/hotspot/share/oops/klass.cpp
+++ b/src/hotspot/share/oops/klass.cpp
@@ -367,11 +370,10 @@ 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) {
+  // 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;
   }

Jul 26 '24 15:07 theRealAph

Oh, it comes as a surprise to me... I was under impression that the first thing hand-coded assembly variants do is check for bitmap != SECONDARY_SUPERS_BITMAP_FULL. At least, it was my recollection from working on JDK-8180450. (And the initial version of the patch with the check in Klass::lookup_secondary_supers_table() and _bitmap(SECONDARY_SUPERS_BITMAP_FULL) only reassured me that's still the case.)

The root cause of this bug is that we're not maintaining the invariant popcount(bitmap) == secondary_supers()->length().

The invariant holds only when bitmap != SECONDARY_SUPERS_BITMAP_FULL. It does help that even in case of non-hashed secondary_supers list secondary_supers()->length() >= popcount(bitmap), but initial probing becomes much less efficient (a random probe followed by a full linear pass over secondary supers list).

Alternatively, all table lookups can be adjusted to start with bitmap != SECONDARY_SUPERS_BITMAP_FULL checks before probing the table. It does add a branch on the fast path (and slightly increases inlined snippet code size), but the branch is highly predictable and works on a value we need anyway. So, I would be surprised to see any performance effects from it. IMO it's easier to reason and more flexible: SECONDARY_SUPERS_BITMAP_FULL == bitmap simply disables all table lookups and unconditionally falls back to linear search.

Jul 26 '24 18:07 iwanowww

Oh, it comes as a surprise to me... I was under impression that the first thing hand-coded assembly variants do is check for bitmap != SECONDARY_SUPERS_BITMAP_FULL. At least, it was my recollection from working on JDK-8180450. (And the initial version of the patch with the check in Klass::lookup_secondary_supers_table() and _bitmap(SECONDARY_SUPERS_BITMAP_FULL) only reassured me that's still the case.)

The root cause of this bug is that we're not maintaining the invariant popcount(bitmap) == secondary_supers()->length().

The invariant holds only when bitmap != SECONDARY_SUPERS_BITMAP_FULL.

Yes, exactly so. That's what I intended to mean.

It does help that even in case of non-hashed secondary_supers list secondary_supers()->length() >= popcount(bitmap), but initial probing becomes much less efficient (a random probe followed by a full linear pass over secondary supers list).

Alternatively, all table lookups can be adjusted to start with bitmap != SECONDARY_SUPERS_BITMAP_FULL checks before probing the table. It does add a branch on the fast path (and slightly increases inlined snippet code size), but the branch is highly predictable and works on a value we need anyway.

True enough. I've been trying to move as much as I can out of the inlined code, though.

So, I would be surprised to see any performance effects from it. IMO it's easier to reason and more flexible: SECONDARY_SUPERS_BITMAP_FULL == bitmap simply disables all table lookups and unconditionally falls back to linear search.

I take your point. But that seems like it's a bit of a sledgehammer to crack a walnut, don't you think? Given that it's only necessary to fix a rare edge case. But I'm not going to be precious about this choice, I'll test for a full bitmap first if you prefer. It's only a couple of instructions, but they are in the fast path that is successful in almost 99% of cases.

Jul 26 '24 21:07 theRealAph

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.

Jul 26 '24 21:07 iwanowww

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; }

Jul 27 '24 00:07 iwanowww

/label remove core-libs

Jul 27 '24 06:07 AlanBateman

@AlanBateman The core-libs label was successfully removed.

Jul 27 '24 06:07 openjdk[bot]

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.

Jul 29 '24 13:07 theRealAph

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.

Jul 29 '24 13:07 theRealAph

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.

Jul 29 '24 23:07 iwanowww

Looks good.

Testing results (hs-tier1 - hs-tier6) are clean.

Great! Is there anyone you'd suggest for additional review?

Sep 09 '24 10:09 theRealAph

I had to merge from JDK head to fix a conflict.

Sep 09 '24 13:09 theRealAph

@dougxc will this affect Graal?

Oct 09 '24 00:10 vnkozlov

@dougxc will this affect Graal?

@mur47x111 can you please check (if you haven't already).

Thanks for the heads up @vnkozlov .

Oct 09 '24 06:10 dougxc

Graal-side change is ready

Oct 09 '24 11:10 mur47x111

Hi, I ran some jmh tests on the arm64 platform and the performance SecondarySupersLookup.testPositive seems to have decreased, is this as expected? before this patch:

Benchmark                             Mode  Cnt   Score   Error  Units
SecondarySupersLookup.testNegative00  avgt   15   2.455 ± 0.215  ns/op
SecondarySupersLookup.testNegative01  avgt   15   2.481 ± 0.202  ns/op
SecondarySupersLookup.testNegative02  avgt   15   2.455 ± 0.216  ns/op
SecondarySupersLookup.testNegative03  avgt   15   2.457 ± 0.212  ns/op
SecondarySupersLookup.testNegative04  avgt   15   2.463 ± 0.209  ns/op
SecondarySupersLookup.testNegative05  avgt   15   2.462 ± 0.211  ns/op
SecondarySupersLookup.testNegative06  avgt   15   2.455 ± 0.216  ns/op
SecondarySupersLookup.testNegative07  avgt   15   2.455 ± 0.215  ns/op
SecondarySupersLookup.testNegative08  avgt   15   2.456 ± 0.215  ns/op
SecondarySupersLookup.testNegative09  avgt   15   2.499 ± 0.199  ns/op
SecondarySupersLookup.testNegative10  avgt   15   2.456 ± 0.214  ns/op
SecondarySupersLookup.testNegative16  avgt   15   2.459 ± 0.214  ns/op
SecondarySupersLookup.testNegative20  avgt   15   2.458 ± 0.216  ns/op
SecondarySupersLookup.testNegative30  avgt   15   2.458 ± 0.215  ns/op
SecondarySupersLookup.testNegative32  avgt   15   2.457 ± 0.217  ns/op
SecondarySupersLookup.testNegative40  avgt   15   2.456 ± 0.217  ns/op
SecondarySupersLookup.testNegative50  avgt   15   2.482 ± 0.209  ns/op
SecondarySupersLookup.testNegative55  avgt   15  12.217 ± 1.594  ns/op
SecondarySupersLookup.testNegative56  avgt   15  12.756 ± 1.523  ns/op
SecondarySupersLookup.testNegative57  avgt   15  11.641 ± 1.264  ns/op
SecondarySupersLookup.testNegative58  avgt   15  11.088 ± 0.066  ns/op
SecondarySupersLookup.testNegative59  avgt   15  11.668 ± 1.256  ns/op
SecondarySupersLookup.testNegative60  avgt   15  21.025 ± 0.146  ns/op
SecondarySupersLookup.testNegative61  avgt   15  20.944 ± 0.175  ns/op
SecondarySupersLookup.testNegative62  avgt   15  21.159 ± 0.297  ns/op
SecondarySupersLookup.testNegative63  avgt   15  49.390 ± 1.943  ns/op
SecondarySupersLookup.testNegative64  avgt   15  49.426 ± 0.989  ns/op
SecondarySupersLookup.testPositive01  avgt   15   1.710 ± 0.070  ns/op
SecondarySupersLookup.testPositive02  avgt   15   1.726 ± 0.071  ns/op
SecondarySupersLookup.testPositive03  avgt   15   1.565 ± 0.169  ns/op
SecondarySupersLookup.testPositive04  avgt   15   1.591 ± 0.064  ns/op
SecondarySupersLookup.testPositive05  avgt   15   1.684 ± 0.115  ns/op
SecondarySupersLookup.testPositive06  avgt   15   1.546 ± 0.156  ns/op
SecondarySupersLookup.testPositive07  avgt   15   1.522 ± 0.134  ns/op
SecondarySupersLookup.testPositive08  avgt   15   1.479 ± 0.114  ns/op
SecondarySupersLookup.testPositive09  avgt   15   1.742 ± 0.061  ns/op
SecondarySupersLookup.testPositive10  avgt   15   1.531 ± 0.123  ns/op
SecondarySupersLookup.testPositive16  avgt   15   1.540 ± 0.150  ns/op
SecondarySupersLookup.testPositive20  avgt   15   1.558 ± 0.169  ns/op
SecondarySupersLookup.testPositive30  avgt   15   1.531 ± 0.096  ns/op
SecondarySupersLookup.testPositive32  avgt   15   1.541 ± 0.139  ns/op
SecondarySupersLookup.testPositive40  avgt   15   1.487 ± 0.101  ns/op
SecondarySupersLookup.testPositive50  avgt   15   1.521 ± 0.144  ns/op
SecondarySupersLookup.testPositive60  avgt   15   1.512 ± 0.163  ns/op
SecondarySupersLookup.testPositive63  avgt   15   1.745 ± 0.100  ns/op
SecondarySupersLookup.testPositive64  avgt   15   1.557 ± 0.190  ns/op
Finished running test 'micro:vm.lang.SecondarySupersLookup'

Apply this patch:

Benchmark                             Mode  Cnt   Score   Error  Units
SecondarySupersLookup.testNegative00  avgt   15   2.559 ± 0.141  ns/op
SecondarySupersLookup.testNegative01  avgt   15   2.603 ± 0.119  ns/op
SecondarySupersLookup.testNegative02  avgt   15   2.550 ± 0.145  ns/op
SecondarySupersLookup.testNegative03  avgt   15   2.588 ± 0.117  ns/op
SecondarySupersLookup.testNegative04  avgt   15   2.556 ± 0.144  ns/op
SecondarySupersLookup.testNegative05  avgt   15   2.623 ± 0.090  ns/op
SecondarySupersLookup.testNegative06  avgt   15   2.674 ± 0.074  ns/op
SecondarySupersLookup.testNegative07  avgt   15   2.615 ± 0.132  ns/op
SecondarySupersLookup.testNegative08  avgt   15   2.559 ± 0.136  ns/op
SecondarySupersLookup.testNegative09  avgt   15   2.581 ± 0.119  ns/op
SecondarySupersLookup.testNegative10  avgt   15   2.612 ± 0.111  ns/op
SecondarySupersLookup.testNegative16  avgt   15   2.571 ± 0.142  ns/op
SecondarySupersLookup.testNegative20  avgt   15   2.594 ± 0.119  ns/op
SecondarySupersLookup.testNegative30  avgt   15   2.560 ± 0.144  ns/op
SecondarySupersLookup.testNegative32  avgt   15   2.653 ± 0.129  ns/op
SecondarySupersLookup.testNegative40  avgt   15   2.594 ± 0.115  ns/op
SecondarySupersLookup.testNegative50  avgt   15   2.604 ± 0.125  ns/op
SecondarySupersLookup.testNegative55  avgt   15  12.003 ± 1.077  ns/op
SecondarySupersLookup.testNegative56  avgt   15  11.483 ± 0.053  ns/op
SecondarySupersLookup.testNegative57  avgt   15  12.506 ± 1.394  ns/op
SecondarySupersLookup.testNegative58  avgt   15  12.027 ± 1.157  ns/op
SecondarySupersLookup.testNegative59  avgt   15  13.481 ± 1.117  ns/op
SecondarySupersLookup.testNegative60  avgt   15  20.952 ± 0.080  ns/op
SecondarySupersLookup.testNegative61  avgt   15  21.006 ± 0.196  ns/op
SecondarySupersLookup.testNegative62  avgt   15  21.007 ± 0.098  ns/op
SecondarySupersLookup.testNegative63  avgt   15  48.050 ± 1.293  ns/op
SecondarySupersLookup.testNegative64  avgt   15  49.669 ± 0.730  ns/op
SecondarySupersLookup.testPositive01  avgt   15   4.235 ± 0.044  ns/op
SecondarySupersLookup.testPositive02  avgt   15   4.215 ± 0.032  ns/op
SecondarySupersLookup.testPositive03  avgt   15   4.211 ± 0.032  ns/op
SecondarySupersLookup.testPositive04  avgt   15   4.219 ± 0.022  ns/op
SecondarySupersLookup.testPositive05  avgt   15   4.244 ± 0.025  ns/op
SecondarySupersLookup.testPositive06  avgt   15   4.217 ± 0.038  ns/op
SecondarySupersLookup.testPositive07  avgt   15   4.221 ± 0.034  ns/op
SecondarySupersLookup.testPositive08  avgt   15   4.233 ± 0.030  ns/op
SecondarySupersLookup.testPositive09  avgt   15   4.266 ± 0.069  ns/op
SecondarySupersLookup.testPositive10  avgt   15   4.223 ± 0.039  ns/op
SecondarySupersLookup.testPositive16  avgt   15   4.234 ± 0.023  ns/op
SecondarySupersLookup.testPositive20  avgt   15   4.223 ± 0.038  ns/op
SecondarySupersLookup.testPositive30  avgt   15   4.219 ± 0.033  ns/op
SecondarySupersLookup.testPositive32  avgt   15   4.225 ± 0.052  ns/op
SecondarySupersLookup.testPositive40  avgt   15   7.201 ± 2.232  ns/op
SecondarySupersLookup.testPositive50  avgt   15   4.198 ± 0.022  ns/op
SecondarySupersLookup.testPositive60  avgt   15   6.369 ± 1.828  ns/op
SecondarySupersLookup.testPositive63  avgt   15  55.590 ± 0.208  ns/op
SecondarySupersLookup.testPositive64  avgt   15  58.098 ± 1.861  ns/op
Finished running test 'micro:vm.lang.SecondarySupersLookup'

Oct 09 '24 12:10 zifeihan

Hi, I ran some jmh tests on the arm64 platform and the performance SecondarySupersLookup.testPositive seems to have decreased, is this as expected? before this patch:

Yes.

The amazingly fast positive test results you're seeing are something of an illusion. After this patch, the single-entry _secondary_super_cache field, shared among all threads, is no longer in use. The use of this field led to catastrophic scaling problems.

What you're seeing in your test reult is a hangover from the "old" way of doing things. Even though C2-compiled code does not update _secondary_super_cache, that field just happens to contain the result of a previous lookup, either from C1 or the interpreter.

After this patch, the positive lookup time is still very fast in all reasonable cases, and it's near to the best that can be done while avoiding extreme slowdowns.

Oct 10 '24 11:10 theRealAph

By the way, to see the performance benefit of this patch run the test case SecondarySuperCacheInterContention.java, like so:

/build/macosx-aarch64-server-fastdebug/jdk/bin/java -XX:-UseSecondarySupersTable -XX:+UseSecondarySupersCache -jar build/macosx-aarch64-server-fastdebug/images/test/micro/benchmarks.jar SecondarySuperCacheInterContention

Oct 15 '24 10:10 theRealAph

@theRealAph Hi, And here is the version for riscv: https://github.com/zifeihan/jdk/commit/2c5296859597f5edc6601a5d1780126ec373d4dc . 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).

Oct 23 '24 05:10 zifeihan

jdk jdk copied to clipboard

8331341: secondary_super_cache does not scale well: C1 and interpreter

HotSpot shared runtime

x86

AArch64

Generally

Progress

Issue

Reviewers

Reviewing

Webrev

Webrevs

jdk
jdk copied to clipboard