Last Comment Bug 699210 - Add memory reporter for base shapes
: Add memory reporter for base shapes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
:
Mentors:
Depends on:
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2011-11-02 13:31 PDT by Brian Hackett (:bhackett)
Modified: 2012-02-20 21:37 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.76 KB, patch)
2011-11-02 13:32 PDT, Brian Hackett (:bhackett)
n.nethercote: review-
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-11-02 13:31:03 PDT
This is a new kind of GC thing added for object-shrink, and should be counted in about:memory.
Comment 1 Brian Hackett (:bhackett) 2011-11-02 13:32:37 PDT
Created attachment 571435 [details] [diff] [review]
patch
Comment 2 Brian Hackett (:bhackett) 2011-11-02 13:33:00 PDT
https://hg.mozilla.org/projects/jaegermonkey/rev/ef1f81733ed8
Comment 3 Nicholas Nethercote [:njn] 2011-11-02 16:27:33 PDT
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 (:bhackett) 2011-11-02 17:50:42 PDT
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 [:njn] 2011-11-02 22:51:59 PDT
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 [:njn] 2012-02-20 21:37:45 PST
This was merged ages ago.

Note You need to log in before you can comment on or make changes to this bug.