RFR 8222955 : Optimize String.replace(CharSequence, CharSequence) for Latin1 encoded strings

[Tagir Valeev]

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.gerasimov at oracle.com> wrote:
>
> Hello!
>
> This enhancement was inspired by a recent discussion at
> compiler-dev at 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
>