RFR: 8254082: AbstractStringBuilder.insert(int dstOffset, CharSequence s, int start, int end) is missing fast-path for String [v4]

[Claes Redestad]

On Sat, 28 Nov 2020 22:05:01 GMT, Сергей Цыпанов <github.com+10835776+stsypanov at openjdk.org> wrote:

>>> @cl4es Could you tell me where I can look for a command to run the benchmark?
>> 
>> See [doc/testing.md](https://github.com/openjdk/jdk/blob/master/doc/testing.md). The [configuration](https://github.com/openjdk/jdk/blob/master/doc/testing.md#configuration) section has a pointer on how to set up and configure the JDK build with JMH, then it should be a simple matter of running `make test TEST=micro:org.openjdk.bench.java.lang.StringConcat`.
>
> Here are the results, looks like we have no regression:
> Benchmark                               (intValue)  Mode  Cnt         original           patched  Units  Units
> StringConcat.concat4String                    4711  avgt   15   27.267 ± 1.231    29.938 ± 1.099  ns/op  ns/op
> StringConcat.concat6String                    4711  avgt   15   39.185 ± 1.604    42.759 ± 2.564  ns/op  ns/op
> StringConcat.concatConst2String               4711  avgt   15   22.345 ± 1.685    21.913 ± 1.716  ns/op  ns/op
> StringConcat.concatConst4String               4711  avgt   15   32.318 ± 4.073    32.847 ± 1.082  ns/op  ns/op
> StringConcat.concatConst6Object               4711  avgt   15    9.227 ± 0.146     9.999 ± 0.939  ns/op  ns/op
> StringConcat.concatConst6String               4711  avgt   15   46.134 ± 2.729    44.903 ± 1.960  ns/op  ns/op
> StringConcat.concatConstBoolByte              4711  avgt   15   13.117 ± 0.725    12.575 ± 0.380  ns/op  ns/op
> StringConcat.concatConstInt                   4711  avgt   15   12.230 ± 0.653    11.890 ± 0.556  ns/op  ns/op
> StringConcat.concatConstIntConstInt           4711  avgt   15   18.152 ± 1.317    17.722 ± 0.566  ns/op  ns/op
> StringConcat.concatConstString                4711  avgt   15   12.644 ± 0.656    13.424 ± 1.400  ns/op  ns/op
> StringConcat.concatConstStringConstInt        4711  avgt   15   20.836 ± 0.703    19.975 ± 0.821  ns/op  ns/op
> StringConcat.concatEmptyConstInt              4711  avgt   15   10.604 ± 0.443    10.229 ± 0.219  ns/op  ns/op
> StringConcat.concatEmptyConstString           4711  avgt   15    4.844 ± 0.174     4.721 ± 0.147  ns/op  ns/op
> StringConcat.concatEmptyLeft                  4711  avgt   15    5.386 ± 0.190     5.314 ± 0.104  ns/op  ns/op
> StringConcat.concatEmptyRight                 4711  avgt   15    5.352 ± 0.471     5.484 ± 0.354  ns/op  ns/op
> StringConcat.concatMethodConstString          4711  avgt   15   11.887 ± 0.560    14.025 ± 1.522  ns/op  ns/op
> StringConcat.concatMix4String                 4711  avgt   15  172.425 ± 6.868   169.658 ± 8.440  ns/op  ns/op

Right.

I won't insist but I think some of these could warrant some extra runs and analysis of the disassembly just to make sure. E.g. `concatMethodConstString` and `concat6String` has regression with just barely non-overlapping errors. If these regressions persist it could be due the small difference in the code path (one extra call depth for `getBytes(byte[], int, byte)`) causing some difference in compilation order, inlining decision or otherwise. 

If the regression is real, the paranoid workaround would be to restore `String.getBytes(byte[], int, byte)` and not use delegation. A tiny bit of code duplication might be preferable to a surprise regression in code that has seen a lot of tuning work..

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

PR: https://git.openjdk.java.net/jdk/pull/402