Ion-inline FinishBoundFunctionInit

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
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

a year ago
Created attachment 8876583 [details] [diff] [review]
bug1372081.patch

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

a year ago
Created attachment 8876760 [details] [diff] [review]
bug1372081.patch

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

Comment 4

a year 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

a year ago
Created attachment 8881531 [details] [diff] [review]
bug1372081.patch

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..

Comment 9

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0d961ec8015a
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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.