906781 - BaselineCompiler: Optimize JSOP_FUNAPPLY with Array argument

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

This hits fallback heavily on jQuery.trigger.  In general it shows up with other DOM code as well.  There are many situations where Array.prototype.splice.call(arguments, X) is used to make a dense Array that is then passed to a funapply.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: baseline-funapply-arrays.patch

Adds a baseline IC stub for handling JSOP_FUNAPPLY where the second argument is a completely dense array (e.g. length == initializedLength, and no holes).

Comment 2

[Kannan Vijayan [:djvj]]

Green on try (linux and linux64).  Will do full try run before pushing.
https://tbpl.mozilla.org/?tree=Try&rev=eb282bbecf17

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 792613 [details] [diff] [review]
baseline-funapply-arrays.patch

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

I doubt the automated tests we have will stress this very heavily.  Can you add some jit-tests for the basic functionality and corner cases like holes in the array or length != initializedLength?

Also, this looks good but I'm not really qualified to review the assembly around the call paths.  Maybe jandem could have a look at that part?  I'm mildly worried about all the new asm and it would be nice if that stuff could be commoned into helper functions or something.

::: js/src/jit/BaselineIC.cpp
@@ +3826,5 @@
>      Address initLength(scratchReg, ObjectElements::offsetOfInitializedLength());
>      masm.branch32(Assembler::BelowOrEqual, initLength, key, &failure);
>  
>      // Hole check and load value.
> +    JS_ASSERT(sizeof(Value) == 8);

this can be a static assert

@@ +6845,5 @@
>      if (!thisv.isObject() || !thisv.toObject().is<JSFunction>())
>          return true;
>      RootedFunction target(cx, &thisv.toObject().as<JSFunction>());
>  
> +    bool isScripted = target->hasScript() &&

hasJITCode?

@@ +7158,5 @@
> +
> +        masm.branchTestObjClass(Assembler::NotEqual, secondArgObj, regsx.getAny(),
> +                                &ArrayObject::class_, failure);
> +
> +        // Get the array elementsand ensure that initializedLength == length

typo

::: js/src/jit/BaselineIC.h
@@ +5149,5 @@
> +{
> +    friend class ICStubSpace;
> +  public:
> +    // The maximum length of an inlineable funcall array.
> +    // Keep this small to avoid controllable stack overflows by peopling passing large

peopling?

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: baseline-funapply-arrays.patch

Comment 5

[Kannan Vijayan [:djvj]]

New patch obsoletes old patch.  Makes suggested changes.  Sending for re-review since bhackett suggested it.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 794692 [details] [diff] [review]
baseline-funapply-arrays.patch

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

Looks good. How much does this win on jQuery?

I agree with bhackett though that this needs jit-tests that pass at least the following objects to the stub: empty array, array with holes, very large array, array with length != initLength, array with getters/setters, non-array object etc to make sure we correctly handle these cases. That should also give decoder's fuzzer something to mutate.

::: js/src/jit/BaselineIC.cpp
@@ +7132,5 @@
> +        masm.branchTest32(Assembler::NonZero,
> +                          Address(BaselineFrameReg, BaselineFrame::reverseOffsetOfFlags()),
> +                          Imm32(BaselineFrame::HAS_ARGS_OBJ),
> +                          failure);
> +    } else {

Nit: JS_ASSERT(applyThing == FunApply_Array);

@@ +7177,5 @@
> +        Label endLoop;
> +        masm.bind(&loop);
> +        masm.branchPtr(Assembler::AboveOrEqual, start, end, &endLoop);
> +        masm.branchTestMagic(Assembler::Equal, Address(start, 0), failure);
> +        masm.addPtr(ImmWord(sizeof(Value)), start);

Nit: masm.addPtr(Imm32(..), ..);

Comment 7

[Kannan Vijayan [:djvj]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Looks good. How much does this win on jQuery?

Overall very little, but jQuery.trigger moves from 0.13 to 0.37 or so with this, about a 3x improvement.

Comment 8

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81fb29b23c8a

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/81fb29b23c8a

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: testcase-bug-906781.patch

Test case for patch.  There are 6 array variants: empty, dense, sparse, large, non-array, and indexed-getter.

The script exercises baseline FUNAPPLY chains for all two-choice variants of the above set (e.g. empty+empty, empty+dense, empty+sparse, sparse+dense, sparse+sparse, etc..)

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 799121 [details] [diff] [review]
testcase-bug-906781.patch

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

Nice :)

Comment 12

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3ed33fd5f2

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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