The patch doesn't leak all the time, and I uploaded it to tryserver. https://tbpl.mozilla.org/?tree=Try&rev=b8d4d0635d9d It is hackish to use nsXPCOMCycleCollectionParticipant that way, but virtual method calls aren't very cheap. Based on my testing there are lots of DOM nodes which are in CC-generation documents, and yet don't have any JS Object in the wrappercache. I could be missing something important here :)

Is NS_INTERFACE_MAP_BEGIN the chunk of code executed during a QI? The basic idea is to never add a node to the graph that is in a currently displayed document? So the only effect this should have is to completely skip nodes for which we currently add, but skip all the children? Looks interesting! This could cut out a fair number of nodes. I'll try it out locally today. The comment at the end doesn't seem accurate to me. :) If skipCycleCollection is true, then it will always be skipped until shutdown. Furthermore, whether an object is skipped or not can vary from collection to collection, and that's okay. Is it possible for XPConnect roots to ever satisfy the condition? Or should those always have JS children, so it doesn't matter?

(In reply to Andrew McCreight [:mccr8] from comment #1) > Is NS_INTERFACE_MAP_BEGIN the chunk of code executed during a QI? Isn't that just beautiful hack ;) > The basic > idea is to never add a node to the graph that is in a currently displayed > document? If it doesn't have a JSObject in the wrapper > So the only effect this should have is to completely skip nodes > for which we currently add, but skip all the children? The only effect should be that the object isn't added to the graph, since traverse shouldn't do anything anyway. But apparently that doesn't quite work. There are some leaks. Mochitest-4 is leak free though :) > Looks interesting! This could cut out a fair number of nodes. I'll try it > out locally today. This certainly should cut out many nodes, like thousands in some cases. Of course those nodes wouldn't really traverse anything anyway, but graph creation does spend quite a bit time in hashtable. > Is it possible for XPConnect roots to ever satisfy the condition? Or should > those always have JS children, so it doesn't matter? Not sure. The patch doesn't quite work yet properly, but I think the approach should be reasonable, since it is after all just trying to prevent adding certainly black nodes - which should affect to other nodes either, to graph.

Interesting, the leak problems seem to come from XUL.

Ah, QI hack doesn't get inherited everywhere. But that shouldn't cause leaks.

I had been working on this in the past. Here's that patch, I just pushed this to try and it seems to work.

This is still quite ugly code-vice, but seems to reduce graph size significantly.

...and also speed up some bad CC cases 50x or so. But of course there can be other still bad cases.

More comments, handles also text nodes.

With the patch the both "high" cases in https://bug702813.bugzilla.mozilla.org/attachment.cgi?id=575341 stay pretty reasonable.

(ofc the patch hasn't even run the tryserver tests yet, so perhaps it is leaking like hell :p. But anyway, I'm using the patch now in my local-opt-ff-build-for-normal-browsing)

boo, leaking in mochitest-2 and -3.

Er, maybe it is a crash after all...

Ok, I wasn't think about textnode's inside attributes :(

fixes the crash

Just some fixes to comments

attachment 577025 [details] [diff] [review] and attachment 577031 [details] [diff] [review] pass tests on try server, which is expected since the patch really just tries to minimize graph size by removing certainly black nodes from purple buffer. And in case when there is no current document, adding a is-in-black-tree to js-gray objects, so that only script objects are traversed (similarly what is done currently when current document is in CCgeneration. I believe shrinking graph size But, - is pre-traversing DOM subtrees fast enough? It is hard to make it faster - Does pre-traverse slow down the case when subtree will be deleted significantly? I haven't seen this happening, but some slow down should happen. Lazy unrooting could reduce the overall effect. One bad case might be some insane innerHTML+= loop, where lots of disconnected, non-black subtrees can be created. - Are there cases when gCCBlackMarkedNodes ends up being enormous, and does such case slow down CC significantly?

And somehow I noticed peterv's patch only yesterday and looked it now. It has very similar idea, but is limited to nodes which are in a document (GetCurrentDoc() != null). So it wouldn't help with https://bug702813.bugzilla.mozilla.org/attachment.cgi?id=575341

Oh, and it doesn't seem to optimize data documents.

(In reply to Olli Pettay [:smaug] from comment #20) > - is pre-traversing DOM subtrees fast enough? It is hard to make it faster The 'high' cases in https://bug702813.bugzilla.mozilla.org/attachment.cgi?id=575341 actually indicate that traversing could be fast enough. We end up traversing 1000000+ nodes in the pre-traverse phase, yes CC time stays quite low. Without the optimization we end up adding all those 1000000 nodes to graph and traversing them there using slow virtual calls.

This adds two new tests. One is creating lots of shallow non-black disconnected subtrees. In that case the patch doesn't seem to slow down CC. The other is creating lots of deep disconnected non-black subtrees and in that case CC does slow down as expected.

But seems like there can be more optimizations. In those two innerHTML cases subtrees are only owned by the subtree members itself, and such subtrees seems to be quite common (in cases when subtree isn't in CCGeneration document).

after browsing some random sites (gmail, facebook, some Finnish news sites etc), 40% of not-in-CCGeneration subtrees are owned only by the members of the subtree. When running tests like dromaeo, both kinds of cases are triggered often. If the tree is owned only by itself (and isn't gray), we could release the subtree separately. Basically skip traversing/marking phase and move all its members to root/unlink/unroot phase. Shouldn't be too hard.

Comment on attachment 577031 [details] [diff] [review] wip4 + beforetraverse for documents But even without such optimizations, what do people think about this? It does kind of move part of CC handling to DOM, but DOM just happens to know its state better and faster than CC. Or, does anyone have other ideas how to reduce CC times in case there are huge not-in-document subtrees?

You can use xpc_IsGrayGCThing instead of xpc_GCThingIsGrayCCThing when you know AddToCCKind will be true. In IsBlackNode and IsGrayNode, you are passing in JSObjects, which have CCKind, so you can use the faster function. Is there any danger of traversing to a parent that doesn't actually have the node as a child? Or does the unoptimizable test catch that? Why is unpurpling delayed instead of being done immediately? I guess in the case where you find a black node somewhere in the subtree, you end up skipping the unpurpling. Hmm. Can gray nodes always be unpurpled? If they have to be traversed by the CC no matter what, then I'd think they'd be traversed AddXPConnectRoots, so they don't have to be purple. I don't know if that would matter.

(In reply to Andrew McCreight [:mccr8] from comment #28) > You can use xpc_IsGrayGCThing instead of xpc_GCThingIsGrayCCThing when you > know AddToCCKind will be true. In IsBlackNode and IsGrayNode, you are > passing in JSObjects, which have CCKind, so you can use the faster function. Ah, thanks. > > Is there any danger of traversing to a parent that doesn't actually have the > node as a child? Or does the unoptimizable test catch that? unoptimizable thing is for that case yes. > Why is unpurpling delayed instead of being done immediately? I guess in the > case where you find a black node somewhere in the subtree, you end up > skipping the unpurpling. We can save lots of iterating if we notice early that the whole document is black. Only those nodes, which are purple, need to be checked, and that happens when their BeforeTraverse is called. > Hmm. Can gray nodes always be unpurpled? In the patch gray nodes are never unpurpled. I didn't want to change that behavior since peterv explicitly add code to always traverse the script objects of gray suspected objects. > If they have to be traversed by > the CC no matter what, then I'd think they'd be traversed AddXPConnectRoots, > so they don't have to be purple. I don't know if that would matter.

I used this patched version all day today. I had the AMCtv website open for about half of the day, and IRCcloud the entire time. heap-unclassified slowly creeped up, but only got to about 33%. After closing all tabs, I have two small zombie compartments: https://crash-stats.mozilla.com/products/Firefox https://mail.google.com/mail/?tab=mm#inbox I don't know if that is related to this patch or not. I don't have any particularly crazy addons enabled, but who knows.

ignore the bugzilla compartment, I reopened that one after closing them all.

The crash-stats compartment went away, possible after I did a CC dump, but the gmail compartment is still around.

The nsDocument for gmail is being held alive for this reason: 0x164188030 [JS Object (XPCWrappedNative_NoHelper) (global=117730a88)] --[xpc_GetJSPrivate(obj)]-> 0x156a6bc50 [XPCWrappedNative] --[mIdentity]-> 0x12487db00 [nsEditor] --[mTxnMgr]-> 0x1256563d0 [nsTransactionManager] --[transaction stack mQue[i]]-> 0x1561d4d90 [nsTransactionItem] --[mTransaction]-> 0x12ca9d800 [EditTxn] --[mChildren[i]]-> 0x130d4fbc0 [EditTxn] --[mElement]-> 0x12ea8c100 [nsGenericDOMDataNode] --[mNodeInfo]-> 0x120bf5f20 [nsNodeInfo ([none]) #text] --[mOwnerManager]-> 0x12435c9c0 [nsNodeInfoManager] --[mDocument]-> 0x12a267000 [nsDocument normal (xhtml) https://mail.google.com/mail/?tab=mm#inbox] Root 0x164188030 is a marked GC object. Doesn't mean much to me. That XPCWrappedNative NoHelper is being held alive by an object that is stored in a marked JSObject (ChromeWindow).

Thanks for the explanations. (In reply to Olli Pettay [:smaug] from comment #29) > > Hmm. Can gray nodes always be unpurpled? > In the patch gray nodes are never unpurpled. > I didn't want to change that behavior since peterv explicitly add code to > always traverse the script objects of gray suspected objects. Right, we definitely need to do that, but I think that these objects will be traversed by the CC even if they aren't purple (because we must do that anyways), thus it doesn't matter if they are purple or not. I could be wrong. We'd still have to make sure to do the script part of the Traverse. I don't think it is a big deal, I was just wondering if it might simplify the code a bit.

Olli, is this still relevant?

Not really.