On Tue, 7 Feb 2023 15:25:05 GMT, Claes Redestad <redes...@openjdk.org> wrote:
>> This adds a local, specialized `copyBytes` method to `String` that avoids >> certain redundant range checks and clamping that JIT has issues removing >> fully. >> >> This has a small but statistically significant effect on `String` >> microbenchmarks, eg.: >> >> Baseline >> >> Benchmark (size) Mode Cnt >> Score Error Units >> StringConstructor.newStringFromArray 7 avgt 15 >> 16.817 ± 0.369 ns/op >> StringConstructor.newStringFromArrayWithCharset 7 avgt 15 >> 16.866 ± 0.449 ns/op >> StringConstructor.newStringFromArrayWithCharsetName 7 avgt 15 >> 22.198 ± 0.396 ns/op >> >> >> Patch: >> >> Benchmark (size) Mode Cnt >> Score Error Units >> StringConstructor.newStringFromArray 7 avgt 15 >> 15.477 ± 0.342 ns/op >> StringConstructor.newStringFromArrayWithCharset 7 avgt 15 >> 15.557 ± 0.352 ns/op >> StringConstructor.newStringFromArrayWithCharsetName 7 avgt 15 >> 21.272 ± 0.398 ns/op >> >> >> Care has to be taken to ensure preconditions have been checked when using >> `checkBytes`. In the case of `String(AbstractStringBuilder)` there's a >> possible pre-existing issue where the constructor might either throw an >> exception or truncate the buffer if the builder byte array and length is not >> in agreement (theoretically possible if you clear/remove and call >> `trimToSize()` concurrently). Adding an explicit check here seem to be the >> right thing to do regardless of this RFE. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > copyrights Our source code is a reference implementation, and people will look at this change as evidence that `Arrays::copyOfRange` should be hand-inlined by savvy coders. Surely we could also fix this small performance pothole by improving C2’s treatment of `Arrays.copyOfRange`. That would benefit all users as well, not just `String`. That is our preferred way to handle things. On the other hand, `String` is an important class and worthy of every tiny tweak we give it. Do we need this fix now? If so, I suggest putting in a comment in the code which says something like “normally one would use Arrays.copyOfRange here, but we get slightly better code in this particular case”. Also, regarding the bug against the JIT, I suggest that we back out this change to `String` when that JIT bug is fixed. Perhaps the comment in `String` should reference that bug. ------------- PR: https://git.openjdk.org/jdk/pull/12453
