RFR: 8331126: [s390x] secondary_super_cache does not scale well [v4]

[Lutz Schmidt]

On Mon, 17 Jun 2024 10:19:46 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

>> s390x Port for [JDK-8180450](https://bugs.openjdk.org/browse/JDK-8180450)
>> 
>> I ran `tier1` test with  `-XX:+UseSecondarySupersTable -XX:+VerifySecondarySupers -XX:+StressSecondarySupers` in fastdebug vm and didn't see any new failure appearing; Except one I have mentioned [here](https://github.com/openjdk/jdk/pull/19368#issuecomment-2154983693); But this is reproducible on every other architecture with these flags. 
>> 
>> 
>> Without Patch: 
>> 
>> SecondarySuperCacheHits.test  avgt   15  0.929 ± 0.010  ns/op
>> 
>> SecondarySuperCacheInterContention.test     avgt   15  1.413 ± 0.007  ns/op
>> SecondarySuperCacheInterContention.test:t1  avgt   15  1.415 ± 0.016  ns/op
>> SecondarySuperCacheInterContention.test:t2  avgt   15  1.410 ± 0.017  ns/op
>> 
>> Benchmark                             Mode  Cnt   Score   Error  Units
>> SecondarySupersLookup.testNegative00  avgt   15   1.806 ± 0.325  ns/op
>> SecondarySupersLookup.testNegative01  avgt   15   2.364 ± 0.236  ns/op
>> SecondarySupersLookup.testNegative02  avgt   15   2.903 ± 0.215  ns/op
>> SecondarySupersLookup.testNegative03  avgt   15   3.417 ± 0.199  ns/op
>> SecondarySupersLookup.testNegative04  avgt   15   3.758 ± 0.102  ns/op
>> SecondarySupersLookup.testNegative05  avgt   15   4.352 ± 0.123  ns/op
>> SecondarySupersLookup.testNegative06  avgt   15   4.800 ± 0.099  ns/op
>> SecondarySupersLookup.testNegative07  avgt   15   5.365 ± 0.060  ns/op
>> SecondarySupersLookup.testNegative08  avgt   15   6.316 ± 0.092  ns/op
>> SecondarySupersLookup.testNegative09  avgt   15   6.669 ± 0.164  ns/op
>> SecondarySupersLookup.testNegative10  avgt   15   7.041 ± 0.164  ns/op
>> SecondarySupersLookup.testNegative16  avgt   15   9.336 ± 0.185  ns/op
>> SecondarySupersLookup.testNegative20  avgt   15  11.373 ± 0.029  ns/op
>> SecondarySupersLookup.testNegative30  avgt   15  15.236 ± 0.051  ns/op
>> SecondarySupersLookup.testNegative32  avgt   15  16.031 ± 0.091  ns/op
>> SecondarySupersLookup.testNegative40  avgt   15  19.197 ± 0.279  ns/op
>> SecondarySupersLookup.testNegative50  avgt   15  23.804 ± 2.387  ns/op
>> SecondarySupersLookup.testNegative55  avgt   15  25.610 ± 1.155  ns/op
>> SecondarySupersLookup.testNegative56  avgt   15  26.128 ± 2.203  ns/op
>> SecondarySupersLookup.testNegative57  avgt   15  26.126 ± 0.881  ns/op
>> SecondarySupersLookup.testNegative58  avgt   15  26.314 ± 0.521  ns/op
>> SecondarySupersLookup.testNegative59  avgt   15  26.750 ± 0.837  ns/op
>> SecondarySupersLookup.testNegative60  avgt   15  27.118 ± 0.557 ...
>
> Amit Kumar has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Update src/hotspot/cpu/s390/macroAssembler_s390.cpp
>  - rename: r_scratch to r_result in repne_scan method

Sorry, I was distracted yesterday and did not finish my review.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3231:

> 3229:   // We test the MSB of r_array_index, i.e., its sign bit
> 3230:   testbit(r_array_index, 63);
> 3231:   // TODO: load immediate on condition could be used here;

How would that help? You have to branch on condition anyway.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3259:

> 3257:   // Is there another entry to check? Consult the bitmap.
> 3258:   testbit(r_bitmap, (bit + 1) & Klass::SECONDARY_SUPERS_TABLE_MASK);
> 3259:   // TODO: load immediate on condition could be use here;

How would that help? You have to branch on condition anyway.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3264:

> 3262:   // Linear probe. Rotate the bitmap so that the next bit to test is
> 3263:   // in Bit 2 for the look-ahead check in the slow path.
> 3264:   if (bit) {

I don't like that. It's C style. Please use `(bit != 0)`.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3275:

> 3273:   call_stub(StubRoutines::lookup_secondary_supers_table_slow_path_stub());
> 3274: 
> 3275:   z_bru(L_done); // pass whatever result we got from a slow path

This one branch could be saved by using "load immediate on condition". But it's after slow path processing.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3325:

> 3323:   assert(Klass::SECONDARY_SUPERS_BITMAP_FULL == ~uintx(0), "");
> 3324: 
> 3325:   z_cghi(r_bitmap, (long)-1);

Why not compare against `(unitx)Klass::SECONDARY_SUPERS_BITMAP_FULL`?

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

Changes requested by lucy (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19544#pullrequestreview-2135877797
PR Review Comment: https://git.openjdk.org/jdk/pull/19544#discussion_r1651084089
PR Review Comment: https://git.openjdk.org/jdk/pull/19544#discussion_r1651084454
PR Review Comment: https://git.openjdk.org/jdk/pull/19544#discussion_r1651077231
PR Review Comment: https://git.openjdk.org/jdk/pull/19544#discussion_r1651094177
PR Review Comment: https://git.openjdk.org/jdk/pull/19544#discussion_r1651113235