8058779: Faster implementation of String.replace(CharSequence, CharSequence)

[Rémi Forax]

Hi Ivan,
<nitpicking mode="on"/>

Le 31 mai 2015 17:54:35 CEST, Ivan Gerasimov <ivan.gerasimov at oracle.com> a écrit :
>Hi Remi!
>
>On 31.05.2015 11:43, Remi Forax wrote:
>> I agree with the others the code is more readable.
>>
>> There is a slightly difference between the current behavior and the 
>> behavior of the proposed code,
>>   "foo".replace("bar", null)
>> should throw a NPE because replacement is null but the proposed code 
>> will return "foo"
>> because replacement.toString() is called after the short-circuit test
>
>> (j < 0).
>>
>Yes, you're right, thanks for catching it!
>And the regression test should have caught that, but I only had 
>"a".replace("a", null) there, which passed.
>
>I fixed the code and added the case to the regression test in the new 
>webrev.
>
>> so the first part of the code should be:
>>   String starget = target.toString();  // implict nullcheck
>>   String srepl = replacement.toString();  // implicit nullcheck
>>   int j = indexOf(starget);
>>   if (j < 0) {
>>     return this;
>>   }
>>   ...
>>
>> also 'i' can be initialized just before the 'do', there is no point
>to 
>> initialize it before.
>>
>> To finish, the two 'final' are useless here but i suppose it's a 
>> matter of style :)
>>
>I moved declaration of i just to save a line.  I don't think it 
>decreased performance.

I don't think so too.
It decrease readability, when you read the code of the loop you have to scroll up to find the initialization of 'i' like in plain old good C.

>
>Declaring the 'value' variable final was suggested by Martin, and I 
>think it is reasonable (see 
>http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/032601.html).
>The other variable is final just for symmetry :)

Either i read the code in an IDE and local variables and fields do not have the same color or i read diff and in that case fields are usually not visible. 
For me, final on local variable is just noise. But as i said it's a matter of taste.

>
>Sincerely yours,
>Ivan

cheers,
Rémi 

>
>
>> cheers,
>> Rémi
>>
>>
>> On 05/31/2015 04:19 AM, Ivan Gerasimov wrote:
>>> Hi everyone!
>>>
>>> Here's another webrev, in which replace() is implemented with 
>>> StringBuilder.
>>> On my benchmark it is almost as fast as the version backed with 
>>> arrays, but this variant is much shorter.
>>>
>>> Credits to Sherman for combining the general algorithm with the case
>
>>> of empty target.
>>>
>>> Comments, further suggestions are welcome!
>>>
>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058779
>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8058779/04/webrev/
>>>
>>> Sincerely yours,
>>> Ivan
>>>
>>
>>
>>