Closed
Bug 1372081
Opened 8 years ago
Closed 8 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•8 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•8 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•8 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•8 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•8 years ago
|
||
Updated patch per the review comments in comment #3.
Attachment #8876760 -
Attachment is obsolete: true
Attachment #8881531 -
Flags: review?(jdemooij)
Comment 6•8 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•8 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•8 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 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
•