Closed
Bug 1368420
Opened 7 years ago
Closed 7 years ago
Map and Set and their iterators can't be nursery allocated
Categories
(Core :: JavaScript: GC, enhancement)
Core
JavaScript: GC
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: anba, Assigned: jonco)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(3 files)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Jonco, how hard do you think it'd be to fix this?
Flags: needinfo?(jcoppeard)
Comment 2•7 years ago
|
||
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•7 years ago
|
||
See also Bug 1389381.
Updated•7 years ago
|
Whiteboard: [qf]
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 4•7 years ago
|
||
I'd like to take this on if no one else is already planning on it.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Comment 5•7 years ago
|
||
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•7 years ago
|
Component: JavaScript Engine → JavaScript: GC
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 6•7 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•7 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 8•7 years ago
|
||
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 9•7 years ago
|
||
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 | ||
Comment 10•7 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•7 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•7 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 13•7 years ago
|
||
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•7 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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d52be65ca653
https://hg.mozilla.org/mozilla-central/rev/bab465c74656
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•