André Bargull [:anba] Assignee
	Description • 8 years ago

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

	André Bargull [:anba] Assignee
	Comment 1 • 8 years ago

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.

	Jan de Mooij [:jandem]
	Comment 2 • 8 years ago

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.

	André Bargull [:anba] Assignee
	Comment 4 • 8 years ago

(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).

	André Bargull [:anba] Assignee
	Comment 5 • 8 years ago

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

	Jan de Mooij [:jandem]
	Comment 6 • 8 years ago

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

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

Thanks!

	Jan de Mooij [:jandem]
	Comment 7 • 8 years ago

(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.

	André Bargull [:anba] Assignee
	Comment 8 • 8 years ago

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);
```

	Tom S [:evilpie]
	Comment 9 • 8 years ago

Can we land this?

	André Bargull [:anba] Assignee
	Comment 10 • 8 years ago

(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.

	André Bargull [:anba] Assignee
	Comment 11 • 8 years ago

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

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

	Pulsebot
	Comment 12 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d07da2290d3e
Inline String.fromCodePoint in Ion. r=jandem

	Phil Ringnalda (:philor)
	Comment 13 • 8 years ago
bugherder

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

	Gerry Chang [:gchang]
	Comment 14 • 8 years ago

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