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

[Vladimir Ivanov]

On Thu, 11 Jul 2024 23:17:10 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.cpp line 1433:
> 
>> 1431: 
>> 1432:   // Don't check secondary_super_cache
>> 1433:   if (super_check_offset.is_register()
> 
> Do you see any effects from this particular change?
> 
> It adds a runtime check on the fast path for all subtype checks (irrespective of whether it checks primary or secondary super). Moreover, the very same check is performed after primary super slot is checked.
> 
> Unless `_secondary_super_cache` field is removed, unconditionally checking the slot at `super_check_offset` is benign.

BTW `MacroAssembler::check_klass_subtype_fast_path` deserves a cleanup: `super_check_offset` can be safely turned into `Register` thus eliminating the code guarded by `super_check_offset.is_register() == false`.

> src/hotspot/share/oops/instanceKlass.cpp line 1410:
> 
>> 1408:     return nullptr;
>> 1409:   } else if (num_extra_slots == 0) {
>> 1410:     if (num_extra_slots == 0 && interfaces->length() <= 1) {
> 
> Since `secondary_supers` are hashed unconditionally now,  is `interfaces->length() <= 1` check still needed?

Also, `num_extra_slots == 0` check is redundant.

> src/hotspot/share/oops/klass.cpp line 284:
> 
>> 282: // which doesn't zero out the memory before calling the constructor.
>> 283: Klass::Klass(KlassKind kind) : _kind(kind),
>> 284:                                _bitmap(SECONDARY_SUPERS_BITMAP_FULL),
> 
> I like the idea, but what are the benefits of initializing `_bitmap` separately from `_secondary_supers`?

Another observation while browsing the code: `_secondary_supers_bitmap` would be a better name. (Same considerations apply to `_hash_slot`.)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1674815196
PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1674798719
PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1674828164