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

[Vitaly Davidovich]

On Thu, Mar 2, 2017 at 11:42 AM, Ulf Zibis <Ulf.Zibis at cosoco.de> wrote:

>
> Am 02.03.2017 um 14:55 schrieb Vitaly Davidovich:
>
>> Implicit null checks ... generally don't cost anything.
>>
> Any additional byte of code in CPU cache prevents other code from staying
> there.

When the optimization applies, there shouldn't be *any* instructions for
the null check on the path - it's done via signal handling.

>
>
>
>>     I can't imagine, if any programmer would rely on such contract,
>> instead of doing the check
>>     explicitly if of significance.
>>
>> I think it's to have consistent behavior, irrespective of control flow.
>> It can detect bugs early on.
>>
> Yes, serving bug detection almost always has some cost and should be
> pondered, but the issue of this patch is to increase performance. Here I
> suspect the real value of the bug detection help.

Performance by way of removing an allocation.  There's already code in that
method that's much more expensive than a test against a register.  So while
I suggested to move the explicit null check into the if block, I don't
think it'll really matter.  But removing an allocation is always a good
thing.

>
>
>     I think, the original reasonable of the contract is to rule out any
>> other exception type or
>>     undefined behaviour in case of relevance of the argument. So I would
>> vote for stating the contract
>>     more precisely in that way.
>>
>>     Also, if I remember correct, if a variable, here replStr, is only
>> referenced later, the
>>     compiler can
>>     move the execution of here replacement.toString() to a later
>> position, so it is not clear to
>>     me, if
>>     the original implementation ever fulfilled the contract.
>>
>> Compiler can only do that if it proves a bunch of things, such as no
>> observable side effects as a result of sinking it down.  If
>> CharSequence.toString isn't devirtualized there, eg, it definitely cannot
>> do it.  I wouldn't bet on it.
>>
> For the original code in case of inlining, I guess, the optimizer would
> place the null check before the if block and the allocation of the
> replacement String after. For the new code, the optimizer can detect the
> duplicate null check inside and outside the if block, so can move it before
> the if block. So both java codes could result in the same machine code.
> IMHO the effective optimization should be proved by HS disassembler.
>
This is Sufficiently Smart Compiler territory.  It's much better to
hand-code this properly, particularly since nothing is lost in readability
or maintainability in this case.

>
> There is also the possibility, that an allocation of an intermediate
> String object is never done, because the anyway involved StringBuilder
> perhaps can insert the replacement sequence without before instantiating a
> String object.

I know C2 can remove some intermediate string allocations, but I would be
very impressed if it handled this particular method in the way you describe.

>
>
> -Ulf
>
>