[NEW BUG]: Avoid allocations in String.replace(CharSequence, CharSequence) in case no replacement happened
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
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.
More information about the core-libs-dev