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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: jonco)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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.
Jonco, how hard do you think it'd be to fix this?
Flags: needinfo?(jcoppeard)
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.
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
I'd like to take this on if no one else is already planning on it.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
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.
Assignee: dothayer → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

2 years ago
Component: JavaScript Engine → JavaScript: GC
(Assignee)

Updated

2 years ago
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 6

2 years ago
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
Attachment #8905512 - Flags: review?(jdemooij)
(Assignee)

Comment 7

2 years ago
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.
Attachment #8905938 - Flags: review?(jdemooij)
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.
Attachment #8905512 - Flags: review?(jdemooij) → review+
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.
Attachment #8905938 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

2 years ago
Depends on: 1398213
(Assignee)

Comment 10

2 years ago
(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.
(Assignee)

Comment 11

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

Comment 12

2 years ago
Patch to work around hazard build internal compiler error by templating OrderedHashTable::forEachRange on a function pointer.
Attachment #8906668 - Flags: review?(jdemooij)
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
Attachment #8906668 - Flags: review?(jdemooij) → review+

Comment 14

2 years ago
Pushed by jcoppeard@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/d52be65ca653
https://hg.mozilla.org/mozilla-central/rev/bab465c74656
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1399889
Depends on: 1400535
You need to log in before you can comment on or make changes to this bug.