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

[Andrew Haley]

On Thu, 11 Jul 2024 23:39:11 GMT, Vladimir Ivanov <vlivanov 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...
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp line 1040:
> 
>> 1038: 
>> 1039:   // Secondary subtype checking
>> 1040:   void lookup_secondary_supers_table(Register sub_klass,
> 
> While browsing the code, I noticed that it's far from evident at call sites which overload is used (especially with so many arguments). Does it make sense to avoid method overloads here and use distinct method names instead?

So I confess: this is surely true, but I failed to think of a name for the known- and unknown-at-compile-time versions. maybe  `check_const_klass_subtype_slow_path_table` and `check_var_klass_subtype_slow_path_table` ?

> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 1981:
> 
>> 1979:       __ load_klass(r19_klass, copied_oop);// query the object klass
>> 1980: 
>> 1981:       BLOCK_COMMENT("type_check:");
> 
> Why don't you move it inside `generate_type_check`?

Sorry, what? Do you mean move just this block comment?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1683182967
PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1683184664