735402 - IonMonkey: Optimize JSOP_FUNAPPLY

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

We want to optimize:

   f.apply(x, y)

Into a call to |f| where |this| is |x| and the arguments are unpacked from |y| (the cases we care about are y being a dense array or arguments object).

I'm not sure if it's worth inlining this one, or if JM+TI does. If |y| is a dense array we should call to a lightweight trampoline that unpacks its elements onto the stack. If |y| is the arguments object (we have to be careful not to require reifing it) we just have to copy our local arguments.

Comment 1

[Nicolas B. Pierron [:nbp]]

Implementing fun.apply will improve the performance of v8-Raytrace, which is calling fun.apply 66594 times at each cycle of the test.  At every call, the size of the argument vector correspond to the number of formal argument expected by the function.

at each cycle,
<args> <times>
0 11913
1     1
2  3331
3 51346
5     2
6     1

No v8 benchmarks are calling fun.apply with an array.

For the moment, it sounds reasonable to bailout in case of underflow.  This means that we do not have to produce a rectifier frame if the number of actual arguments is not matching.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Inline fun.apply when used with lazy arguments (x64 only)

This patch add support for fun.apply on x64.
v8 Raytrace goes from 750 VAP to 1100 VAP. (with --no-jm)

ARM and x86 will probably need support for stack alignment.

Comment 3

[Nicolas B. Pierron [:nbp]]

Add a dependency on Bug 765480 since it is needed to avoid some error in the test suite.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] from comment #2)
> Created attachment 635739 [details] [diff] [review]
> Inline fun.apply when used with lazy arguments (x64 only)

This patch add support to fun.apply by creating a new way to make a JS function call.
We need a new way to handle the function call because the number of arguments is only known at runtime and is not known at compile time.

As the number of argument is only known at runtime, we are facing multiple limitations:
- We cannot bailout safely while unexpected arguments are on the stack.
- We cannot have one instruction to push each argument.

The current patch handles fun.apply(o, arguments).

The option chosen in this patch is to copy the argument vector in emitPushArguments and undo its effects in emitPopArguments.

The stack should be walkable/unwind during/after the call which means that the number of effective arguments should be recovered from somewhere.  So we are doing the same thing as the rectifier frame, we dynamically creating a frame descriptor which contains the number of arguments and recover the compile-time framePushed by unwinding dynamically copied arguments.

We handle the bailout case identically as bailed rectifier frames by storing into the parent frame type the fact that the current frame was a JS frame instead of an exit frame to avoid the math on the frame descriptor which cause bad recoveries.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Inline fun.apply when used with lazy arguments (x64/x86)

Same concepts, ported to x86.

The current perfs are: master / patched (--no-jm) / --no-ion
x86: 800 / 1800 / 3700
x64: 750 / 1750 / 3150

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 638784 [details] [diff] [review]
Inline fun.apply when used with lazy arguments (x64/x86)

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

Sorry for the late review. r=me with the LabelWithStack and argc comments addressed.

::: js/src/ion/Bailouts.cpp
@@ +346,5 @@
> +        frame->changePrevType(IonFrame_Bailed_JS);
> +        return;
> +    }
> +
> +    JS_NOT_REACHED("invalid");

Instead, above, can you assert that frame->prevType() == IonFrame_JS?

::: js/src/ion/CodeGenerator.cpp
@@ +835,5 @@
> +    // Join with all arguments copied and the extra stack usage computed.
> +    masm.bind(&end);
> +
> +    // Push |this|.
> +    masm.Push(ToValue(apply, LApplyArgsGeneric::ThisIndex));

As discussed on IRC, here and when we push |this|, we should just sneak around the stack tracking (a feature I've personally come to find kind of annoying...) - hopefully that's enough to make LabelWithStack unnecessary.

@@ +872,5 @@
> +
> +    // Copy the arguments of the current function.
> +    emitPushArguments(apply, copyreg);
> +
> +    masm.checkStackAlignment();

Check with Marty; I think had to remove this from the normal invoke path.

@@ +924,5 @@
> +        if (!apply->hasSingleTarget()) {
> +            // Check whether the provided arguments satisfy target argc.
> +            masm.load16ZeroExtend(Address(calleereg, offsetof(JSFunction, nargs)), copyreg);
> +            masm.cmp32(copyreg, argcreg);
> +            masm.j(Assembler::Above, &thunk);

Since we don't know argc at compile time, it seems like we couldn't have fixed up arguments for a known callee. So the check above and below for hasSingleTarget should go away. (I think, also, Assembler::Above has to be Assembler::NotEqual.)

Anyway, we should definitely add tests for underflow/overflow of the single target case.

@@ +952,5 @@
> +        }
> +
> +        masm.bind(&rejoin);
> +
> +        // masm.checkStackAlignment();

nit: Remove dead code.

@@ +960,5 @@
> +        if (!markSafepoint(apply))
> +            return false;
> +
> +        // Recover the number of arguments from the frame descriptor.
> +        masm.movePtr(Address(StackPointer, 0), copyreg);

Somewhere, should we JS_STATIC_ASSERT that copyreg != JS return regs?

::: js/src/ion/Ion.cpp
@@ +1372,5 @@
>  int js::ion::LabelBase::id_count = 0;
> +
> +void ion::disbaleIonScript(JSScript *script)
> +{
> +    IonSpew(IonSpew_Abort, "Disable Ion of script %s:%d", script->filename, script->lineno);

nit: "Disabling Ion compilation of script"

::: js/src/ion/Ion.h
@@ +228,5 @@
>  {
>      return cx->hasRunOption(JSOPTION_ION) && cx->typeInferenceEnabled();
>  }
>  
> +void disbaleIonScript(JSScript *script);

nit: rename this to ForbidCompilation

::: js/src/ion/IonBuilder.cpp
@@ +3181,5 @@
> +        return makeCall(native, argc, false);
> +
> +    // Reject when called with an Array or object.
> +    MPassArg *passVp = current->peek(-1)->toPassArg();
> +    if (passVp->getArgument()->type() != MIRType_ArgObj)

It would be better to use TypeInference if possible - that or, add a comment saying that the following pattern is unlikely so we don't care about it:

var x = arguments;
while (...) {
    f.apply(this, x);

(Sniffing MIR is generally discouraged because phis will make it look like no information is available.)

::: js/src/ion/IonFrameIterator.h
@@ +201,5 @@
>      MachineState machineState() const;
> +
> +#ifdef DEBUG
> +    void dump() const;
> +#endif

nit: Can you remove the #ifdef DEBUG here (and below)?

::: js/src/ion/Lowering.cpp
@@ +290,5 @@
> +LIRGenerator::visitApplyArgs(MApplyArgs *apply)
> +{
> +    JS_ASSERT(CallTempReg0 != CallTempReg1);
> +    JS_ASSERT(CallTempReg0 != ArgumentsRectifierReg);
> +    JS_ASSERT(CallTempReg1 != ArgumentsRectifierReg);

nit: Can make these JS_STATIC_ASSERT?

@@ +294,5 @@
> +    JS_ASSERT(CallTempReg1 != ArgumentsRectifierReg);
> +    JS_ASSERT(apply->getFunction()->type() == MIRType_Object);
> +
> +    // Height of the current argument vector.
> +    //uint32 argslot = getArgumentSlotForCall();

nit: remove dead code

@@ +300,5 @@
> +
> +    if (target && target->isNative()) {
> +        IonSpew(IonSpew_Abort, "native.apply(., arguments) is not supported yet.");
> +        return false;
> +        // JS_NOT_REACHED("track un-implemented versions");

Is this case necessary? If we support the !target case, then treating target->isNative the same should work. Otherwise, could we treat this as a dead-end, and unindent the code in the else case?

::: js/src/ion/MIR.h
@@ +1200,5 @@
>  };
>  
> +// fun.apply(self, arguments)
> +class MApplyArgs
> +  : public MAryInstruction<3>,

You can use MTernaryInstruction here, and not have to initOperand()

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #6)
> ::: js/src/ion/CodeGenerator.cpp
> @@ +835,5 @@
> > +    // Join with all arguments copied and the extra stack usage computed.
> > +    masm.bind(&end);
> > +
> > +    // Push |this|.
> > +    masm.Push(ToValue(apply, LApplyArgsGeneric::ThisIndex));
> 
> As discussed on IRC, here and when we push |this|, we should just sneak
> around the stack tracking (a feature I've personally come to find kind of
> annoying...) - hopefully that's enough to make LabelWithStack unnecessary.

Now I am using masm.pushValue instead and as expected, we can remove the LabelWithStack.

> @@ +872,5 @@
> > +
> > +    // Copy the arguments of the current function.
> > +    emitPushArguments(apply, copyreg);
> > +
> > +    masm.checkStackAlignment();
> 
> Check with Marty; I think had to remove this from the normal invoke path.

I checked LCallGeneric, and the checkStackAlignment is still here, on the other hand it does not matter much because it is garantee because we are only pushing Values, which means that the stack would stay aligned for ARM.

> @@ +924,5 @@
> > +        if (!apply->hasSingleTarget()) {
> > +            // Check whether the provided arguments satisfy target argc.
> > +            masm.load16ZeroExtend(Address(calleereg, offsetof(JSFunction, nargs)), copyreg);
> > +            masm.cmp32(copyreg, argcreg);
> > +            masm.j(Assembler::Above, &thunk);
> 
> Since we don't know argc at compile time, it seems like we couldn't have
> fixed up arguments for a known callee. So the check above and below for
> hasSingleTarget should go away.

We can still specialized the check by inlining the constant when we have a known target.

> I think, also, Assembler::Above has to be Assembler::NotEqual.

I renamed the Label "thunk" to "underflow". The way the stack is represented we do not care about the overflow, only about the underflow.  We need to care about the underflow because the Ion frames does not check for existing arguments and assume is has at least the expected number of arguments.  So in case of underflow we need to produce a rectifier frame.

In case of overflow, the Ion frame will find all the formal arguments above the current frame and it would not cause any error.  The issue is finding the actual vector of arguments, the current implementation assume that the original vector of argument just above the current frame is the vector of actual arguments, except for the formal arguments.  Using a rectifier frame with exactly the number of formal argument expected by the Ion frame is unnecessary and would increase the complexity to recover any actual argument — because we will need to skip the potential rectifier frame.

> Anyway, we should definitely add tests for underflow/overflow of the single
> target case.

As the overflow case is handle seamlessly, we only need to care about underflow to create the rectifier frame.

> ::: js/src/ion/IonBuilder.cpp
> @@ +3181,5 @@
> > +        return makeCall(native, argc, false);
> > +
> > +    // Reject when called with an Array or object.
> > +    MPassArg *passVp = current->peek(-1)->toPassArg();
> > +    if (passVp->getArgument()->type() != MIRType_ArgObj)
> 
> It would be better to use TypeInference if possible - that or, add a comment
> saying that the following pattern is unlikely so we don't care about it:
> 
> var x = arguments;
> while (...) {
>     f.apply(this, x);

Indeed, this is not frequent but still desirable.

> @@ +300,5 @@
> > +
> > +    if (target && target->isNative()) {
> > +        IonSpew(IonSpew_Abort, "native.apply(., arguments) is not supported yet.");
> > +        return false;
> > +        // JS_NOT_REACHED("track un-implemented versions");
> 
> Is this case necessary? If we support the !target case, then treating
> target->isNative the same should work. Otherwise, could we treat this as a
> dead-end, and unindent the code in the else case?

I will add a test case for it:

function max() {
  return Math.max.apply(null, arguments);
}

> ::: js/src/ion/MIR.h
> @@ +1200,5 @@
> >  };
> >  
> > +// fun.apply(self, arguments)
> > +class MApplyArgs
> > +  : public MAryInstruction<3>,
> 
> You can use MTernaryInstruction here, and not have to initOperand()

If I use MTernaryInstruction, the I need to overload congruentTo to return false, so this does not sounds like a big deal.

Comment 8

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/0c60ad6d78ce
https://hg.mozilla.org/projects/ionmonkey/rev/b3c75238a267 (Fix)