RFR: 8331341: secondary_super_cache does not scale well: C1 and interpreter [v8]

[Vladimir Ivanov]

On Thu, 25 Jul 2024 16:05:49 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> 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 i...
>
> Andrew Haley has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 40 commits:
> 
>  - Merge branch 'clean' into JDK-8331658-work
>  - Cleanup
>  - temp
>  - Merge branch 'JDK-8331658-work' of https://github.com/theRealAph/jdk into JDK-8331658-work
>  - Review comments
>  - Review comments
>  - Review comments
>  - Review comments
>  - Review feedback
>  - Review feedback
>  - ... and 30 more: https://git.openjdk.org/jdk/compare/34ee06f5...248f44dc

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](https://github.com/openjdk/jdk/pull/19989/commits/54050a5c2c0aa1d6a9e36d0240c66345765845e3) 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


Do we miss

-------------

PR Comment: https://git.openjdk.org/jdk/pull/19989#issuecomment-2251567973