Closed
Bug 1314569
Opened 8 years ago
Closed 8 years ago
Purge ShapeTables on compacting GC
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
(Whiteboard: [MemShrink:P1])
Attachments
(1 file)
51.37 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
On websites like GMail or cnn.com, we can easily have more than 3 MB of ShapeTables. Information in ShapeTables is redundant (except for the free list, see below) so we could deallocate shape tables and reconstruct them when needed. There are a few issues to consider: (1) Code may store ShapeTable* and ShapeTable::Entry* pointers on the stack across a GC. For this, we could either not purge in active zones, or introduce a holder-class that prevents purging within the current zone. (2) For dictionary shapes, the ShapeTable also stores the slot free list. The simplest option is to only remove the shape table if its free list is empty (and then measure how common the non-empty free list case is). (3) Perf overhead. If we do this only on shrinking GCs, it shouldn't be an issue I think. (4) ShapeTables require owned BaseShapes and in some cases, when we get rid of the shape table, we could make the owner shapes point to an unowned BaseShape instead. That's a lot more complicated and probably not worth the additional memory savings, though.
Assignee | ||
Comment 1•8 years ago
|
||
On areweslimyet.com, when I look at the "JS: After TP5 [+30s]" reports, we have: js-main-runtime total: 133.38 MB tree-tables: 8.03 MB dict-tables: 1,37 MB So we could win more than 9 MB here, which is like 6-7% of all JS memory.
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #1) > So we could win more than 9 MB here, which is like 6-7% of all JS memory. FWIW, it's a similar 6-7% for the "After TP5 [+30s, forced GC]" reports - ~118 MB total, > 8 MB tree/dict tables.
Comment 3•8 years ago
|
||
Regarding (2), is the free list one of https://dxr.mozilla.org/mozilla-central/rev/3e73fd638e687a4d7f46613586e5156b8e2af846/js/src/jsgc.h#614 or something else? Also, those are impressive savings :)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #3) > Regarding (2), is the free list one of > https://dxr.mozilla.org/mozilla-central/rev/ > 3e73fd638e687a4d7f46613586e5156b8e2af846/js/src/jsgc.h#614 or something else? It's the one in ShapeTable, this one: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/js/src/vm/Shape.h#186-188 Basically we add to the free list when we remove a data property from a dictionary object. Then when we need a slot for a new property, we can grab one from the free list instead of allocating new slots.
Assignee | ||
Comment 5•8 years ago
|
||
I'll write a patch today or tomorrow. This should be pretty simple.
Flags: needinfo?(jdemooij)
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee | ||
Comment 6•8 years ago
|
||
This adds a new GC phase to shrinking GCs to purge shape tables. We purge all tables that have an empty free list. I changed methods that return a ShapeTable or ShapeTable::Entry* to take either AutoKeepShapeTables& or AutoCheckCannotGC&, to ensure we don't purge tables we're operating on. AutoKeepShapeTables prevents purging shape tables in the current zone. The patch also cleans up Shape::search a bit. I split it in 2 versions, so I could make one of them fallible (the one that returns a ShapeTable::Entry*) and the other version can stay infallible (that one has a lot more callers and can fall back to a linear search). about:memory confirms this works well, and this is green on Try so far (except for a bogus assert I fixed in this patch).
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8807129 -
Flags: review?(jcoppeard)
Comment 7•8 years ago
|
||
Comment on attachment 8807129 [details] [diff] [review] Patch Review of attachment 8807129 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, but I have a couple of questions. ::: js/src/vm/NativeObject.cpp @@ +997,5 @@ > MOZ_ASSERT(slot < slotSpan()); > > if (inDictionaryMode()) { > + AutoCheckCannotGC nogc; > + if (ShapeTable* table = lastProperty()->ensureTableForDictionary(cx, nogc)) { What's the point of creating this if it doesn't exist just so we can free a slot in it? Is it because there is an invariant that we don't purge shape tables which have free slots? If so, how is it OK to recoverFromOutOfMemory() if this fails? ::: js/src/vm/NativeObject.h @@ +726,5 @@ > * after calling object-parameter-free shape methods, avoiding coupling > * logic across the object vs. shape module wall. > */ > static bool allocSlot(ExclusiveContext* cx, HandleNativeObject obj, uint32_t* slotp); > + void freeSlot(uint32_t slot, ExclusiveContext* cx); The parameter order reads strangely here. It would be more consistent to take cx first. ::: js/src/vm/Shape.cpp @@ +540,3 @@ > ShapeTable::Entry* entry = nullptr; > + if (obj->inDictionaryMode()) { > + AutoCheckCannotGC nogc; Do we need this as well as AutoKeepShapeTables above? @@ +1042,1 @@ > Should we assert the result of maybeTable() is non-null here?
Attachment #8807129 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Thanks for the quick review! (In reply to Jon Coppeard (:jonco) from comment #7) > What's the point of creating this if it doesn't exist just so we can free a > slot in it? Is it because there is an invariant that we don't purge shape > tables which have free slots? If so, how is it OK to > recoverFromOutOfMemory() if this fails? The free list we store in ShapeTables is the list of free slots *in the object itself*, not the table. So if we remove a property, we want to add its slot to the free list so we can reuse this slot later for another property. We create a ShapeTable because it stores the object's free list, but other than that it's mostly unrelated. I'll add a comment describing this. > The parameter order reads strangely here. It would be more consistent to > take cx first. Sometimes the cx parameter is added last to indicate the function is infallible, like here: http://searchfox.org/mozilla-central/source/js/src/jsarray.cpp#2375 We don't do this often and this one returns |void| so maybe I should change it. > > + AutoCheckCannotGC nogc; > > Do we need this as well as AutoKeepShapeTables above? Good catch, we can remove it. > Should we assert the result of maybeTable() is non-null here? Yes will do.
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3cf01afceb6 Purge ShapeTables on shrinking GCs. r=jonco
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b3cf01afceb6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #1) > On areweslimyet.com, when I look at the "JS: After TP5 [+30s]" reports, we > have: > > js-main-runtime total: 133.38 MB > > tree-tables: 8.03 MB > dict-tables: 1,37 MB > > So we could win more than 9 MB here, which is like 6-7% of all JS memory. The minimum on this graph used to be about 121 MB AFAICS and after this patch landed it's 112-114 MB, so we got most of the 9 MB here \o/
Comment 12•8 years ago
|
||
\o/
You need to log in
before you can comment on or make changes to this bug.
Description
•