Closed Bug 1314569 Opened 3 years ago Closed 3 years ago

Purge ShapeTables on compacting GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 file)

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.
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.
(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.
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 :)
(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.
I'll write a patch today or tomorrow. This should be pretty simple.
Flags: needinfo?(jdemooij)
Whiteboard: [MemShrink] → [MemShrink:P1]
Attached patch PatchSplinter Review
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/b3cf01afceb6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(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/
Depends on: 1317402
Depends on: 1315262
You need to log in before you can comment on or make changes to this bug.