1405994 - Get rid of arguments copy when entering JIT code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

When we enter Baseline or Ion from C++, we stack-allocate a Vector and if there are more formals than actuals, we copy the arguments to it and add |undefined| for missing arguments.

It's much nicer to pass the number of missing formals to the enterJIT trampoline and push |undefined| there. Then we can remove the Vector and make SetEnterJitData infallible.

Unfortunately EnterJIT is very platform dependent. At some point we should unify the implementations and pass EnterJitData* directly but that seemed overkill for this bug. This patch passes jit-tests on x86/x64/ARM/ARM64.

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8915522 [details] [diff] [review]
Patch

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

This patch sounds good to me, even after a careful review.
Still, I prefer to ask Sean for feedback on arm64 comments and conditions.

::: js/src/jit/arm/Trampoline-arm.cpp
@@ +101,5 @@
>  /*
>   * This method generates a trampoline for a c++ function with the following
>   * signature:
>   *   void enter(void* code, int argc, Value* argv, InterpreterFrame* fp, CalleeToken
>   *              calleeToken, JSObject* scopeChain, Value* vp)

nit: update the signature here.

@@ +179,5 @@
>      aasm->as_bic(r4, r4, Imm8(JitStackAlignment - 1));
>      // r4 is now the aligned on the bottom of the list of arguments.
>      static_assert(sizeof(JitFrameLayout) % JitStackAlignment == 0,
>        "No need to consider the JitFrameLayout for aligning the stack");
>      // sp' = ~(JitStackAlignment - 1) & (sp - argc * sizeof(Value)) - sizeof(JitFrameLayout)

nit: update this comment with "- numPaddedArgs * sizeof(Value)"

@@ +189,5 @@
> +    // If we're constructing, decrement r5: we will push new.target later.
> +    {
> +        Label noNewTarget;
> +        masm.branchTest32(Assembler::Zero, r9, Imm32(CalleeToken_FunctionConstructing),
> +                          &noNewTarget);

Note-for-later: This is wrong but it works. CalleeToken are not meant to be a bit-field but a value which is incremented.  Fortunately for us, we only have 0, 1 and 2 values, so this works fine in this particular case.

::: js/src/jit/arm64/Trampoline-arm64.cpp
@@ +181,5 @@
> +            masm.moveValue(UndefinedValue(), ValueOperand(r24));
> +            masm.Str(x24, MemOperand(tmp_sp, Operand(8), vixl::PostIndex));
> +
> +            // Set the condition codes for |cmp reg_numPadded, 2| (using the old value).
> +            masm.Subs(reg_numPadded, reg_numPadded, Operand(1));

Sean?

nit: I do not think the comment above this instruction is accurate.

   // Set the condition codes for |cmp reg_numPadded, 1| (using the old value)

Same thing for the previous loop.

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +111,4 @@
>      masm.shll(Imm32(3), r13);   // r13 = argc * sizeof(Value)
> +
> +    // r15 = numPaddedArgs * sizeof(Value)
> +    masm.movl(numPaddedArgs, r15);

nit: This is a size_t value, by pure correctness use either masm.mov, masm.movq or masm.movePtr.

Comment 2

[Sean Stangl [:sstangl]]

Comment on attachment 8915522 [details] [diff] [review]
Patch

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

r+ with change. Very happy to see these vectors reduced!

There's probably more we can do to further limit the amount of argument-copying around calls.

::: js/src/jit/arm64/Trampoline-arm64.cpp
@@ +184,5 @@
> +            // Set the condition codes for |cmp reg_numPadded, 2| (using the old value).
> +            masm.Subs(reg_numPadded, reg_numPadded, Operand(1));
> +
> +            // Branch if arguments remain.
> +            masm.B(&loopHead, vixl::Condition::gt);

SUBS sets the condition code from the new value, not the old value.

There's no need to open-code this logic, however -- it's preferable to use the helper function MacroAssembler::branchSub32(), which will be more obvious when used with Condition::NonZero, since it's standardized across all platforms.

Same thing for the previous loop.

Comment 3

[Jan de Mooij [:jandem]]

I just realized that instead of this we can use the arguments rectifier, if we remove ArgumentsRectifierReg (bug 1406340).

Then this becomes pretty trivial, the only annoying bit is the platform-dependent profiler code that has to be updated. bbouvier is also making changes to this code (bug 1347740), so I'll just wait for that to avoid bitrot.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Frame iteration changes

You get to review this since you changed all this code recently :)

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Use rectifier trampoline instead of a Vector

And now this is a much simpler patch.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8916605 [details] [diff] [review]
Part 1 - Frame iteration changes

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

Hey, I know this code!

Looks good to me, thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7774c8002d5f
part 1 - Handle rectifier frames following c++ entry frame in profiler code. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1fe0931730
part 2 - Use arguments rectifier when entering JIT code instead of a Vector. r=nbp

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7774c8002d5f
https://hg.mozilla.org/mozilla-central/rev/fd1fe0931730