1372081 - Ion-inline FinishBoundFunctionInit

Status: RESOLVED FIXED

Description

[André Bargull [:anba]]

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);

Comment 1

[André Bargull [:anba]]

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.

Comment 2

[André Bargull [:anba]]

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

Comment 3

[Jan de Mooij [:jandem]]

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.

Comment 4

[André Bargull [:anba]]

(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?

Comment 5

[André Bargull [:anba]]

Created attachment 8881531 [details] [diff] [review]
bug1372081.patch

Updated patch per the review comments in comment #3.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8881531 [details] [diff] [review]
bug1372081.patch

Review of attachment 8881531 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks!

Comment 7

[Jan de Mooij [:jandem]]

(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 8

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cab9c7d94d21f175ec0b66dcab69cc9aea49b6f3

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d961ec8015a
Ion-inline FinishBoundFunctionInit for normal and bound functions. r=jandem

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d961ec8015a