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

[Ivan Gerasimov]

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

-- 
With kind regards,
Ivan Gerasimov