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

[Ivan Gerasimov]

On 31.05.2015 18:54, Ivan Gerasimov wrote:
> 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.
>
Which is right here:
http://cr.openjdk.java.net/~igerasim/8058779/05/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.
>
> 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 :)
>
> Sincerely yours,
> Ivan
>
>
>> 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
>>>
>>
>>
>>
>
>
>