704623 - track memory used by orphan DOM nodes in about:memory

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

Given the current DOM memory reporter we fail to report the memory used by orphan nodes, that is, nodes that are currently not part of a live document (reachable from a window object). We should add a way to track these and report that in about:memory. This would've helped when dealing with bug 701443 and other similar bugs.

Comment 1

[Johnny Stenback (:jst)]

Attachment: WIP for tracking orphan nodes.

This tracks orphan nodes in a doubly linked list (for fast insertion and removal, and for doing so w/o any extra overhead per node). The XBL code still needs work, but this shows the general idea.

Comment 2

[Johnny Stenback (:jst)]

Attachment: Make the nodeinfo manager know its documents URI

This lets us know what URI a node came from even after its document was destroyed. We can *almost* get this info w/o this patch through the principal pointer, but the system principal has no URI, so this closes that gap.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Show orphan node stats in about:memory

This adds an orphan-nodes section to about:memory and shows how much memory is used by orphan nodes per URI.

Comment 4

[Johnny Stenback (:jst)]

And for anyone who wants to try these patches out, they apply on top of the patch in bug 704621.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 577508 [details] [diff] [review]
Make the nodeinfo manager know its documents URI


>   /**
>+   * Get/Set the document URI. Can return null if not set yet. This is
>+   * primarily used to track origins of orphan nodes after their
>+   * document has been destroyed. This is especially important for
>+   * documents whose principal has no URI (i.e. chrome).
>+   */
>+  nsIURI *GetDocumentURI();
>+  void SetDocumentURI(nsIURI *aURI);
Why is this needed?
OwnerDoc() returns always a valid document object from which DocumentURI can be get.

Comment 6

[Johnny Stenback (:jst)]

Not in the case of an orphan node, in that case the owner doc can be null, no?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

You can't create a node which does not have owner document, and you can't delete owner document
before all the nodes which has it as OwnerDoc() are deleted.
OwnerDoc() returns always a valid value. That is why it is OwnerDoc(), not GetOwnerDoc() ;)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 577507 [details] [diff] [review]
WIP for tracking orphan nodes.

>+  void UnOrphan()
>+  {
>+    NS_ASSERTION((mFlags & NODE_IS_ORPHAN), "Orphan node orphaned again?");
Wrong assertion message?


>@@ -1136,7 +1199,10 @@ private:
>         return next;
>       }
>       nsINode* parent = cur->GetNodeParent();
>-      if (parent == aRoot) {
>+
>+      // If the parent is the root, or if the paret is an orphan node
'paret'

>+      // (which means it won't have a next sibling), return null.
>+      if (parent == aRoot || parent->IsOrphan()) {
But in which case does parent->IsOrphan() get executed.
If someone is iterating orphaned subtree, aRoot should be ==parent or nsnull.
And if it null, the while loop would just iterate once more to execute
GetNodeParent() which would return null.
If aRoot is not ==parent or not aRoot, the caller has passed non valid value to the method, IMO.



>       return nsnull;
>     }
>     nsIContent* cur = this->GetParent();
>+
>+    // If cur (our parent) is an orphan, return it as it won't have a
>+    // previous sibling.
>+    if (reinterpret_cast<nsINode*>(cur)->IsOrphan()) {
reinterpret_cast? Why not static_cast?
But I don't understand why this all is needed.
GetPreviousSibling() returns null, so while (iter) doesn't get executed
and cur will be returned.


>+++ b/content/base/src/nsDocument.cpp
>@@ -1913,6 +1913,12 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ns
> 
>   nsINode::Unlink(tmp);
> 
>+  // XXXjst: This aint right, figure out how to reliably (un)orphan
>+  // document objects.
>+  if (!tmp->IsOrphan()) {
>+    tmp->Orphan();
>+  }
Since orphan means effectively "not-bound-to-a-window", I think only
window should make a document orphan. And documents never in a window
should always be orphan.


>@@ -7009,6 +7022,8 @@ nsDocument::Destroy()
>   // XXX We really should let cycle collection do this, but that currently still
>   //     leaks (see https://bugzilla.mozilla.org/show_bug.cgi?id=406684).
>   nsContentUtils::ReleaseWrapper(static_cast<nsINode*>(this), this);
>+
>+  Orphan();
Is this really the right place for this?
Maybe when window releases its pointer to document, or when
scriptglobalobject of document is set to null?


>+bool nsINode::IsNodeInOrphanList(const nsINode *aNode)
>+{
>+  nsINode *n = sOrphanNodeHead->mNextOrphanNode;
>+
>+  while (n != sOrphanNodeHead) {
>+    if (n == aNode) {
>+      return true;
>+    }
>+
>+    n = n->mNextOrphanNode;
>+  }
>+
>+  return false;
>+}


Would it be bad to track all the subtree roots, and not only orphans.
In that case all the documents would be in the subtreeroot list, and all the
other nodes whenever they aren't in document.


>@@ -3683,6 +3704,8 @@ nsINode::doRemoveChildAt(PRUint32 aIndex
>     nsNodeUtils::ContentRemoved(this, aKid, aIndex, previousSibling);
>   }
> 
>+  // XXXjst: What if contentremoved moved the node, should we still
>+  // unbind here?
>   aKid->UnbindFromTree();
I sure hope contentremoved implementations don't move nodes anywhere.


>@@ -188,8 +188,10 @@ nsTextNode::BindToAttribute(nsIAttribute
>   NS_ASSERTION(!GetNodeParent(), "Unbind before binding!");
>   NS_ASSERTION(HasSameOwnerDoc(aAttr), "Wrong owner document!");
> 
>+  UnOrphan();
>   mParent = aAttr;
>   SetParentIsContent(false);
>+  // XXXjst: Uh, is this right?
>   ClearInDocument();
Well, there is not GetNodeParent() path from textnode to document, so
it isn't quite in document.
(Fortunately we're getting rid of textnodes in attributes)

Comment 9

[Johnny Stenback (:jst)]

(In reply to Olli Pettay [:smaug] from comment #7)
> You can't create a node which does not have owner document, and you can't
> delete owner document
> before all the nodes which has it as OwnerDoc() are deleted.
> OwnerDoc() returns always a valid value. That is why it is OwnerDoc(), not
> GetOwnerDoc() ;)

Hmm, I thought I hit a case where that was not true... I'll undo that change and see if I'm right or if I fixed the wrong problem there... This has gone through a good bit of mangling back n' fourth so it's entirely possible that I was fighting something unrelated that led me to believe I needed this...

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 577509 [details] [diff] [review]
Show orphan node stats in about:memory

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

::: content/base/src/nsGenericElement.cpp
@@ +3767,5 @@
> +    nsCOMPtr<nsIURI> uri;
> +    n->NodePrincipal()->GetURI(getter_AddRefs(uri));
> +
> +    if (!uri) {
> +      str += NS_LITERAL_CSTRING("system/");

As per bug 704621, is the system/content nesting split necessary?

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 577508 [details] [diff] [review]
Make the nodeinfo manager know its documents URI

This patch really is not necessary.

Comment 12

[Johnny Stenback (:jst)]

Attachment: Updated patch for tracking orphan nodes.

This passed try, and this is also immune to crashes in case someone forgets to mark a node as an orphan node before deleting it (bug debug builds will assert in such a case).

Comment 13

[Boris Zbarsky [:bzbarsky]]

orphanNodeListHead can end up misaligned..  We should ensure it has at least 8-byte alignment on all platforms.  Simplest way is probably to make the array size sizeof(nsINode) + 8 and then point the pointer to the first 0 mod 8 address in the array.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Also, have you done some tests of tree iteration (e.g. some querySelector tests for nonexistent elements on large trees)?  What's the performance impact on that and node removal/insertion of the new branches and writes?

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 595230 [details] [diff] [review]
Updated patch for tracking orphan nodes.

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

What's not clear to me is where these nodes show up in about:memory.  Is it under "dom+style/window-objects/other"?  I can't work out how the orphan nodes are traced by the DOM memory reporter.

An interesting experiment would be do just load about:memory with this patch and repeatedly hit the "Update" button.  Every time you do that the old DOM is discarded and a new one is generated.  In trunk builds this causes heap-unclassified to go up, hopefully a sizeable chunk of that will now instead fall under orphan nodes.

::: content/base/public/nsINode.h
@@ +1115,5 @@
> +#ifdef MOZILLA_INTERNAL_API
> +  // Mark this node as an orphan node. This marking is only relevant
> +  // for this node itself, not its children. Its children are not
> +  // considered orphan until they themselves are removed from their
> +  // parent and also.

"and also."  Is this sentence truncated?

@@ +1116,5 @@
> +  // Mark this node as an orphan node. This marking is only relevant
> +  // for this node itself, not its children. Its children are not
> +  // considered orphan until they themselves are removed from their
> +  // parent and also.
> +  void Orphan()

"Orphan" isn't a verb.  "MakeOrphan"?  "MarkAsOrphan"?

@@ +1132,5 @@
> +
> +  // Unmark this node as an orphan node. Do this before inserting this
> +  // node into a parent or otherwise associating it with some other
> +  // owner.
> +  void UnOrphan()

"MarkAsNonOrphan"?

Comment 16

[Johnny Stenback (:jst)]

(In reply to Boris Zbarsky (:bz) from comment #14)
> Also, have you done some tests of tree iteration (e.g. some querySelector
> tests for nonexistent elements on large trees)?  What's the performance
> impact on that and node removal/insertion of the new branches and writes?

I did a test where I (in C++) create a single node with 4000000 children (all text nodes) and then iterate from first child to last and from last to first with and w/o this patch, the time of that iteration is ~250ms with this patch, ~200 w/o. So that's a 25% slowdown in the inlined getters. If you feel that's significant and not lost in the noise when hitting these getters from JS I can write a JS test as well.

I fixed the alignment problem per your recommendation as well.

Comment 17

[Boris Zbarsky [:bzbarsky]]

When hitting them from JS, it may be lost in the noise.  The question is whether it's lost in the noise when iterating over a subtree using GetNextNode.

How do builds before/after this patch do on the last column of http://www.webkit.org/perf/slickspeed/ for example?

Comment 18

[Johnny Stenback (:jst)]

Attachment: Patch that shows orphan node memory usage in about:memory

Nick, the previous patch only adds the code to track orphan nodes, this patch adds the memory reporter stuff to show the memory usage of those orphan nodes.

This patch does *not* appear to catch much of the memory that's now reported in heap-unclassified when one repeatedly clicks Update in about:memory. I'm not sure what's going on there... If I instead *reload* about:memory, then I see a huge increase in the memory used for orphan nodes (until the GC kicks in an cleans up), but not when I click Update. What's the difference there?

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 595329 [details] [diff] [review]
Patch that shows orphan node memory usage in about:memory

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

::: content/base/src/nsGenericElement.cpp
@@ +3759,5 @@
> +  NS_NAMED_LITERAL_CSTRING(kOrphanNodesSize,
> +                           "Memory used by orphan DOM nodes.");
> +  NS_NAMED_LITERAL_CSTRING(kOrphanRootNodeCount,
> +                           "Number of orphan root nodes.");
> +  nsCAutoString strprefix("explicit/dom/orphan-nodes/");

"dom" should be "dom+style" now.

Comment 20

[Nicholas Nethercote [inactive]]

> This patch does *not* appear to catch much of the memory that's now reported
> in heap-unclassified when one repeatedly clicks Update in about:memory. I'm
> not sure what's going on there... If I instead *reload* about:memory, then I
> see a huge increase in the memory used for orphan nodes (until the GC kicks
> in an cleans up), but not when I click Update. What's the difference there?

There's not much difference.  The onload handler does a small amount of observer stuff and then calls update(), which discards the old <body> node and generates a new one (with all the content beneath it).  If you hit the "Update" button it calls update() directly.  Could there be some recycling of DOM nodes if you don't reload?

See
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/content/aboutMemory.js#127 for the code in question.

Comment 21

[Johnny Stenback (:jst)]

(In reply to Boris Zbarsky (:bz) from comment #17)
> When hitting them from JS, it may be lost in the noise.  The question is
> whether it's lost in the noise when iterating over a subtree using
> GetNextNode.
> 
> How do builds before/after this patch do on the last column of
> http://www.webkit.org/perf/slickspeed/ for example?

This seems to cost us ~3% in the last column on that page. That's based on an average of 5 runs where a build with the patch applied actually showed the fastest result. So yes, I think there's likely a measurable cost here, but a very small one.

Comment 22

[Boris Zbarsky [:bzbarsky]]

OK.  I think I can live with a 3% loss there....

Comment 23

[Johnny Stenback (:jst)]

Attachment: Updated patch for tracking orphan nodes.

This fixes the things that njn and bz pointed out...

Comment 24

[Johnny Stenback (:jst)]

Attachment: Patch that shows orphan node memory usage in about:memory

This fixes a rather embarrassing bug in the memory reporter where it would only count the size of orphan nodes, but not include the size of their children.

With that fixed, the amount of memory used by orphan nodes in about:memory while clicking the Update button goes up, and heap-unclassified stays constant (relatively speaking). Thanks for bringing that testcase to my attention njn!

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 596078 [details] [diff] [review]
Patch that shows orphan node memory usage in about:memory

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

jst, I assume there's never any interlinking between the orphaned nodes and the non-orphaned nodes?  We definitely want to avoid double-reporting any nodes.

::: content/base/src/nsGenericElement.cpp
@@ +3858,5 @@
> +  PRInt64 size = n->SizeOf();
> +  for (nsIContent* node = n->GetFirstChild(); node;
> +       node = node->GetNextSibling()) {
> +    size += GetNodeSize(node);
> +  }

It's worth mentioning that I'm working on bug 723799 at the moment and if I land that first this'll need to change a bit... SizeOf() will become SizeOfIncludingThis(), which returns |size_t|, and needs a |nsMallocSizeOfFun| arg.

Not too hard, I'm happy to answer questions if it happens.

Comment 26

[Johnny Stenback (:jst)]

(In reply to Nicholas Nethercote [:njn] from comment #25)
> jst, I assume there's never any interlinking between the orphaned nodes and
> the non-orphaned nodes?  We definitely want to avoid double-reporting any
> nodes.

The only interlinking is for XBL and native anonymous content, but the size of that content does not get included in the size of the DOM tree that it is associated with. So at this point there is no double counting here AFAIK.

> It's worth mentioning that I'm working on bug 723799 at the moment and if I
> land that first this'll need to change a bit... SizeOf() will become
> SizeOfIncludingThis(), which returns |size_t|, and needs a
> |nsMallocSizeOfFun| arg.
> 
> Not too hard, I'm happy to answer questions if it happens.

Good to know, but yeah, this should be easy to adapt to that change.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 596075 [details] [diff] [review]
Updated patch for tracking orphan nodes.


>-  nsIContent* GetNextSibling() const { return mNextSibling; }
>-  nsIContent* GetPreviousSibling() const { return mPreviousSibling; }
>+  nsIContent* GetNextSibling() const
>+  {
>+    return IsOrphan() ? nsnull : mNextSibling;
Just curious, would NS_UNLIKELY(IsOrphan()) ? nsnull : mNextSibling;
be any faster? Probably not.


>+++ b/content/base/src/nsAttrAndChildArray.cpp
>@@ -232,13 +232,19 @@ nsAttrAndChildArray::TakeChildAt(PRUint3
>   PRUint32 childCount = ChildCount();
>   void** pos = mImpl->mBuffer + AttrSlotsSize() + aPos;
>   nsIContent* child = static_cast<nsIContent*>(*pos);
>+
>+  NS_ASSERTION(!child->IsOrphan(), "Child should not be an orphan here");
>+
You could use MOZ_ASSERT here and elsewhere in .cpp files.
(I don't want to use it in nsI*.h interface files, since including it
can be ugly in some cases.)


> nsINode::~nsINode()
> {
>   NS_ASSERTION(!HasSlots(), "nsNodeUtils::LastRelease was not called?");
>+
>+  NS_ASSERTION(IsOrphan(), "Node should be orphan by the time it's deleted!");
>+
>+  if (IsOrphan()) {
>+    mPreviousOrphanNode->mNextOrphanNode = mNextOrphanNode;
>+    mNextOrphanNode->mPreviousOrphanNode = mPreviousOrphanNode;
>+  }
Since something is horribly wrong if nsINode isn't orphan here,
I'd just remove the if check and replace
NS_ASSERTION with stronger MOZ_ASSERT


>+nsINode::Init()
>+{
>+  // Allocate static storage for the head of the list of orphan nodes
>+  static char orphanNodeListHead[sizeof(nsINode) + 8];
>+  char *aligned =
>+    &orphanNodeListHead[0] + (size_t(&orphanNodeListHead[0]) % 8);
We don't have any macro for this kind of hackery?

>+
>+  sOrphanNodeHead = (nsINode *)aligned;
Use C++ casting.


>+  sOrphanNodeHead->mNextOrphanNode = sOrphanNodeHead;
>+  sOrphanNodeHead->mPreviousOrphanNode = sOrphanNodeHead;
>+
>+  sOrphanNodeHead->mFirstChild = (nsIContent *)0xdeadbeef;
>+  sOrphanNodeHead->mParent = (nsIContent *)0xdeadbeef;
ditto


>@@ -2266,6 +2272,9 @@ nsGlobalWindow::SetNewDocument(nsIDocume
>   if (!aState) {
>     if (reUseInnerWindow) {
>       if (newInnerWindow->mDocument != aDocument) {
>+        newInnerWindow->mDocument->MarkAsOrphan();
>+        aDocument->MarkAsNonOrphan();
>+
I assume there is a null check both for 
newInnerWindow->mDocument and aDocument
-U 8 would create better diffs


>   SetDocument(aDocument);
> 
>+  NS_ASSERTION(aDocument->IsOrphan(), "New document must be orphan!");
>+  aDocument->MarkAsNonOrphan();
Same here. aDocument is certainly non-null, right?

Comment 28

[Johnny Stenback (:jst)]

Attachment: Updated patch for tracking orphan nodes.

Updated per Olli's review, carrying r+ forward.

Comment 29

[Johnny Stenback (:jst)]

First part landed in inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/67d25814ec44

Comment 30

[Boris Zbarsky [:bzbarsky]]

Looks like about a 4% hit on Dromaeo (CSS) (aka "jQuery gunk"), at least for the non-PGO builds on Linux and Windows...  :(

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

That sounds way too much.
Can we think of any other way to track orphan DOM nodes?
Mark all JS gray and run traceAll CC without unlink? That would be slow, but should
capture pretty almost all the DOM nodes.

Comment 32

[Andrew McCreight [:mccr8]]

Maybe iterate over all wrapped natives (held by XPConnect) and look for nodes, see if they are in documents, if not, find the topmost node?  You could even use the DOM tree marking bits to avoid repeats.  Basically, a variant of nsGenericElement::CanSkip.  It wouldn't catch everything, I assume, but probably the most interesting case of these are things being leaked by webpages, and they probably hold them directly from JS.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #32)
> Maybe iterate over all wrapped natives (held by XPConnect) and look for
> nodes, see if they are in documents, if not, find the topmost node?
Wrappednatives and Objectholders (for slimwrappers)

Comment 34

[Andrew McCreight [:mccr8]]

If this regression really isn't showing up with PGO (it runs less often, so maybe there just aren't enough data points yet), then I wonder if just fussing with the code a bit might help.

Comment 35

[Boris Zbarsky [:bzbarsky]]

It's there with PGO too:

   Dromaeo (CSS) decrease 4.85% on Win7 Mozilla-Inbound
   Dromaeo (CSS) decrease 5.04% on XP Mozilla-Inbound
   Dromaeo (CSS) decrease 1.48% on Linux Mozilla-Inbound

and of course on Mac.

Comment 36

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/67d25814ec44

Comment 37

[Nicholas Nethercote [inactive]]

Will this be backed out?  AIUI, at the moment we have the worst of all worlds -- the perf regression is present, but the follow-up patch that exposes the orphan node counts in about:memory hasn't landed, so we're not getting anything in return for the regression...

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Backed out
https://hg.mozilla.org/mozilla-central/rev/5b8195516c11

Comment 39

[Andrew McCreight [:mccr8]]

Oddly the backout seems to have caused a regression of about 5% on Linux and Mac, in addition to the improvement on Windows.

Comment 40

[Justin Lebar (not reading bugmail)]

I'm honestly pretty surprised that an extra, predictable branch here would cause a 5% regression in any benchmark.  I wonder if something else is going wrong (e.g. the compiler isn't inlining nextSibling, for some reason).

Comment 41

[Justin Lebar (not reading bugmail)]

If we can't figure out a way to make this work, we could at least report the total number of orphaned nodes in about:memory.  This would let us tell whether astronomically high heap-unclassified is likely due to orphaned nodes.

Comment 42

[Andrew McCreight [:mccr8]]

I'll look at an approach using XPConnect wrapper tables.  I do something like this in one of my cycle collector dump analysis scripts[1] and it seemed to work pretty well.  I'd imagine that most of these are being kept alive by JS, so it should catch a lot of it.

[1] https://github.com/amccreight/heapgraph/blob/master/cc/dom_grouper.py

Comment 43

[Andrew McCreight [:mccr8]]

Turns out my idea is not super awesome because it will only find orphan DOMs with preserved wrappers in them.  Maybe I could just scan the entire JS heap for objects that point to C++...

Comment 44

[Andrew McCreight [:mccr8]]

I have a crude first pass at this.  I need to integrate jst's stuff from the first patch that shows which document the orphans are associated with.  But it seems to work nicely on khuey's example that creates a million orphan DOM nodes:

211,724,572 B (100.0%) -- explicit
├──157,404,664 B (74.34%) -- window-objects
│  ├──154,769,440 B (73.10%) ── orphans
│  ├────1,837,480 B (00.87%) -- top(chrome://browser/content/browser.xul, id=1)

This iterates over everything in the JS heap, looking for objects with nsISupports pointers in their private slot, which is done using js::Class flags.  We attempt to QI things we find to nodes.  If that succeeds, and they aren't in a document, then I use khuey's SubtreeRoot to get the root of the DOM.  If that's not in the hash set of visited roots, it calculates the size of the DOM tree, then puts the root in a hash table to avoid duplication.

Comment 45

[Andrew McCreight [:mccr8]]

Attachment: find orphans by examining all JS objects, WIP

I should fold this into the existing about:memory place that iterates over the entire JS heap, but that is in the JS engine, so it will require the usual callback dance.

Comment 46

[Andrew McCreight [:mccr8]]

First crack at turning this into a multi-reporter:

├──154,771,984 B (71.76%) -- dom+style
│  └──154,771,984 B (71.76%) -- orphan-nodes
│     ├──154,760,448 B (71.76%) ── https://bug702813.bugzilla.mozilla.org/attachment.cgi?id=574869 [2]
│     ├────────9,008 B (00.00%) ── chrome://browser/content/browser.xul [3]
│     ├────────2,320 B (00.00%) ── about:newtab
│     └──────────208 B (00.00%) ── about:memory?verbose

I need to be slicker and stick them under "window objects", with the relevant window, I guess.  Also I maybe should move this traversal into the JS engine so we only traverse the JS heap once.

Comment 47

[Andrew McCreight [:mccr8]]

Attachment: traverse JS heap to find orphan nodes, WIP

Slightly improved version that does not traverse the JS heap an additional time, by hooking into the existing JS heap traversal about:memory does.  I still need to cut out the string generating section, push it into the DOM, so it can generate strings identical to the window stuff so it groups nicely.

Comment 48

[Andrew McCreight [:mccr8]]

njn is going to refactor the JS memory reporter in bug 687724 that will make it easier for me to hook up the orphan memory properly.

Comment 49

[Nicholas Nethercote [inactive]]

mccr8, I can take this one over.

Comment 50

[Nicholas Nethercote [inactive]]

mccr8:  GetNodeSize() is called on the root node of every orphaned DOM sub-tree.  It looks like this:

static
PRInt64 GetNodeSize(nsINode *n)
{       
    PRInt64 size = n->SizeOfIncludingThis(Foo);
    for (nsIContent* node = n->GetFirstChild(); node;
         node = node->GetNextNode(n)) {
        size += node->SizeOfIncludingThis(Foo);
    }   
    return size;
}

This measures the root node and the root node's children, but doesn't go any further down the sub-tree.  I suspect the body of the loop was meant to be this?

        size += GetNodeSize(node);

Is that right?

Comment 51

[Johnny Stenback (:jst)]

nsINode::GetNextNode() is effectively (sort of) a tree iterator, so mccr8's loop will hit every node in n.

Comment 52

[Nicholas Nethercote [inactive]]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #51)
> nsINode::GetNextNode() is effectively (sort of) a tree iterator, so mccr8's
> loop will hit every node in n.

Ah, yep.  Thank!

Comment 53

[Nicholas Nethercote [inactive]]

Attachment: Track memory used by orphan DOM nodes.

Here's a revised version.  The key change is that the orphan nodes are now
measured and reported by the JS compartment reporter, along with the other
per-compartment measurements.

This is arguably not as nice as grouping them with the other DOM
measurements.  But it's much easier to implement this way, and it's not
*completely* crazy because these DOM nodes are only accessible from JS
objects.  I think it's good enough, because it'll still show up under the
same top window (i.e. tab).

Comment 54

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #53)
> The key change is that the orphan nodes are now
> measured and reported by the JS compartment reporter, along with the other
> per-compartment measurements.

I'm not sure what you mean by this, as I'm not hip to the about:memory lingo.  Do you mean that orphan memory will show up under top(...)/window(...)/js/compartment(...)/orphan-dom-nodes instead of something like top(...)/window(...)/dom/orphan-dom-nodes?  That sounds reasonable enough.  It is a little weird, but as you say it does make some amount of sense, and it will show the tab it is from which is the important thing.  It is too bad the about:memory reporter thing doesn't support relative paths, because then you could do "../dom" ;)

Does this affect any sort of other-measurements?  Specifically, is there any place where you use some kind of total that includes all memory allocated by the JS engine, then including the DOM nodes stuff as a subheading of that will obviously be bad.  But probably all the JS engine totals are built bottom-up?  It will also produce slightly incomplete results if two compartments point to the same DOM tree, but that doesn't really matter.

Did you try this out with one of the tons-of-orphans test cases we have floating around to make sure it is sane?  If you don't have one handy I can find one for you to use.

One problem with this patch (from the part I wrote...) is that it only covers one kind of JS to C++ pointers.  You can see all of the cases in NoteGCThingXPCOMChildren in nsXPConnect.  I think this doesn't matter now, because we're only looking at nodes, but in the grim future nodes will use the new DOM bindings and this will have to be fixed, and we'll have to push all JS object across the XPConnect barrier, but no need to do that now.  Just something to keep in mind.

Comment 55

[Nicholas Nethercote [inactive]]

Here's an example.  It shows up on under the "js" sub-tree for the relevant tab.

├──20,625,760 B (27.66%) -- window-objects
│  ├──15,215,552 B (20.41%) -- top(about:memory?verbose, id=6)
│  │  ├──14,319,976 B (19.20%) -- active/window(about:memory?verbose)
│  │  │  ├──11,792,616 B (15.81%) -- js/compartment([System Principal], about:memory?verbose)
│  │  │  │  ├───5,750,784 B (07.71%) -- gc-heap
│  │  │  │  │   ├──2,791,424 B (03.74%) ── strings
│  │  │  │  │   ├──2,597,424 B (03.48%) -- objects
│  │  │  │  │   │  ├──2,192,688 B (02.94%) ── non-function
│  │  │  │  │   │  └────404,736 B (00.54%) ── function
│  │  │  │  │   ├────163,720 B (00.22%) -- shapes
│  │  │  │  │   │    ├───73,640 B (00.10%) ── dict
│  │  │  │  │   │    ├───47,040 B (00.06%) ── base
│  │  │  │  │   │    └───43,040 B (00.06%) ── tree
│  │  │  │  │   ├─────98,672 B (00.13%) ── unused-gc-things
│  │  │  │  │   ├─────81,592 B (00.11%) ── arena-admin
│  │  │  │  │   ├─────11,664 B (00.02%) ── scripts
│  │  │  │  │   └──────6,288 B (00.01%) ── sundries
│  │  │  │  ├───5,101,736 B (06.84%) ── orphan-dom-nodes
│  │  │  │  ├─────424,872 B (00.57%) ── string-chars
│  │  │  │  ├─────263,008 B (00.35%) ── analysis-temporary
│  │  │  │  ├──────90,208 B (00.12%) -- shapes-extra
│  │  │  │  │      ├──43,264 B (00.06%) ── compartment-tables
│  │  │  │  │      ├──33,920 B (00.05%) ── dict-tables
│  │  │  │  │      └──13,024 B (00.02%) ── tree-tables
│  │  │  │  ├──────64,176 B (00.09%) -- objects
│  │  │  │  │      ├──44,976 B (00.06%) ── elements
│  │  │  │  │      └──19,200 B (00.03%) ── slots
│  │  │  │  ├──────31,696 B (00.04%) ── type-inference/script-main
│  │  │  │  ├──────30,744 B (00.04%) ── script-data
│  │  │  │  ├──────18,800 B (00.03%) ── mjit-data
│  │  │  │  └──────16,592 B (00.02%) ── other-sundries


There's also the js-main-runtime summary sub-tree under "Other Measurements", where it is included.  You're right that including them under there is weird.  Hmm.  Here's an example:

41,580,304 B (100.0%) -- js-main-runtime
├──31,174,592 B (74.97%) -- compartments
│  ├──17,633,280 B (42.41%) -- gc-heap
│  │  ├───4,838,512 B (11.64%) -- objects
│  │  │   ├──2,796,752 B (06.73%) ── non-function
│  │  │   └──2,041,760 B (04.91%) ── function
│  │  ├───4,838,248 B (11.64%) ── unused-gc-things
│  │  ├───4,065,760 B (09.78%) ── strings
│  │  ├───2,388,248 B (05.74%) -- shapes
│  │  │   ├──1,432,640 B (03.45%) ── tree
│  │  │   ├────528,248 B (01.27%) ── base
│  │  │   └────427,360 B (01.03%) ── dict
│  │  ├───1,142,496 B (02.75%) ── scripts
│  │  ├─────258,048 B (00.62%) ── arena-admin
│  │  ├─────101,808 B (00.24%) ── type-objects
│  │  └─────────160 B (00.00%) ── sundries
│  ├───5,110,328 B (12.29%) ── orphan-dom-nodes
│  ├───...


I tested it with about:memory itself -- if you hit any of the update buttons at the bottom, the old DOM is discarded and a new one generated.  If you can remind me of any sites that caused orphan nodes I'll happily test with them (except for irccloud, because IIRC I never managed to show the bad leak with it).

Comment 56

[Nicholas Nethercote [inactive]]

Attachment: Do the paths properly.

This patch applies on top of the previous one, though I'll fold them
together before landing.

It changes things so that orphan nodes are reported in "dom" sub-trees.
Orphan nodes are still measured by the JS multi-reporter on a
per-compartment basis.  However, ReportCompartmentStats now takes two paths:
|cJSPathPrefix| is used for most of the per-compartment numbers, and
|cDOMPathPrefix| is used just for the "orphan-nodes" number.

Various bits of the path construction plumbing had to change to support
this, including the usage of CompartmentStats::{extra1,extra2}.

It's not the nicest patch I've ever written, but I think it's good enough.

Comment 57

[Nicholas Nethercote [inactive]]

I forgot to mention:  I tested it with the "synthetic benchmark" attachment in bug 702813 and got 150MB of orphan nodes, which sounds good.

Comment 58

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #57)
> I forgot to mention:  I tested it with the "synthetic benchmark" attachment
> in bug 702813 and got 150MB of orphan nodes, which sounds good.

Was your heap-unclassified reasonable?

Comment 59

[Nicholas Nethercote [inactive]]

> > I forgot to mention:  I tested it with the "synthetic benchmark" attachment
> > in bug 702813 and got 150MB of orphan nodes, which sounds good.
> 
> Was your heap-unclassified reasonable?

8% :)

Comment 60

[Andrew McCreight [:mccr8]]

Comment on attachment 640483 [details] [diff] [review]
Do the paths properly.

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

Thanks for taking this bug.

::: dom/workers/WorkerPrivate.cpp
@@ +144,5 @@
> +    nsCString cJSPathPrefix(mRtPath);
> +    cJSPathPrefix += js::IsAtomsCompartment(c)
> +                   ? NS_LITERAL_CSTRING("compartment(web-worker-atoms)/")
> +                   : NS_LITERAL_CSTRING("compartment(web-worker)/");
> +    cstats->extra1 = strdup(cJSPathPrefix.get());

Sorry if this is a stupid question, but does extra1 need to be freed?  Doesn't strdup do a malloc somewhere?  I don't know if it is being freed somewhere.

Comment 61

[Nicholas Nethercote [inactive]]

> Sorry if this is a stupid question, but does extra1 need to be freed? 

Not a stupid question!  You're right, it does need to be freed.  I'll add a destructor for WorkerJSRuntimeStats.

Comment 62

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57ee1a91e771

Comment 63

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/57ee1a91e771

Comment 64

[Johnny Stenback (:jst)]

Woohoo! Thanks guys for driving this in!