Inline |new Array(x)|

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 2 bugs, {perf})

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Keywords: perf
(Assignee)

Comment 1

4 years ago
Created attachment 8524664 [details] [diff] [review]
Patch

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+
(Assignee)

Comment 3

4 years ago
(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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Updated

4 years ago
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.