Andrew McCreight [:mccr8] Assignee
	Description • 13 years ago

In Bug 700645, there are millions of DOM nodes that the cycle collector sees but are invisible to the memory reporter.  bz says that the DOM memory reporter only visits nodes reachable from extant windows, which will miss disconnected subtrees.  We should be able to piggyback onto the cycle collector to report these nodes.

I'm assuming the DOM memory reporter uses, or could use, a hash table to avoid double counting nodes.  I'm also assuming the DOM memory reporter takes some set of DOM nodes and traces out from them.

One approach is to add the cycle collector roots (nodes in the purple buffer and things reported by TraceXPConnectRoots) that are DOM nodes as roots for the memory reporter traversal.  This will miss nodes indirectly visited by the CC, such as those held by JS.

Another approach would be to run a full cycle collection, with a custom listener that defines noteRefCountedObject to record any DOM nodes encountered.  These additional DOM nodes would be treated as roots for traversal by the DOM memory reporter.  Heavyweight, but may be easier to implement.  Could perturb results, cause slowdowns (in bug 700645 the CC takes multiple seconds, so loading about:memory will also take at least that long!).

This still won't catch all DOM nodes, but at least it will get visibility on DOM nodes that are killing the cycle collector.

Relevant comment from bug 700645:
(In reply to Boris Zbarsky (:bz) from comment #24)
> So it looks like the DOM memory reporter walks all extant windows and for
> each one it walks the window's document (if any) and for each document it
> walks the non-anonymous nodes in that document.
> 
> That's going to miss disconnected subtrees of various sorts.  It might also
> miss documents whose inner window has already gone away (assuming this can
> happen, of course).
> 
> The question is what we can do about it.  Adding every DOM subtree root to
> some sort of global hashset sounds expensive.  Can we assume that we only
> need to worry about stuff with JS wrappers to find subtrees (not subtree
> roots, note!) and walk some sort of xpconnect hashtables for now?  At least
> until the new DOM bindings are done?

	Justin Lebar (not reading bugmail)
	Comment 1 • 13 years ago

Would it be possible and desirable to report these nodes separately from the current DOM reporter?  In this case, such a split would let us see that the millions of DOM nodes in question aren't attached to a window.

	Andrew McCreight [:mccr8] Assignee
	Comment 2 • 13 years ago

That could be interesting.  It would still have to be entangled with the current implementation to avoid double counting.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 13 years ago

(In reply to Andrew McCreight [:mccr8] from comment #0)
> In Bug 700645, there are millions of DOM nodes that the cycle collector sees
> but are invisible to the memory reporter.  bz says that the DOM memory
> reporter only visits nodes reachable from extant windows, which will miss
> disconnected subtrees. 
If that is really the case, the reporter will miss also all the XHR/DOMParser/document.implementation.createDocument created documents. And that could be a lot of documents in
some web apps.

	Mounir Lamouri (:mounir)
	Comment 4 • 13 years ago

(In reply to Olli Pettay [:smaug] from comment #3)
> (In reply to Andrew McCreight [:mccr8] from comment #0)
> > In Bug 700645, there are millions of DOM nodes that the cycle collector sees
> > but are invisible to the memory reporter.  bz says that the DOM memory
> > reporter only visits nodes reachable from extant windows, which will miss
> > disconnected subtrees. 
> If that is really the case, the reporter will miss also all the
> XHR/DOMParser/document.implementation.createDocument created documents. And
> that could be a lot of documents in
> some web apps.

The original plan made was to have a hashtable listing all documents so we would have been able to count documents and nodes disconnected from a window. Maybe we should do that? (I can try to find some cycles next week otherwise someone can jump on it)

	Andrew McCreight [:mccr8] Assignee
	Comment 5 • 13 years ago

So, it looks like my assumptions were wrong.  The DOM memory reporter for a window basically iterates over all children in the document, and gets their sizes individually.  I suppose if we do a full CC we could only count nodes that aren't in a currently displayed document (there's code to do this already) and hope that won't double count anything.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 13 years ago

> If that is really the case, the reporter will miss also all the XHR/DOMParser
> /document.implementation.createDocument created documents. 

Yes, the current reporter misses those.

	Nicholas Nethercote [inactive]
	Comment 7 • 13 years ago

> I suppose if we do a full CC we could only count nodes
> that aren't in a currently displayed document (there's code to do this
> already) and hope that won't double count anything.

DMD can detect double-reported bytes, once it knows about the relevant reporters (which I'm working on making easier) but I'd rather if we had a good idea up-front about avoiding double-counting rather than just hoping.

	Andrew McCreight [:mccr8] Assignee
	Comment 8 • 13 years ago

I looked at this a little, but I'm not sure it can be made to work simply by running a cycle collection with a special listener.  The problem is that the listener is passed ref counted classes (like JSContext and nsXBLBinding) that aren't nsISupports, so there's no safe way to turn the void* pointer into an nsINode etc.  I can hack around this by looking at their text description and manually ruling things out, but that's not something we'd want to ship.

Possibly this could be worked around a bit by creating a more strongly typed listener interface, but that could be tricky, too.

jst had another idea for attacking this problem by tracking DOM nodes that aren't owned by documents, so maybe that will work better.

	Andrew McCreight [:mccr8] Assignee
	Comment 9 • 13 years ago

Here's my experimental patch so you can see what I was going for.  It crashes immediately when you do about:memory.

	Andrew McCreight [:mccr8] Assignee
	Comment 10 • 13 years ago

As I said, I think this won't work without restructuring of the cycle collector.  Bug 704623 can look at alternate approaches.