Closed Bug 823061 Opened 7 years ago Closed 7 years ago

IonMonkey: GetPropertyCache should handle length property of arrays (Dense & Typed).

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

When a function is used with multiple kinds of arrays such as Dense arrays and Typed arrays, the type inference contains 2 type objects which cannot be inlined as being one type.

Ideally we should split the function based on the type of the argument (either typed array or dense array), but in the mean time adding support to GetPropertyCache is a good substitute.

This issue appear hundreds of times in PdfJS octane benchmark, especially in the decrypt function, which is already folding the length property access out of the loop so this should be a small impact.
This benchmark show the current performances when GetPropertyCache is used:

dense array length: 1215.488037109375
typed array length: 2170.376953125
This patch improves PdfJS performance by 1.4% (mostly Array.length) and give the following results on the previous micro benchmark:

dense array length: 25.161865234375
typed array length: 44.572998046875

Which correspond to a factor ~45 on this micro benchmark, and optimize both dense array and typed arrays.
Attachment #693926 - Flags: review?(jdemooij)
Comment on attachment 693926 [details] [diff] [review]
GetPropertyCache supports arrays length.

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

Nice, r=me with comments below addressed.

::: js/src/ion/IonCaches.cpp
@@ +731,5 @@
> +        outReg = output().typedReg().gpr();
> +    }
> +
> +    masm.loadPtr(Address(object(), JSObject::offsetOfElements()), outReg);
> +    masm.load32(Address(outReg, ObjectElements::offsetOfLength()), outReg);

Problem with dense arrays is that the length does not always fit in an int32 if you have something like |new Array(4294967295)|. Can you add a test for this that fails with the current patch? It's so uncommon/weird that we don't really care about its performance, so the easiest thing is to just treat it as a failure:

JS_ASSERT(object() != outReg);
masm.branchTest32(Assembler::Signed, outReg, outReg, &failure);

@@ +792,5 @@
> +    }
> +
> +    // Implement the negated version of JSObject::isTypedArray predicate.
> +    masm.loadBaseShape(object(), tmpReg);
> +    masm.loadBaseShapeClass(tmpReg, tmpReg);

Nit: you can use masm.loadObjClass(object(), tmpReg);

::: js/src/ion/IonCaches.h
@@ +110,5 @@
>          struct {
>              Register object;
>              PropertyName *name;
>              TypedOrValueRegisterSpace output;
>              bool allowGetters;

Nit: "allowGetters : 1" while you are here.

::: js/src/jstypedarray.cpp
@@ +109,5 @@
>   * can be created implicitly by constructing a TypedArray with a size.
>   */
>  
> +UnrootedShape
> +js::GetArrayBufferShape(JSContext *cx, HandleObject globalObj)

Nit: remove this function if we don't use it.
Attachment #693926 - Flags: review?(jdemooij) → review+
Btw, can you also add a test with a GETPROP that accesses the length of both a dense and typed array? Just to make sure we have test coverage for these IC stubs.
https://hg.mozilla.org/mozilla-central/rev/259aa5177333
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.