RFR: 8332587: RISC-V: secondary_super_cache does not scale well

Mon Jun 3 18:14:04 UTC 2024

On Tue, 21 May 2024 08:31:53 GMT, Gui Cao <gcao at openjdk.org> wrote:

> Implementation of subtype checking [JDK-8180450](https://bugs.openjdk.org/browse/JDK-8180450) for linux-riscv64.
> This optimization depends on availability of the Zbb extension which has the cpop instruction.
> 
> ### Correctness testing:
> 
> - [x]  Run tier1-3, hotspot:tier4 tests on SOPHON SG2042 (release)
> - [x] Run tier1-3 tests with -XX:+UseRVV on qemu 8.1.0 (fastdebug)
> - [x] Run all of tier1 with `-XX:+VerifySecondarySupers`
> 
> ### JMH tested on Banana Pi BPI-F3 board (has Zbb) and Enable UseZbb, Not Enable UseZba, UseZbs
> Original:
> 
> Benchmark                             Mode  Cnt    Score   Error  Units
> SecondarySuperCacheHits.test  avgt   15  11.375 ± 0.071  ns/op
> SecondarySuperCacheInterContention.test     avgt   15  646.087 ± 32.587  ns/op
> SecondarySuperCacheInterContention.test:t1  avgt   15  600.090 ± 83.779  ns/op
> SecondarySuperCacheInterContention.test:t2  avgt   15  692.084 ± 73.218  ns/op
> SecondarySupersLookup.testNegative00  avgt   15   16.420 ± 0.239  ns/op
> SecondarySupersLookup.testNegative01  avgt   15   18.307 ± 0.260  ns/op
> SecondarySupersLookup.testNegative02  avgt   15   21.695 ± 0.458  ns/op
> SecondarySupersLookup.testNegative03  avgt   15   24.855 ± 0.664  ns/op
> SecondarySupersLookup.testNegative04  avgt   15   27.305 ± 0.522  ns/op
> SecondarySupersLookup.testNegative05  avgt   15   29.719 ± 0.385  ns/op
> SecondarySupersLookup.testNegative06  avgt   15   32.231 ± 0.498  ns/op
> SecondarySupersLookup.testNegative07  avgt   15   33.747 ± 0.603  ns/op
> SecondarySupersLookup.testNegative08  avgt   15   35.856 ± 0.629  ns/op
> SecondarySupersLookup.testNegative09  avgt   15   37.077 ± 0.546  ns/op
> SecondarySupersLookup.testNegative10  avgt   15   39.408 ± 0.465  ns/op
> SecondarySupersLookup.testNegative16  avgt   15   51.041 ± 0.547  ns/op
> SecondarySupersLookup.testNegative20  avgt   15   58.722 ± 0.922  ns/op
> SecondarySupersLookup.testNegative30  avgt   15   77.310 ± 0.654  ns/op
> SecondarySupersLookup.testNegative32  avgt   15   81.116 ± 0.854  ns/op
> SecondarySupersLookup.testNegative40  avgt   15   96.311 ± 0.840  ns/op
> SecondarySupersLookup.testNegative50  avgt   15  115.427 ± 0.838  ns/op
> SecondarySupersLookup.testNegative55  avgt   15  124.371 ± 1.076  ns/op
> SecondarySupersLookup.testNegative56  avgt   15  126.796 ± 0.916  ns/op
> SecondarySupersLookup.testNegative57  avgt   15  127.952 ± 1.202  ns/op
> SecondarySupersLookup.testNegative58  avgt   15  131.956 ± 4.515  ns/op
> SecondarySupersLookup.testNegative59  avgt   15  131.858 ± 1.066  ns/op
> SecondaryS...

Some more comments.

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 3525:

> 3523:   mv(result, 1);
> 3524: 
> 3525:   ld(r_bitmap, Address(r_sub_klass, Klass::bitmap_offset()));

~~when bitmap is `SECONDARY_SUPERS_BITMAP_FULL`, can we skip the hash+bitmap lookup, and go to linear lookup (e.g. `repne_scan ` below)?~~
Ignore it, I think the logic is put in `lookup_secondary_supers_table_slow_path`, seems that's fine.

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 3605:

> 3603:   LOOKUP_SECONDARY_SUPERS_TABLE_REGISTERS;
> 3604: 
> 3605:   Label L_matched, L_fallthrough, L_huge;

Seems to me, `L_bitmap_full` is more meanful than `L_huge`.

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 3607:

> 3605:   Label L_matched, L_fallthrough, L_huge;
> 3606: 
> 3607:   // Make sure that result is nonzero

value `1` of `result` here means mismatch? so the comment is confusing. I think something like "initialize result value to 1 which means mismatch." will be more meaningful.

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 3619:

> 3617:   // The bitmap is full to bursting.
> 3618:   // Implicit invariant: BITMAP_FULL implies (length > 0)
> 3619:   assert(Klass::SECONDARY_SUPERS_BITMAP_FULL == ~uintx(0), "");

maybe the assert messge could be "Adjust this code" too?

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 3620:

> 3618:   // Implicit invariant: BITMAP_FULL implies (length > 0)
> 3619:   assert(Klass::SECONDARY_SUPERS_BITMAP_FULL == ~uintx(0), "");
> 3620:   addi(t0, r_bitmap, (u1)1);

A comment like "check if bitmap is `SECONDARY_SUPERS_BITMAP_FULL `" will help to understand.

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 3659:

> 3657:     // complexity?
> 3658:     bind(L_huge);
> 3659:     repne_scan(r_array_base, r_super_klass, r_array_length, t0);

~~Seems this will start to scan at 0 index in r_array_base? maybe we just need to start at index 64?~~
Ignore it, I misunderstand the code, it's only for bitmap full case, and it starts to scan from 0 index.

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

PR Review: https://git.openjdk.org/jdk/pull/19320#pullrequestreview-2094397977
PR Review Comment: https://git.openjdk.org/jdk/pull/19320#discussion_r1624826855
PR Review Comment: https://git.openjdk.org/jdk/pull/19320#discussion_r1624854699
PR Review Comment: https://git.openjdk.org/jdk/pull/19320#discussion_r1624836606
PR Review Comment: https://git.openjdk.org/jdk/pull/19320#discussion_r1624832722
PR Review Comment: https://git.openjdk.org/jdk/pull/19320#discussion_r1624859553
PR Review Comment: https://git.openjdk.org/jdk/pull/19320#discussion_r1624768434