Closed Bug 1496847 Opened Last year Closed Last year

GETELEM Generic Cache attaches for dense elements, shadowing more specific optimization opportunity.

Categories

(Core :: JavaScript Engine: JIT, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: mgaudet, Assigned: djvj)

References

Details

(Whiteboard: [qf:p1:f64])

Attachments

(1 file)

The IC added in Bug 1488786 has a small design flaw. 

I described an IC chain pathology in Bug 1494537, Comment #10: An edited quote follows: 

> If we attach a generic stub, generic stub will serve all requests 
> until the GC causes us to discard stubs and we come around and try to attach
> a new stub. ...
> 
> However, it could also be a large degradation too: Imagine a GETELM sees
> the following sequence of indices: [102912, 0, 1, 2, 3,..., 1024]. Even though
> we have a faster stub for every write after the first, if we attach the 
> generic stub first, it will service all requests, we'll not hit the fallback
> stub again, and we'll not attach a faster stub after that. 

This should be fixed by making the generic getelem stub only attach for sparse indices, and guard index is greater than denseInitialized/denseCapacity.
Priority: -- → P1
Whiteboard: [qf:p1:f64]
Comment on attachment 9016394 [details] [diff] [review]
restrict-generic-getelem-to-sparse-accesses.patch

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

Phabricator won't let me ask for review on this patch directly, because it is on top of the patch for bug 1494537.  Boo.  Using old-style review request.  Small patch so this shouldn't be a big deal.
Attachment #9016394 - Flags: review?(tcampbell)
Comment on attachment 9016394 [details] [diff] [review]
restrict-generic-getelem-to-sparse-accesses.patch

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +2888,5 @@
> +
> +    // Load obj->elements.
> +    masm.loadPtr(Address(obj, NativeObject::offsetOfElements()), scratch);
> +
> +    // Ensure index is greater than than the capacity.

s/greater than than/outside of/
Attachment #9016394 - Flags: review?(tcampbell) → review+
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/237c50cb98bc
Restrict GetElem generic stub to access on sparse indexes only. r=tcampbell
https://hg.mozilla.org/mozilla-central/rev/237c50cb98bc
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
No longer depends on: 1494537
You need to log in before you can comment on or make changes to this bug.