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

[Vladimir Ivanov]

On Mon, 29 Jul 2024 10:35:07 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 incrementally with one additional commit since the last revision:
> 
>   Minor

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.

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

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