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

Vitaly Davidovich vitalyd at gmail.com
Thu Mar 2 17:16:04 UTC 2017

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

>     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

More information about the core-libs-dev mailing list