On Mon, 4 Dec 2023 13:27:50 GMT, Claes Redestad <[email protected]> wrote:
>> I can send a separate PR to remove it in case, unless we have any plans to >> use it elsewhere > > `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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16933#discussion_r1414118658
