1799028 - Cache for-in iterators on the Shape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Iain Ireland [:iain]]

Looking at Speedometer profiles, Jan noticed that retrieving iterators from the cache in for-in loops was a performance hotspot. For example this is one of the hottest functions in the elm subbenchmark.

We can make it faster to retrieve the iterator by caching it on the shape. In particular, we already have ShapeCachePtr, which can cache several different things; it seems reasonable to reuse it here.

Comment 1

[Iain Ireland [:iain]]

Attachment: Bug 1799028: Cache iterator in shape r=jandem

I've left the existing caching mechanism in place for now, and added the shape-based caching on top of that. We can consider removing the existing iterator cache in a follow-up.

Comment 2

[Iain Ireland [:iain]]

Attachment: Bug 1799028: Factor out iterator activation and share implementation of GuardAndGetIterator r=jandem

The next patch adds a CacheIR op to get the iterator from the shape cache, which also needs to activate iterators.

The only difference between the two implementations of GuardAndGetIterator was how we load the enumerators. Making this shared also lets us share the code in the next patch.

Depends on D162048

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1799028: Add ObjectToIteratorResult r=jandem

Depends on D162049

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1799028: Transpile ObjectToIteratorResult r=jandem

Depends on D162050

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1799028: Move enumerators from Realm to Compartment r=jandem

This makes it possible for us to bake the address of the active iterator list into stub data. If it's per-realm, then we can't assume that all objects reaching a given stub should be added to the same list. With a per-compartment list, we can be sure every object reaching a given stub shares the compartment (even if it's a CCW representing an object from another compartment).

While moving this, I also changed it from being a pointer to a NativeIterator to embedding a NativeIterator directly. This saves an indirection, makes some previous fallible code infallible, and makes everything a little simpler.

Depends on D162051

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1799028: Refactor iterator sentinel r=jandem

We were sometimes asserting when deleting compartments because the GCPtr destructor for the unused fields of the iterator sentinel saw that they were being destroyed outside of a GC. This wasn't a problem when the sentinel was heap-allocated because we used a UniquePtr with FreePolicy instead of DeletePolicy, so the destructor never ran.

I think the best fix is to avoid storing those unused fields in the first place by splitting out a superclass.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd5d3f62e4da
Cache iterator in shape r=jandem
https://hg.mozilla.org/integration/autoland/rev/97d6a930de1b
Factor out iterator activation and share implementation of GuardAndGetIterator r=jandem
https://hg.mozilla.org/integration/autoland/rev/471e76800bc3
Add ObjectToIteratorResult r=jandem
https://hg.mozilla.org/integration/autoland/rev/3e245a08238b
Transpile ObjectToIteratorResult r=jandem
https://hg.mozilla.org/integration/autoland/rev/b0dc5f1fd855
Move enumerators from Realm to Compartment r=jandem
https://hg.mozilla.org/integration/autoland/rev/ba555724d7cb
Refactor iterator sentinel r=jandem

Comment 8

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cd5d3f62e4da
https://hg.mozilla.org/mozilla-central/rev/97d6a930de1b
https://hg.mozilla.org/mozilla-central/rev/471e76800bc3
https://hg.mozilla.org/mozilla-central/rev/3e245a08238b
https://hg.mozilla.org/mozilla-central/rev/b0dc5f1fd855
https://hg.mozilla.org/mozilla-central/rev/ba555724d7cb

Comment 9

[Mayank Bansal]

Some improvements on the following Speedometer tests:

Elm-TodoMVC/*
React-Redux-TodoMVC/*
and maybe Preact also

Comment 10

[Iain Ireland [:iain]]

I measured about a 7% improvement on Elm while developing this patch. The React numbers weren't significant in my sample size but also looked like there might be a small win.