Thank you Tagir for suggestion!
On 4/26/19 1:24 AM, Tagir Valeev wrote:
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();
}
Personally, I like it better to see plain 32 arithmetic (assuming it is
correct, of course), but I agree that using Math.xxxExact() here
improves readability.
Moreover, it turns out to give measurable performance improvement!
Shortly, I'll send the updated webrev with this suggestion included and
with some more additional work.
Thanks again,
Ivan
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
--
With kind regards,
Ivan Gerasimov
