Closed Bug 1372081 Opened 8 years ago Closed 8 years ago

Ion-inline FinishBoundFunctionInit

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 2 obsolete files)

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);
Attached patch bug1372081.patch (obsolete) — Splinter Review
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)
Attached patch bug1372081.patch (obsolete) — Splinter Review
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 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)
(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?
Attached patch bug1372081.patchSplinter Review
Updated patch per the review comments in comment #3.
Attachment #8876760 - Attachment is obsolete: true
Attachment #8881531 - Flags: review?(jdemooij)
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+
(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..
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: