Closed Bug 906781 Opened 6 years ago Closed 6 years ago

BaselineCompiler: Optimize JSOP_FUNAPPLY with Array argument

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: djvj, Assigned: djvj)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Assignee: general → kvijayan
Attached patch baseline-funapply-arrays.patch (obsolete) — Splinter Review
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).
Attachment #792613 - Flags: review?(bhackett1024)
Green on try (linux and linux64).  Will do full try run before pushing.
https://tbpl.mozilla.org/?tree=Try&rev=eb282bbecf17
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?
Attachment #792613 - Flags: review?(bhackett1024) → review+
Attachment #794692 - Flags: review?(jdemooij)
Attachment #792613 - Attachment is obsolete: true
New patch obsoletes old patch.  Makes suggested changes.  Sending for re-review since bhackett suggested it.
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(..), ..);
(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.
https://hg.mozilla.org/mozilla-central/rev/81fb29b23c8a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Flags: in-testsuite?
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..)
Attachment #799121 - Flags: review?(jdemooij)
Comment on attachment 799121 [details] [diff] [review]
testcase-bug-906781.patch

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

Nice :)
Attachment #799121 - Flags: review?(jdemooij) → review+
Attachment #794692 - Flags: review?(jdemooij) → review+
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.