699210 - Add memory reporter for base shapes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

This is a new kind of GC thing added for object-shrink, and should be counted in about:memory.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/projects/jaegermonkey/rev/ef1f81733ed8

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 571435 [details] [diff] [review]
patch

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1342,5 @@
> +        case JSTRACE_BASE_SHAPE:
> +        {
> +            curr->gcHeapShapesBase += thingSize;
> +            break;
> +        }

There's a big bug here -- the PropertyTable has moved to the BaseShape, but you're still counting it through the Shape, which means that you'll multiply-count every PropertyTable that belongs to a shared BaseShape.  You should move Shape::sizeOfPropertyTable() to BaseShape.

And once you've done that, does it still make sense to have the tree-tables vs. dict-tables distinction?  I guess it depends if every BaseShape clearly belongs to only tree or dict shapes.  I don't know if that's the case, but I don't see a BaseShape::inDictionary().

Comment 4

[Brian Hackett [Laid off!]]

Each base shape that has a property table is owned by the referring shape --- if two shapes have the same base shape then that base shape has no property table.  So there should be no double counting.

http://hg.mozilla.org/projects/jaegermonkey/file/tip/js/src/jsscope.h#l295

Comment 5

[Nicholas Nethercote [inactive]]

Good to hear there's no double-counting.  Can you move sizeOfPropertyTable() into BaseShape anyway?  It's only used for memory reporting, and it makes it more obvious what's going on.

Comment 6

[Nicholas Nethercote [inactive]]

This was merged ages ago.