Inline String.fromCodePoint

RESOLVED FIXED in Firefox 52

Status

()

P5
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks: 1 bug)

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
Inlining String.fromCodePoint will make it possible for users to switch from String.fromCharCode to fromCodePoint without suffering a performance loss.
(Assignee)

Comment 1

2 years ago
Created attachment 8784428 [details] [diff] [review]
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.
Attachment #8784428 - Flags: feedback?(jdemooij)
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.
Attachment #8784428 - Flags: feedback?(jdemooij) → feedback+
(Assignee)

Comment 3

2 years ago
Created attachment 8784849 [details] [diff] [review]
inline_fromcodepoint.patch
Attachment #8784428 - Attachment is obsolete: true
Attachment #8784849 - Flags: review?(jdemooij)
(Assignee)

Comment 4

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

Comment 5

2 years ago
I forgot to ask this: Do you think it makes sense to use range analysis information to avoid the bailout check?
Comment on attachment 8784849 [details] [diff] [review]
inline_fromcodepoint.patch

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

Thanks!
Attachment #8784849 - Flags: review?(jdemooij) → review+
(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.
(Assignee)

Comment 8

2 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);
```
Can we land this?
Flags: needinfo?(jdemooij)

Updated

2 years ago
Flags: needinfo?(jdemooij) → needinfo?(andrebargull)

Updated

2 years ago
Priority: -- → P5
(Assignee)

Comment 10

2 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.
Flags: needinfo?(andrebargull)
(Assignee)

Comment 11

2 years ago
Created attachment 8798068 [details] [diff] [review]
bug1297749.patch

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d55e89b2945
Attachment #8784849 - Attachment is obsolete: true
Attachment #8798068 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d07da2290d3e
Inline String.fromCodePoint in Ion. r=jandem
Keywords: checkin-needed
Blocks: 1307395

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d07da2290d3e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
status-firefox51: affected → wontfix
You need to log in before you can comment on or make changes to this bug.