Right now, we have one big "dom" category in about:memory; per-node type statistics would be helpful for diagnosing issues.

This patch is a split-out bit of my patch in bug 686795; I chose the node types to report based on that bug and a few others that seemed useful. bz, do those make sense, or should I change the list? Reporting on the total number of DOM nodes seemed helpful in the aforementioned bug, so I included that as well.

Comment on attachment 629495 [details] [diff] [review] patch > nsDocument::DocSizeOfExcludingThis(nsWindowSizes* aWindowSizes) const >+ case nsIDOMNode::ATTRIBUTE_NODE: This case will never be here: this is looping over nodes reachable via firstChild/nextSibling links from the document, which will never include attribute nodes. >+ case nsIDOMNode::DOCUMENT_NODE: This won't get hit either, since we start with the firstChild of the document, so we'll never have a Document node on our walk. You could either assert that these cases are hit, or just let them fall through to mDOM. r=me with that

(In reply to Boris Zbarsky (:bz) from comment #2) > > nsDocument::DocSizeOfExcludingThis(nsWindowSizes* aWindowSizes) const > >+ case nsIDOMNode::ATTRIBUTE_NODE: > > This case will never be here: this is looping over nodes reachable via > firstChild/nextSibling links from the document, which will never include > attribute nodes. Is there a good way to include them in the walking process? I suppose they don't generally take up that much space (since we haven't noticed them with DMD), but for completeness...

> Is there a good way to include them in the walking process? The SizeOf() for nsGenericElement can include them if desired (via the attribute map in the slots, iirc). Note that we allocate attribute nodes lazily, so unless the page explicitly tries to work with them they just won't be present.

Comment on attachment 629495 [details] [diff] [review] patch Review of attachment 629495 [details] [diff] [review]: ----------------------------------------------------------------- My confidence in this patch is hurt by the fact that I identified three assertion failures in about:memory just from reading the code. (I confirmed one of these by running the code.) Have you tested this? Nonetheless, the basic idea is sound. I'd like to see and try a revised patch. ::: dom/base/nsWindowMemoryReporter.cpp @@ +157,5 @@ > aWindow->SizeOfIncludingThis(&windowSizes); > > REPORT("/dom", windowSizes.mDOM, > "Memory used by a window and the DOM within it."); > aWindowTotalSizes->mDOM += windowSizes.mDOM; Memory reporters must specify leaf nodes in the about:memory trees. But you have paths ".../dom", ".../dom/element-nodes", etc. The ".../dom" one needs to be changed to ".../dom/other" or ".../dom/misc" or something. This would have caused an assertion failure in about:memory if the other assertion failure below hadn't happened first. @@ +163,5 @@ > + // Don't bother breaking the per-node numbers down on a global basis, > + // just throw them all into one basket for the total size. > + REPORT("/dom/element-nodes", windowSizes.mDOMNodes.mElementNodes, > + "Memory used by the element nodes in a window's DOM."); > + aWindowTotalSizes->mDOM += windowSizes.mDOMNodes.mElementNodes; We currently have a "window-objects-dom" measurement in about:memory, which is the sum of the "dom" measurement for each window. It should be split into "window-objects-dom-other", "window-objects-dom-element-nodes, etc. Bug 760352 will help with this. @@ +192,5 @@ > + NS_LITERAL_CSTRING("dom/n-nodes"), > + nsIMemoryReporter::KIND_OTHER, > + nsIMemoryReporter::UNITS_COUNT, > + windowSizes.mDOMNumberOfNodes, > + NS_LITERAL_CSTRING("The number of DOM nodes in a window"), "node-count" or "number-of-nodes" would be a better name. The description should be a complete sentence, i.e. end in '.'. This would cause an assertion in about:memory, if it wasn't for the assertion below. This causes an assertion in about:memory because KIND_OTHER reporters can't have a '/' in their name. Bug 760352 will lift this restriction, but it will take at least a week to get review, so I suggest you just take out this measurement for now and focus on the node byte measurements. ::: dom/base/nsWindowMemoryReporter.h @@ +35,5 @@ > + size_t mCDATANodes; > + size_t mCommentNodes; > + size_t mDocumentNodes; > + // Everything else gets accumulated into mDOM. > + } mDOMNodes; Having a struct doesn't gain much. Can you follow the lead of the layout numbers and just define mDOMElementNodes, mDOMAttributeNodes, etc.

(In reply to Nicholas Nethercote [:njn] from comment #5) > My confidence in this patch is hurt by the fact that I identified three > assertion failures in about:memory just from reading the code. (I confirmed > one of these by running the code.) Have you tested this? Tested by the tried-and-true, "hey, I wrote some printfs and those worked, let's throw the numbers in about:memory...and it compiles! Ship it!" Obviously not a winning strategy. Thank you for reviewing it anyway. > @@ +163,5 @@ > > + // Don't bother breaking the per-node numbers down on a global basis, > > + // just throw them all into one basket for the total size. > > + REPORT("/dom/element-nodes", windowSizes.mDOMNodes.mElementNodes, > > + "Memory used by the element nodes in a window's DOM."); > > + aWindowTotalSizes->mDOM += windowSizes.mDOMNodes.mElementNodes; > > We currently have a "window-objects-dom" measurement in about:memory, which > is the sum of the "dom" measurement for each window. It should be split > into "window-objects-dom-other", "window-objects-dom-element-nodes, etc. > Bug 760352 will help with this. It's not clear to me what the rationale is here. Is the idea that we should have sums across all relevant leaf nodes? I didn't think that would be helpful in this instance, hence the above approach and the comment. In any event, I've gone ahead and implemented separate summed categories as requested. > ::: dom/base/nsWindowMemoryReporter.h > @@ +35,5 @@ > > + size_t mCDATANodes; > > + size_t mCommentNodes; > > + size_t mDocumentNodes; > > + // Everything else gets accumulated into mDOM. > > + } mDOMNodes; > > Having a struct doesn't gain much. Can you follow the lead of the layout > numbers and just define mDOMElementNodes, mDOMAttributeNodes, etc. Done. (Though I think for the layout numbers, it would be less messy if there was a single struct; no parameter passing is involved for the DOM numbers, so the rationale is less relevant there.)

Comment on attachment 630170 [details] [diff] [review] patch Review of attachment 630170 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the new version. Yeah, if you're modifying memory reporters, you need to test them with about:memory. In fact, running all the relevant tests is an even better idea -- within $BUILD/_tests/testing/mochitest, run "python runtests.py --chrome --autorun --test-path=toolkit/components/aboutmemory/tests/". > It's not clear to me what the rationale is here. Is the idea that > we should have sums across all relevant leaf nodes? Pretty much. The measurements answer the question "how is the DOM memory usage split up?" I've found that quite useful at times for the JS engine (and see bug 711130 for a request for more of it, which I'm on the way to implementing). Looks like you removed the ATTRIBUTE_NODE and DOCUMENT_NODE counters -- were they not significant? ::: content/base/src/nsDocument.cpp @@ +9703,5 @@ > nsIDocument::DocSizeOfExcludingThis(aWindowSizes); > > for (nsIContent* node = nsINode::GetFirstChild(); > node; > + node = node->GetNextNode(this)) { AIUI, putting the brace on the next line is common (standard?) when the for loop header spans multiple lines. ::: dom/base/nsWindowMemoryReporter.cpp @@ +160,1 @@ > "Memory used by a window and the DOM within it."); This description needs updating. @@ +160,5 @@ > "Memory used by a window and the DOM within it."); > aWindowTotalSizes->mDOM += windowSizes.mDOM; > > + // Don't bother breaking the per-node numbers down on a global basis, > + // just throw them all into one basket for the total size. I think this comment is wrong and can be removed. ::: dom/base/nsWindowMemoryReporter.h @@ +34,1 @@ > size_t mDOM; Please rename this mDOMOther to match the "dom/other" path name. @@ +34,2 @@ > size_t mDOM; > + size_t mDOMNumberOfNodes; This field is unused and can be removed.

Pushed with requested changes: https://hg.mozilla.org/integration/mozilla-inbound/rev/aff9bc34ee04 I hope the updated descriptions are OK; I figured it wasn't worth asking for re-review for those. I can fix in a followup if you'd like them changed. (In reply to Nicholas Nethercote [:njn] from comment #7) > > It's not clear to me what the rationale is here. Is the idea that > > we should have sums across all relevant leaf nodes? > > Pretty much. The measurements answer the question "how is the DOM memory > usage split up?" I've found that quite useful at times for the JS engine > (and see bug 711130 for a request for more of it, which I'm on the way to > implementing). Maybe we should look into automatically summing these? I filed bug 762040 for that. > Looks like you removed the ATTRIBUTE_NODE and DOCUMENT_NODE counters -- were > they not significant? bz indicated in comment 2 that those cases will never get hit in this function. (I'd imagine DOCUMENT_NODE is insignificant and ATTRIBUTE_NODE might get lumped in with ELEMENT_NODEs's SizeOfThis function, depending on how it's implemented.) > ::: content/base/src/nsDocument.cpp > @@ +9703,5 @@ > > nsIDocument::DocSizeOfExcludingThis(aWindowSizes); > > > > for (nsIContent* node = nsINode::GetFirstChild(); > > node; > > + node = node->GetNextNode(this)) { > > AIUI, putting the brace on the next line is common (standard?) when the for > loop header spans multiple lines. I've left this unchanged to avoid a bit of churn; prevailing style at least in content/base/src seems to be to keep the brace close, even on multi-line ifs. I can't find information yea or nay on this, but I also didn't look very hard. :)