Closed Bug 1100513 Opened 10 years ago Closed 10 years ago

Inline |new Array(x)|

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(Keywords: perf)

Attachments

(1 file)

Ion currently inlines |new Array(int32-constant)|, but we should also optimize the non-constant case.

Ember (bug 1097376) has a |new Array(arguments.length)| in some really hot code.
Keywords: perf
Attached patch PatchSplinter Review
Tries to do the allocation inline if the length <= the template object's length. This is probably true in a lot of cases; the Ember benchmark I was looking at creates many arrays with length == 1 at the same site.

If the length > the template object's length we use an OOL callVM, this is still at least 2x faster than what we were doing because it avoids the GetTypeCallerInitObject call in js_Array.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8524664 - Flags: review?(bhackett1024)
Comment on attachment 8524664 [details] [diff] [review]
Patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +4350,5 @@
> +    // for the length in lengthReg.
> +    if (!templateObject->hasSingletonType() && templateObject->length() <= inlineLength)
> +        masm.branch32(Assembler::Above, lengthReg, Imm32(templateObject->length()), ool->entry());
> +    else
> +        masm.jump(ool->entry());

This looks fine, but why not just always make a copy of the template object?  If the passed in length is greater than the template object's capacity we'll end up with an array which hasn't been fully allocated, but it's fine for new Array(n) to do that.  Additionally, masm.createGCObject could be modified so that if it is allocating a nursery array it can dynamically size the elements according to the passed in length, since those elements are allocated out of the nursery too.
Attachment #8524664 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett (:bhackett) from comment #2)
> This looks fine, but why not just always make a copy of the template object?
> If the passed in length is greater than the template object's capacity we'll
> end up with an array which hasn't been fully allocated, but it's fine for
> new Array(n) to do that.

If `n` is pretty large (but < ArrayObject::EagerAllocationMaxLength), it's more efficient to allocate space immediately instead of reallocating repeatedly while filling the array later.
https://hg.mozilla.org/mozilla-central/rev/ea2d1aa200e4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 874174
Comment on attachment 8524664 [details] [diff] [review]
Patch

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

::: js/src/jit/MIR.h
@@ +2649,5 @@
> +class MNewArrayDynamicLength
> +  : public MUnaryInstruction,
> +    public IntPolicy<0>::Data
> +{
> +    AlwaysTenured<ArrayObject*> templateObject_;

Can you make use of an MConstant to hold this template object instead? (like MNewArray / MNewObject)
This would be easier for recovering this instruction later.
You need to log in before you can comment on or make changes to this bug.