752952 - nsIDocument's PropertyTables should be accounted for in about:memory

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nathan Froyd [:froydnj]]

On the HTML5 single-page spec, post-bug 747508, the single biggest offender is:

==24328== Unreported: 1 block(s) in record 1 of 15374
==24328==  3,149,824 bytes (3,145,732 requested / 4,092 slop)
==24328==  2.77% of the heap (2.77% cumulative unreported)
==24328==    at 0x4C27193: malloc (vg_replace_malloc.c:263)
==24328==    by 0x68521B9: moz_malloc (mozalloc.cpp:97)
==24328==    by 0x9A4407B: PL_DHashAllocTable (pldhash.cpp:117)
==24328==    by 0x9A44A9E: ChangeTable(PLDHashTable*, int) (pldhash.cpp:560)
==24328==    by 0x9A44E43: PL_DHashTableOperate (pldhash.cpp:645)
==24328==    by 0x89AD7AA: nsPropertyTable::SetPropertyInternal(nsPropertyOwner, nsIAtom*, void*, void (*)(void*, nsIAtom*, void*, void*), void*, bool, void**) (nsPropertyTable.cpp:260)
==24328==    by 0x8968FB5: nsPropertyTable::SetProperty(nsPropertyOwner, nsIAtom*, void*, void (*)(void*, nsIAtom*, void*, void*), void*, bool, void**) (nsPropertyTable.h:137)
==24328==    by 0x896CC09: nsINode::SetProperty(unsigned short, nsIAtom*, void*, void (*)(void*, nsIAtom*, void*, void*), bool, void**) (nsGenericElement.cpp:231)
==24328==    by 0x854C598: nsINode::SetProperty(nsIAtom*, void*, void (*)(void*, nsIAtom*, void*, void*), bool) (nsINode.h:636)
==24328==    by 0x86EE60F: nsTextFrame::GetInFlowContentLength() (nsTextFrameThebes.cpp:635)
==24328==    by 0x870280E: nsTextFrame::ReflowText(nsLineLayout&, int, nsRenderingContext*, bool, nsHTMLReflowMetrics&, unsigned int&) (nsTextFrameThebes.cpp:7261)
==24328==    by 0x86C2FA5: nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) (nsLineLayout.cpp:872)

Fixing this is just a matter of modifying nsIDocument::DocSizeOfExcludingThis and adding in mPropertyTable and friends to the appropriate places.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

DMD-checked, etc.

Ambivalent if this should get its own node or be rolled into the dom/ blob.  I lean towards splitting it out because the DMD stacks make it appear as though layout is using it quite a bit, so it doesn't make much sense putting it in one or the other.  Olli, do you have an opinion on this?

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: patch

Here, let me not screw up the paths...

Comment 3

[Nicholas Nethercote [inactive]]

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

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

Splitting it out seems fine;  we can revisit later if necessary.

::: dom/base/nsWindowMemoryReporter.cpp
@@ +332,5 @@
>    REPORT("window-objects/dom/comment-nodes", windowTotalSizes.mDOMCommentNodes,
>           "Memory used for DOM comment nodes within windows. "
>           "This is the sum of all windows' 'dom/comment-nodes' numbers.");
>  
> +  REPORT("window-objects//property-tables",

Double slash.  Did you attach the same patch again?  r=me with that fixed.

Comment 4

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c09642d4cd0

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c09642d4cd0