Closed Bug 1501328 Opened Last year Closed Last year

JSVM/CacheIR - Add optimized stub for out-of-bounds GETELEMs of indexed properties on Arrays

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: djvj, Assigned: djvj)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p1])

Attachments

(1 file, 1 obsolete file)

This case is hit during GDocs load of large documents.  Up to 5 million GETELEM reads on a sparse array, which hits the fallback path of ICs consistently.

We can optimize a good chunk of the slowpath away here, but ensuring that the proto chain does not contain any indexed properties and has no dense elements, that the receiver is an array, that the index is out of bounds, and then zero in on a simple shape search on the receiving object.

Preliminary patch is attached.

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1526bf4dfa3310634b58791872ac098335c723d
Assignee: nobody → kvijayan
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
Attachment #9019388 - Flags: review?(mgaudet)
Comment on attachment 9019388 [details] [diff] [review]
cacheir-oob-getelem-on-arrays.patch

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

Mostly seems straightforward, but I think your instincts are right about that dense element assertion, and think that should be a pre-req to going in. 

I also kinda feel like some tests might be worth putting together, but wouldn't withold r+ based on that.

::: js/src/jit/CacheIR.cpp
@@ +2240,5 @@
> +        return false;
> +    }
> +
> +    // Indexed properties on the prototype chain aren't handled by the helper.
> +    if (ObjectMayHaveExtraIndexedProperties(nobj->staticPrototype())) {

It might be worth expanding the comment a bit; I found it confusing why this is the right guard -- the trick seems to be that because the index is sparse, it's sufficient to check there are no extra indexed properties on the prototype.

::: js/src/vm/NativeObject.cpp
@@ +2540,5 @@
> +{
> +    // Callers should have ensured that this object has a static prototype.
> +    MOZ_ASSERT(obj->hasStaticPrototype());
> +
> +    // TODO (KVKV): Add assertion to ensure that int_id is not defined in either

Agreed!

@@ +2544,5 @@
> +    // TODO (KVKV): Add assertion to ensure that int_id is not defined in either
> +    // the dense properties of the proto chain, and that there exist no
> +    // indexed properties on any of the proto chain objects.
> +
> +    RootedId id(cx, INT_TO_JSID(int_id));

MOZ_ASSERT(INT_FITS_IN_JSID(int_id));
Attachment #9019388 - Flags: review?(mgaudet)
Updated patch with review comments addressed.
Attachment #9019388 - Attachment is obsolete: true
Attachment #9019841 - Flags: review?(mgaudet)
Comment on attachment 9019841 [details] [diff] [review]
cacheir-oob-getelem-on-arrays.patch

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

Looks good.

::: js/src/jit/CacheIR.cpp
@@ +2240,5 @@
> +        return false;
> +    }
> +
> +    // Here, we ensure that the prototype chain does not define any indexed
> +    // properties on the shape lineage.  This allows us to guard on the

nit: maybe say 'sparse indexed' properties. 

Whenever I read 'indexed properties' in this stuff, my finger goes up and I say "What about dense... oh wait. We've already checked that. We're good".

@@ +2271,5 @@
> +
> +    // At this point, we are guaranteed that the indexed property will not
> +    // be found on one of the prototypes.  We are assured that we only have
> +    // to check that the receiving object has the property.
> +

nit: Drop extra whitespace here.
Attachment #9019841 - Flags: review?(mgaudet) → review+
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d12102a7cea7
CacheIR stub for out-of-bounds indexed GetElement on Arrays. r=mgaudet
https://hg.mozilla.org/mozilla-central/rev/d12102a7cea7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.