Hello!
Great optimization!
+ // overflow-conscious code
+ int resultLen = valLen + (++p) * deltaLen;
+ if (Integer.MAX_VALUE / p < deltaLen ||
+ Integer.MAX_VALUE - resultLen < 0) {
+ throw new OutOfMemoryError();
+ }
Is it really necessary to introduce unconditional division by unknown
divisor in the common path? Probably this would be better (common path
should be intrinsified)?
int resultLen;
try {
resultLen = Math.addExact(valLen, Math.multiplyExact(++p, deltaLen));
}
catch(ArithmeticException ignored) {
throw new OutOfMemoryError();
}
Or alternatively
long resultLenLong = ((long)(++p)) * deltaLen + valLen; // long never
overflows here
int resultLen = (int)resultLenLong;
if (resultLenLong != resultLen) {
throw new OutOfMemoryError();
}
With best regards,
Tagir Valeev.
On Thu, Apr 25, 2019 at 1:01 PM Ivan Gerasimov
<ivan.gerasi...@oracle.com> wrote:
>
> Hello!
>
> This enhancement was inspired by a recent discussion at
> compiler-...@openjdk.java.net.
>
> It seems to be a non-uncommon situation when String.replace(CS, CS) is
> called with this and both arguments being Latin1 strings, so it seems
> reasonable to optimize for such case.
>
> Here are the fresh benchmark results (see the webrev for the source of
> the benchmark):
> -- prior the fix:
> Benchmark Mode Cnt Score Error Units
> StringReplace.replace1_0 avgt 24 70.860 ± 5.239 ns/op
> StringReplace.replace1_1 avgt 24 82.661 ± 1.007 ns/op
> StringReplace.replace1_2 avgt 24 97.251 ± 1.186 ns/op
>
> -- after the fix:
> Benchmark Mode Cnt Score Error Units
> StringReplace.replace1_0 avgt 24 52.855 ± 0.982 ns/op
> StringReplace.replace1_1 avgt 24 23.849 ± 0.066 ns/op
> StringReplace.replace1_2 avgt 24 62.266 ± 0.552 ns/op
>
> So the speedup was x1.3, x3.4, x1.5.
>
> Would you please help review the fix?
>
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8222955
> WEBREV: http://cr.openjdk.java.net/~igerasim/8222955/00/webrev/
>
> Mach5 job is in progress and looks green so far (a few test groups are
> left).
>
> Thanks in advance!
>
> --
> With kind regards,
> Ivan Gerasimov
>
