[NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened

[Vitaly Davidovich]

On Thu, Mar 2, 2017 at 8:26 AM, Jonathan Bluett-Duncan <
jbluettduncan at gmail.com> wrote:

> Hi Christoph and Vitaly,
>
> I am struggling to understand why the null-check for `replacement` has
> been moved within the `if (j < 0) { ... }` block. Could one of you explain
> to me the reasoning behind this change?
>
> I ask because, AFAICT, making it so that it only throws an NPE if
> `replacement` is null *and* `j < 0`, rather than throwing NPE at the very
> start of the method, seems to be going against the class-level doc which
> says, "Unless otherwise noted, passing a null argument to a constructor or
> method in this class will cause a NullPointerException to be thrown."
>
If there's no early bail out, there's an implicit null check when
replacement.toString() is called; in fact, if a null is never seen (based
on profile), there won't be any code generated for the null check (sigsegv
handling will be used instead).  If we add an explicit null check at the
top of the method, I'm not confident that the JIT will eliminate it in
cases where the method does not bail out early.  It probably doesn't
matter, really, but I don't see much harm in delaying the explicit null
check until the last moment, so to speak.

>
> Other than that, I am equally happy with `replacement` itself having an
> explicit-nullness-related comment instead of target having an
> implicit-nullness comment - it seems to deliver the same sort of message. :)
>
> Kind regards,
> Jonathan
>
> On 2 March 2017 at 07:26, Christoph Dreis <christoph.dreis at freenet.de>
> wrote:
>
>> Hey Vitaly and Jonathan,
>>
>> > As mentioned offline, I would move the null check right before "return
>> this".
>>
>> @Vitaly: Thanks again. Adjusted.
>> @Jonathan: Thanks. Thought about that as well, but I'd probably rather go
>> with explaining the explicit nullcheck.
>>
>> See the adjusted patch below. What do you think?
>>
>> ===== PATCH ======
>> diff --git a/src/java.base/share/classes/java/lang/String.java
>> b/src/java.base/share/classes/java/lang/String.java
>> --- a/src/java.base/share/classes/java/lang/String.java
>> +++ b/src/java.base/share/classes/java/lang/String.java
>> @@ -2177,11 +2177,13 @@
>>       */
>>      public String replace(CharSequence target, CharSequence replacement)
>> {
>>          String tgtStr = target.toString();
>> -        String replStr = replacement.toString();
>>          int j = indexOf(tgtStr);
>>          if (j < 0) {
>> +            // Explicit nullcheck of replacement to fulfill NPE contract
>> in early out
>> +            Objects.requireNonNull(replacement);
>>              return this;
>>          }
>> +        String replStr = replacement.toString();
>>          int tgtLen = tgtStr.length();
>>          int tgtLen1 = Math.max(tgtLen, 1);
>>          int thisLen = length();
>>
>>
>