Add memory reporter for base shapes

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
This is a new kind of GC thing added for object-shrink, and should be counted in about:memory.
(Assignee)

Comment 1

6 years ago
Created attachment 571435 [details] [diff] [review]
patch
Assignee: general → bhackett1024
Attachment #571435 - Flags: review?(nnethercote)
(Assignee)

Comment 2

6 years ago
https://hg.mozilla.org/projects/jaegermonkey/rev/ef1f81733ed8
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().
Attachment #571435 - Flags: review?(nnethercote) → review-
(Assignee)

Comment 4

6 years ago
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
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.
This was merged ages ago.
Blocks: 563700
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.