Use AllChildrenIterator to traverse DOM nodes when memory reporting

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

2 years ago
This gets us coverage for anonymous nodes, but removes coverage for
EventTargets.
Assignee

Comment 1

2 years ago
There are still some ComputedValues that are being missed by memory reporting.
I don't know how or why.
Attachment #8897266 - Flags: review?(bobbyholley)
Comment on attachment 8897266 [details] [diff] [review]
Use AllChildrenIterator to traverse DOM nodes when memory reporting

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

I don't understand the comment about EventTargets?

Probably need to use DMD to figure out why we're still missing nodes.
Attachment #8897266 - Flags: review?(bobbyholley) → review+
Assignee

Comment 3

2 years ago
> I don't understand the comment about EventTargets?

We were talking on IRC about how AllChildrenIterator misses some nodes that the old traversal hit. I investigated and found that they were all EventTargets. I'm not sure what to make of that because of my lack of familiarity with DOM data structures.
EventTarget is just the base class of node. There is no such thing as a concrete EventTarget.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
>There is no such thing as a
> concrete EventTarget.
FWIW, there will be real soon, bug 1379688.
But such EventTarget has nothing to do with DOM tree.
Assignee

Comment 6

2 years ago
Here's an example of a node touched by the existing traversal and missed by AllChildrenIterators:

> (gdb) p *node
> $6 = {<nsINode> = {<mozilla::dom::EventTarget> = {<nsIDOMEventTarget> = {<nsISupports> = {
>           _vptr$nsISupports = 0x7fffebc45358 <vtable for mozilla::dom::Comment+16>}, <No data fields>}, <nsWrapperCache> = {
>         _vptr$nsWrapperCache = 0x7fffebc457c0 <vtable for mozilla::dom::Comment+1144>, mWrapper = 0x0, mFlags = 0, mBoolFlags = 131074}, <No data fields>}, 
>     mNodeInfo = {mRawPtr = 0x7fffc11f5280}, mParent = 0x7fffc9d09000, 
>     mNextSibling = 0x7fffc9d0c630, mPreviousSibling = 0x0, mFirstChild = 0x0, 
>     {mPrimaryFrame = 0x0, mSubtreeRoot = 0x0}, mSlots = 0x0}, 
>   static sTabFocusModel = 7, static sTabFocusModelAppliesToXUL = false}

I saw the EventTarget and mistakenly thought that was important. Looking again, the `vtable for mozilla::dom::Comment` parts suggest that it's a Comment node. All such nodes that I inspected in the debugger looked similar.

I also noticed from some debugging printfs that these nodes always show up at the start of the existing traversal. I.e. they are always the first node or first few nodes of a document.

bholley: any additional thoughts about this? Does this make sense to you?
Flags: needinfo?(bobbyholley)
Yeah, that makes perfect sense.

The old code was traversing from the Document node, whereas the new code traverses from the root Element, which is a child of the Document node. It is the only child allowed of the document, aside from comment nodes. And we can't use AllChildrenIterator on the Document node, because it requires nsIContent (which includes most everything but Document nodes).

So I guess if we want to make this code entirely accurate, we should really walk over the entire GetNextSibling() list of doc->GetFirstChild(), and invoke SizeOfNodeTree on each node (one of which will be the root that we're measuring now). This does't matter for stylo, since CSS operates on Elements (and comment nodes are not Elements). But it seems sensible for completeness to invoke the measurement function on all the nodes in the document.

Thanks for digging into that!
Flags: needinfo?(bobbyholley)
Assignee

Comment 8

2 years ago
bholley: I used printfs to check that the nodes found by this traversal is a
strict superset of the nodes found by the old traversal, and that there are no
dups. But I'd appreciate a double-check from you as well. Thanks.
Attachment #8897683 - Flags: review?(bobbyholley)
Assignee

Updated

2 years ago
Attachment #8897266 - Attachment is obsolete: true
Attachment #8897683 - Flags: review?(bobbyholley) → review+
Assignee

Comment 9

2 years ago
Comment on attachment 8897683 [details] [diff] [review]
Use AllChildrenIterator to traverse DOM nodes when memory reporting

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

::: dom/base/nsDocument.cpp
@@ +12546,4 @@
>    for (nsIContent* node = nsINode::GetFirstChild();
>         node;
> +       node = node->GetNextSibling()) {
> +    AddSizeOfNodeTree(GetRootElement(), aWindowSizes);

Glad I did a DMD run because this `GetRootElement()` is supposed to be `node`! (My printf-checking run was with a similar but distinct patch, and they didn't match.)
Assignee

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/43225077009176ff4bbb2a90690b04bcb1ece721
Bug 1390404 - Use AllChildrenIterator to traverse DOM nodes when memory reporting. r=bholley.
Assignee

Updated

2 years ago
Blocks: 1281964

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/432250770091
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(In reply to Nicholas Nethercote [:njn] from comment #9)
> Comment on attachment 8897683 [details] [diff] [review]
> Use AllChildrenIterator to traverse DOM nodes when memory reporting
> 
> Review of attachment 8897683 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDocument.cpp
> @@ +12546,4 @@
> >    for (nsIContent* node = nsINode::GetFirstChild();
> >         node;
> > +       node = node->GetNextSibling()) {
> > +    AddSizeOfNodeTree(GetRootElement(), aWindowSizes);
> 
> Glad I did a DMD run because this `GetRootElement()` is supposed to be
> `node`! (My printf-checking run was with a similar but distinct patch, and
> they didn't match.)

Doh! Sorry for missing that.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.