1368420 - Map and Set and their iterators can't be nursery allocated

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[André Bargull [:anba]]

Map and Set and their iterators require foreground finalization which means they can't be nursery allocated.

six-speed-map-set-es6 and six-speed-map-set-object-es6 are slow because of this limitation.

Comment 1

[Till Schneidereit [:till]]

Jonco, how hard do you think it'd be to fix this?

Comment 2

[Jan de Mooij [:jandem]]

Seems like this will work if we use AllocateObjectBuffer for the extra data, similar to what we do for ArgumentsObject etc.

This could be big also because it avoids post barriers when adding nursery-allocated objects to new maps/sets. These barriers often show up.

Comment 3

[Sean Stangl [:sstangl]]

See also Bug 1389381.

Comment 4

[Alex Thayer [:alexical] (she/her)]

I'd like to take this on if no one else is already planning on it.

Comment 5

[Alex Thayer [:alexical] (she/her)]

A bit of a backlog popped up for me, so I can't take this this second. I'll come back to it if no one takes it in the mean time.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1368420-map-set-iterator

Patch to allocate map and set iterator objects and associated Range data in the nursery.

The main complication here is that finalizers are not called for dead nursery objects, but these iterators make use of ordered hash table ranges which are kept in a list attached to the table and must be removed when they die.

The patch adds a second list to OrderedHashTable to track ranges allocated in the nursery.  Tables containing nursery ranges are added to a vector in the compartment.  When we sweep the nursery we drop nursery ranges from any tables in this vector.

Allocation of iterators is has to ensure that the Range is in the nursery if the JSObject is so that we can do to easy inside nursery check on the object that checks the chunk trailer.

Timings for benchmark that repeatedly iterates a 10 element map (average of 3 runs):

Before: 3.211 seconds
After:  2.109 seconds

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1368420-map-set

Patch to allocate map and set objects in the nursery.

This removes post barriers on object keys for nursery-allocated maps/sets but not map values.  This is more complicated and I'm not sure what the best way to achieve this is.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8905512 [details] [diff] [review]
bug1368420-map-set-iterator

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

Cool. There's some duplication, but probably hard to avoid.

::: js/src/ds/OrderedHashTable.h
@@ +84,5 @@
>      AllocPolicy alloc;
>      mozilla::HashCodeScrambler hcs;  // don't reveal pointer hash codes
>  
> +    template <typename F>
> +    void forEachRange(F f) {

Nice.

::: js/src/jscompartment.h
@@ +1227,5 @@
> +    /*
> +     * List of Map and Set objects with nursery-allocated iterators. Such
> +     * iterators need to be swept after minor GC.
> +     */
> +    js::Vector<JSObject*, 0, js::SystemAllocPolicy> mapsAndSetsWithNurseryIterators;

Any reason for storing this in JSCompartment instead of Zone? Fine either way.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8905938 [details] [diff] [review]
bug1368420-map-set

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

LGTM!

::: js/src/jscompartment.cpp
@@ +935,5 @@
>          watchpointMap->sweep();
>  }
>  
>  bool
> +JSCompartment::addMapWithNurseryMemory(MapObject* obj)

Nit: I'd define this and the SetObject method in the header file to avoid an out-of-line call for just a Vector append.

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Any reason for storing this in JSCompartment instead of Zone?

Just convenience because JSCompartment already had a sweepAfterMinorGC() method.

> Nit: I'd define this and the SetObject method in the header file to avoid an
> out-of-line call for just a Vector append.

Good idea.

Comment 11

[Jon Coppeard (:jonco)]

Annoyingly the first patch causes the hazard analysis to die (bug 1398213).  I'm going to try to work around this.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: bug1368420-work-around

Patch to work around hazard build internal compiler error by templating OrderedHashTable::forEachRange on a function pointer.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8906668 [details] [diff] [review]
bug1368420-work-around

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

::: js/src/ds/OrderedHashTable.h
@@ +83,5 @@
>      Range* nurseryRanges;       // list of all live Ranges on this table in the GC nursery
>      AllocPolicy alloc;
>      mozilla::HashCodeScrambler hcs;  // don't reveal pointer hash codes
>  
> +    template <void (*f)(Range* range, uint32_t arg)>

Nit: please add a comment mentioning this works around bug 1398213.

@@ +496,5 @@
>          static size_t offsetOfNext() {
>              return offsetof(Range, next);
>          }
> +
> +        static void onTableDestroyed(Range *range, uint32_t arg) {

Nit: * with the type, and below

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d52be65ca653
Allocate Map and Set iterators in the nursery r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/bab465c74656
Allocate Map and Set objects in the nursery r=jandem

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d52be65ca653
https://hg.mozilla.org/mozilla-central/rev/bab465c74656