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

[Ivan Gerasimov]

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.

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