Closed
Bug 1372081
Opened 7 years ago
Closed 7 years ago
Ion-inline FinishBoundFunctionInit
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file, 2 obsolete files)
29.24 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Ion-inlining FinishBoundFunctionInit improves this µ-benchmark from 110ms to 60ms for me: var bf = function f(){}; var t = Date.now(); for (var i = 0; i < 1000000; ++i) { bf.bind(); } print(Date.now() - t);
Assignee | ||
Comment 1•7 years ago
|
||
The usual disclaimer for any Ion patches from me: I don't really know what I'm doing, I'm simply copying bits and pieces I've seen elsewhere. :-) Open questions: - Do I need the policy for newly added MIR node? - Do I need to call setGuard() in MFinishBoundFunctionInit, because the OOL path can throw an exception/invoke user accessors? - Is "setResultType(MIRType::Value)" correct for known |undefined| return types? - I'm not sure all parts of the CodeGenerator changes are correct. (FWIW all jstests and jit-tests pass with this patch.) And I've moved the contents of intrinsic_FinishBoundFunctionInit to JSFunction::finishBoundFunctionInit, so we can more easily call it from the OOL path of the generated code.
Attachment #8876583 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•7 years ago
|
||
Duh, requesting a single register to write a Value to doesn't work in 32bit. Updated LFinishBoundFunctionInit to use BOX_PIECES, and CodeGenerator::visitFinishBoundFunctionInit() to use ToOutValue(lir). Otherwise no additional changes compared to the original patch. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6843b278a5fccfd5f8a346af54e86c3a1d5b34e6
Attachment #8876583 -
Attachment is obsolete: true
Attachment #8876583 -
Flags: review?(jdemooij)
Attachment #8876760 -
Flags: review?(jdemooij)
Comment 3•7 years ago
|
||
Comment on attachment 8876760 [details] [diff] [review] bug1372081.patch Review of attachment 8876760 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. This looks great, some minor suggestions below. ::: js/src/jit/CodeGenerator.cpp @@ +12556,5 @@ > + const size_t boundLengthOffset = FunctionExtended::offsetOfExtendedSlot(BOUND_FUN_LENGTH_SLOT); > + > + // Take the slow path if the target is not a JSFunction. > + masm.loadObjClass(target, outReg); > + masm.branchPtr(Assembler::NotEqual, outReg, ImmPtr(&JSFunction::class_), slowPath); Nit: can combine these 2 lines like this: masm.branchTestObjClass(Assembler::NotEqual, target, outReg, &JSFunction::class_, slowPath); (It's a little more efficient because it compares the clasp without loading it in a register.) ::: js/src/jit/MCallOptimize.cpp @@ +1688,5 @@ > + callInfo.setImplicitlyUsedUnchecked(); > + > + auto* ins = MFinishBoundFunctionInit::New(alloc(), boundFunction, targetFunction, argCount); > + current->add(ins); > + current->push(ins); This works but the result of this call is not used so this adds register pressure and instructions we could avoid. I'd MOZ_ASSERT(BytecodeIsPopped(pc)) and then instead of current->push(ins) do: pushConstant(UndefinedValue()); And make the following changes: * Remove setResultType in the MIR constructor. * Change BOX_PIECES to 0 in the LIR definition. * Change defineBox to add * Change codegen, you probably need a second temp then ::: js/src/jit/MIR.h @@ +13767,5 @@ > + : public MTernaryInstruction, > + public Mix3Policy<ObjectPolicy<0>, ObjectPolicy<1>, IntPolicy<2>>::Data > +{ > + MFinishBoundFunctionInit(MDefinition* bound, MDefinition* target, MDefinition* argCount) > + : MTernaryInstruction(bound, target, argCount) Uber nit: indent with 2 spaces less. ::: js/src/vm/SelfHosting.cpp @@ +438,5 @@ > { > CallArgs args = CallArgsFromVp(argc, vp); > MOZ_ASSERT(args.length() == 3); > MOZ_ASSERT(IsCallable(args[1])); > + MOZ_ASSERT(args[2].isInt32()); Hm is this guaranteed to be int32? Sometimes JITs may store integers as doubles.
Attachment #8876760 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #3) > ::: js/src/vm/SelfHosting.cpp > @@ +438,5 @@ > > { > > CallArgs args = CallArgsFromVp(argc, vp); > > MOZ_ASSERT(args.length() == 3); > > MOZ_ASSERT(IsCallable(args[1])); > > + MOZ_ASSERT(args[2].isInt32()); > > Hm is this guaranteed to be int32? Sometimes JITs may store integers as > doubles. `args[2]` holds the array length of the rest argument of FunctionBind. Is it still possible in that case that the JIT stores the array length as a double?
Assignee | ||
Comment 5•7 years ago
|
||
Updated patch per the review comments in comment #3.
Attachment #8876760 -
Attachment is obsolete: true
Attachment #8881531 -
Flags: review?(jdemooij)
Comment 6•7 years ago
|
||
Comment on attachment 8881531 [details] [diff] [review] bug1372081.patch Review of attachment 8881531 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks!
Attachment #8881531 -
Flags: review?(jdemooij) → review+
Comment 7•7 years ago
|
||
(In reply to André Bargull from comment #4) > `args[2]` holds the array length of the rest argument of FunctionBind. Is it > still possible in that case that the JIT stores the array length as a double? Maybe keep the isInt32 assert, yeah. It's hard to say offhand..
Assignee | ||
Comment 8•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cab9c7d94d21f175ec0b66dcab69cc9aea49b6f3
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d961ec8015a Ion-inline FinishBoundFunctionInit for normal and bound functions. r=jandem
Keywords: checkin-needed
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0d961ec8015a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•