Closed Bug 1297749 Opened 8 years ago Closed 8 years ago

Inline String.fromCodePoint

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Inlining String.fromCodePoint will make it possible for users to switch from String.fromCharCode to fromCodePoint without suffering a performance loss.
Attached patch inline_fromcodepoint.patch (obsolete) — Splinter Review
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+
Attached patch inline_fromcodepoint.patch (obsolete) — Splinter Review
Attachment #8784428 - Attachment is obsolete: true
Attachment #8784849 - Flags: review?(jdemooij)
(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).
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.
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)
Flags: needinfo?(jdemooij) → needinfo?(andrebargull)
Priority: -- → P5
(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)
Attached patch bug1297749.patchSplinter Review
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+
Keywords: checkin-needed
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: es6perf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: