On Mon, 4 Dec 2023 15:57:45 GMT, Claes Redestad <[email protected]> wrote:
>> `StringUTF16::equals` was used before >> https://bugs.openjdk.org/browse/JDK-8215017 - I don't remember exactly what >> performance verification I did back then. x86 intrinsics seem to emit the >> exact same asm, aarch64 does a little bit of extra optimization for the >> UTF16 case (avoids code and a branch for checking tailing byte, but only if >> the UTF16 string is 1-3 elements long). Perhaps we should double-check that >> JDK-8215017 didn't significantly regress some UTF16 equality checks before >> we rip out code. > > Going out on this tangent... I've compared the mainline (Base) with a build > where `String::equals` is restored to pre-JDK-8215017 using a modified > version of the StringEquals microbenchmark that tests UTF16 Strings of size > 3. One test (EQ) where the strings are equals, another (NE) where they are > not. The EQ one is the main contender for a case that would benefit from > avoiding the trailing byte check on aarch64: > > On my M1 (aarch64): > > > Name Cnt Base Error Test Error Unit Change > StringEquals.equalsUTF16_3_EQ 5 1,750 ± 0,009 1,864 ± 0,401 ns/op 0,94x > (p = 0,070 ) > StringEquals.equalsUTF16_3_NE 5 1,674 ± 0,026 1,839 ± 0,179 ns/op 0,91x > (p = 0,001*) > * = significant > ``` > > Similarly restoring the pre-JDK-8215017 version seem to be a net loss on x86: > > Name Cnt Base Error Test Error Unit Change > StringEquals.equalsUTF16_3_EQ 5 2.885 ± 0.054 2.886 ± 0.036 ns/op 1.00x > (p = 0.837 ) > StringEquals.equalsUTF16_3_NE 5 2.581 ± 0.002 2.756 ± 0.003 ns/op 0.94x > (p = 0.000*) > * = significant > > > So it seems JDK-8215017 was either neutral or a small performance win > (phew!). The avoided branch and overall reduction in code complexity > outweighs the win (if any) from not having the redundant `COMPARE_BYTE` chunk > of code emitted in the StringUTF16 case. There are other platforms I can't > currently check, but I think it's likely that we'd see similar numbers there. > So it seems to be the case that `StringUTF16::equals` can be safely removed. Nice one, I can happily send a PR for it, but I feel bad about it; you have performed the check yourself (and were aware of what dropping it would mean...) separated question @cl4es, how do you obtained the 'Change` column with the `p` value? is in some jdk script I've missed? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16933#discussion_r1414125814 PR Review Comment: https://git.openjdk.org/jdk/pull/16933#discussion_r1414129103
