RFR: 8345120: A likely bug in StringSupport::chunkedStrlenShort [v3]

[Maurizio Cimadamore]

On Fri, 29 Nov 2024 09:46:55 GMT, Per Minborg <pminborg at openjdk.org> wrote:

>> This PR proposes to rewrite the `StringSupport::chunkedStrlen*`  methods, fixing a bug in the `short_strlen` variant for odd offsets (`offset % 2 != 0`).
>> 
>> This PR also improves performance on modern hardware, as there is no need for pre-looping alignment. Removing this improves performance by about 30% for larger strings.
>> 
>> It passes the `jdk_foreign` test suit. Also, `tier1` through `tier3` passes on various platforms and configurations.
>> 
>> Base:
>> 
>> 
>> Benchmark                               (size)  Mode  Cnt    Score   Error  Units
>> InternalStrLen.changedElementQuad            1  avgt   30    2.057 ? 0.012  ns/op
>> InternalStrLen.changedElementQuad            4  avgt   30    3.776 ? 0.031  ns/op
>> InternalStrLen.changedElementQuad           16  avgt   30    6.690 ? 0.060  ns/op
>> InternalStrLen.changedElementQuad          251  avgt   30   48.581 ? 0.764  ns/op
>> InternalStrLen.changedElementQuad         1024  avgt   30  196.188 ? 3.484  ns/op
>> InternalStrLen.chunkedDouble                 1  avgt   30    1.903 ? 0.013  ns/op
>> InternalStrLen.chunkedDouble                 4  avgt   30    3.446 ? 0.025  ns/op
>> InternalStrLen.chunkedDouble                16  avgt   30    5.759 ? 0.062  ns/op
>> InternalStrLen.chunkedDouble               251  avgt   30   26.892 ? 0.141  ns/op
>> InternalStrLen.chunkedDouble              1024  avgt   30   72.940 ? 1.562  ns/op
>> InternalStrLen.chunkedSingle                 1  avgt   30    1.897 ? 0.015  ns/op
>> InternalStrLen.chunkedSingle                 4  avgt   30    5.357 ? 0.560  ns/op
>> InternalStrLen.chunkedSingle                16  avgt   30    3.821 ? 0.052  ns/op
>> InternalStrLen.chunkedSingle               251  avgt   30   19.482 ? 0.190  ns/op
>> InternalStrLen.chunkedSingle              1024  avgt   30   38.938 ? 0.411  ns/op
>> InternalStrLen.chunkedSingleMisaligned       1  avgt   30    2.230 ? 0.147  ns/op
>> InternalStrLen.chunkedSingleMisaligned       4  avgt   30    5.424 ? 0.688  ns/op
>> InternalStrLen.chunkedSingleMisaligned      16  avgt   30    9.573 ? 0.063  ns/op
>> InternalStrLen.chunkedSingleMisaligned     251  avgt   30   22.242 ? 0.182  ns/op
>> InternalStrLen.chunkedSingleMisaligned    1024  avgt   30   45.442 ? 0.252  ns/op
>> InternalStrLen.elementByteMisaligned         1  avgt   30    1.616 ? 0.041  ns/op
>> InternalStrLen.elementByteMisaligned         4  avgt   30    2.982 ? 0.018  ns/op
>> InternalStrLen.elementByteMisaligned        16  avgt   30    8.662 ? 0.085  ns/op
>> InternalStrLe...
>
> Per Minborg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix imports

src/java.base/share/classes/jdk/internal/foreign/StringSupport.java line 187:

> 185:         // Prevent over scanning as we step by 2
> 186:         final long endScan = toOffset & ~1; // The last bit is zero
> 187:         for (; offset < endScan; offset += Short.BYTES) {

Question: do we care about finding unaligned sequences of two consecutive zero bytes? Because I don't think this method can detect those. E.g. if the first zero byte in a terminator sequence starts at an odd offset I don't think we can recognize the terminator.

This is a bit of a weird side-effect, because now this method no longer behaves like a plain byte loop where we look for two consecutive bytes. But, I suppose, in a way it also makes sense: if I want to read a C string whose characters are 2-bytes each, then I definitively need to scan the segment (at given offset) two bytes at a time. A terminator at an "odd offset" from the start of the scan is not good. I believe the javadoc is a bit "light" on how this terminator is actually found :-)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22451#discussion_r1863463824