1297749 - Inline String.fromCodePoint

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[André Bargull [:anba]]

Inlining String.fromCodePoint will make it possible for users to switch from String.fromCharCode to fromCodePoint without suffering a performance loss.

Comment 1

[André Bargull [:anba]]

Attachment: inline_fromcodepoint.patch

The current patch is based on the existing MFromCharCode class and also on MSimdGeneralShuffle for the range checking. I don't know if this approach is correct at all, so it'd be great to get your feedback on this change.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8784428 [details] [diff] [review]
inline_fromcodepoint.patch

Review of attachment 8784428 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks! Just some nits and a question, after that I can r+ this.

::: js/src/jit/CodeGenerator.cpp
@@ +7690,5 @@
> +    OutOfLineCode* ool = oolCallVM(StringFromCodePointInfo, lir, ArgList(codePoint),
> +                                   StoreRegisterTo(output));
> +
> +    // Bailout if input is not a valid code point.
> +    bailoutCmp32(Assembler::Above, codePoint, Imm32(unicode::NonBMPMax), snapshot);

Are you using a bailout (instead of letting the OOL path throw an exception) because the MIR instruction is movable and so there could be observable differences in behavior if we throw on the OOL path (for instance, when this is hoisted out of a loop we never enter)? That's worth documenting here.

::: js/src/jit/MCallOptimize.cpp
@@ +1729,5 @@
> +
> +    callInfo.setImplicitlyUsedUnchecked();
> +
> +    MToInt32* codePoint = MToInt32::New(alloc(), callInfo.getArg(0));
> +    current->add(codePoint);

Hm this is a no-op; we already ensured the first argument has type MIRType::Int32. The same applies to inlineStrFromCharCode, can you remove this from both and just use callInfo.getArg(0) directly?

::: js/src/jit/VMFunctions.cpp
@@ +418,5 @@
> +    if (!str_fromCodePoint_one_arg(cx, rval, &rval))
> +        return nullptr;
> +
> +    MOZ_ASSERT(rval.isString());
> +    return rval.toString();

Nit: we usually don't add these asserts and rely on toString() doing the necessary checks.

Comment 3

[André Bargull [:anba]]

Attachment: inline_fromcodepoint.patch

Comment 4

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> ::: js/src/jit/CodeGenerator.cpp
> @@ +7690,5 @@
> > +    OutOfLineCode* ool = oolCallVM(StringFromCodePointInfo, lir, ArgList(codePoint),
> > +                                   StoreRegisterTo(output));
> > +
> > +    // Bailout if input is not a valid code point.
> > +    bailoutCmp32(Assembler::Above, codePoint, Imm32(unicode::NonBMPMax), snapshot);
> 
> Are you using a bailout (instead of letting the OOL path throw an exception)
> because the MIR instruction is movable and so there could be observable
> differences in behavior if we throw on the OOL path (for instance, when this
> is hoisted out of a loop we never enter)? That's worth documenting here.
> 

Yes, that's the reason. In my first attempt I didn't use the extra bailout, so when I tested my changes I wondered what will happen when fromCodePoint, when called with an invalid code point, gets hoisted out of a loop. Obviously I got the wrong results, that means fromCodePoint threw an exception before the loop was entered. And that's observable when another instruction in the loop has side-effects (see the jit-test from the patch for such an example).

Comment 5

[André Bargull [:anba]]

I forgot to ask this: Do you think it makes sense to use range analysis information to avoid the bailout check?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8784849 [details] [diff] [review]
inline_fromcodepoint.patch

Review of attachment 8784849 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 7

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #5)
> I forgot to ask this: Do you think it makes sense to use range analysis
> information to avoid the bailout check?

It does make sense, though I doubt eliminating this branch matters much. It could be done as follow-up bug/patch, maybe with some micro-benchmarking, if you're interested.

Comment 8

[André Bargull [:anba]]

I wonder what will be a good micro-benchmark for that change, because I don't think simple tests like the following one are really convincing.

```
// before: 70ms, after: 4ms (=> call to fromCodePoint removed)

var d = dateNow();
for (var i = 0; i < 1000000; ++i) String.fromCodePoint((i >> 20) ? (i & 0x10ffff) : (i & 0xfffff));
print(dateNow() - d);
```

Comment 9

[Tom S]

Can we land this?

Comment 10

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #9)
> Can we land this?

I'm currently in the process of bit-unrotting my patches and pushing them to Try. As soon as that's finished, I'll add the check-in needed flag for my bugs.

Comment 11

[André Bargull [:anba]]

Attachment: bug1297749.patch

Patch needed bit-unrotting, carrying r+ from jandem.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d07da2290d3e
Inline String.fromCodePoint in Ion. r=jandem

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d07da2290d3e

Comment 14

[Gerry Chang [:gchang]]

Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.