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

[Сергей Цыпанов]

On Sun, 29 Nov 2020 01:21:08 GMT, Claes Redestad <redestad at openjdk.org> wrote:

>> 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..

Looks like you are right: delegation caused tiny regression (I've rerun the benchmark in 10 forks) which is missing from the case with no delegation (third column):
Benchmark                               (intValue)  Mode  Cnt         original          patched     no delegate Units
StringConcat.concat4String                    4711  avgt  100   26.400 ± 0.092   28.278 ± 0.046  26.245 ± 0.045 ns/op
StringConcat.concat6String                    4711  avgt  100   37.846 ± 0.219   40.054 ± 0.150  37.857 ± 0.175 ns/op
StringConcat.concatConst2String               4711  avgt  100   18.732 ± 0.125   19.600 ± 0.153  18.598 ± 0.178 ns/op
StringConcat.concatConst4String               4711  avgt  100   29.303 ± 0.165   31.085 ± 0.086  29.246 ± 0.194 ns/op
StringConcat.concatConst6Object               4711  avgt  100    9.107 ± 0.164    8.948 ± 0.010   8.964 ± 0.059 ns/op
StringConcat.concatConst6String               4711  avgt  100   40.441 ± 0.111   41.037 ± 0.052  40.503 ± 0.352 ns/op
StringConcat.concatConstBoolByte              4711  avgt  100   11.934 ± 0.054   12.173 ± 0.197  11.904 ± 0.026 ns/op
StringConcat.concatConstInt                   4711  avgt  100   10.968 ± 0.024   11.041 ± 0.034  10.956 ± 0.006 ns/op
StringConcat.concatConstIntConstInt           4711  avgt  100   16.853 ± 0.070   16.958 ± 0.064  16.932 ± 0.070 ns/op
StringConcat.concatConstString                4711  avgt  100   11.440 ± 0.113   12.049 ± 0.116  11.485 ± 0.116 ns/op
StringConcat.concatConstStringConstInt        4711  avgt  100   18.205 ± 0.068   18.612 ± 0.043  18.146 ± 0.065 ns/op
StringConcat.concatEmptyConstInt              4711  avgt  100    9.733 ± 0.121    9.742 ± 0.250   9.592 ± 0.011 ns/op
StringConcat.concatEmptyConstString           4711  avgt  100    4.601 ± 0.054    4.589 ± 0.030   4.579 ± 0.023 ns/op
StringConcat.concatEmptyLeft                  4711  avgt  100    5.088 ± 0.031    5.109 ± 0.040   5.119 ± 0.059 ns/op
StringConcat.concatEmptyRight                 4711  avgt  100    4.896 ± 0.038    4.876 ± 0.059   4.844 ± 0.025 ns/op
StringConcat.concatMethodConstString          4711  avgt  100   11.561 ± 0.157   12.126 ± 0.148  11.580 ± 0.162 ns/op
StringConcat.concatMix4String                 4711  avgt  100  156.119 ± 0.675  154.814 ± 0.169 155.655 ± 1.741 ns/op
So I've dropped delegation and restored `String.getBytes(byte[], int, byte)`

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

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