712170 - [CC] Don't add nsJSEventListeners to the CC graph that have no grandchildren

Status: RESOLVED INCOMPLETE

(Core :: DOM: Events, defect)

Description

[Andrew McCreight [:mccr8]]

After bug 702036, nsJSEventListeners don't drag in quite as many nodes.  But it is still fairly common to see hundreds of them pointing to a particular JSContext.  If GetOutstandingRequests() is non-zero for the context, we root it.  We should be able to take advantage of this at least some of the time to not add the event listeners to the graph.

Are the event listeners held alive by the context?  If so, we can just skip the listener if the context has an outstanding request.  If not, we have to do some more detailed analysis (maybe it will only be okay to skip it if the context is the only out edge of the listener?).

There is also the usual disclaimer that we can't skip the node if there are any script objects, but that doesn't seem common.

We should also see how common it is for these contexts to have outstanding requests.

Comment 1

[Andrew McCreight [:mccr8]]

This isn't quite right.  I think the thing to look for is to see if the nsJSEventListener is necessarily not part of a cycle.  They only have two children, an nsGlobalWindow and a JSContext.  These children often have no children, in which case we don't need to add the nsJSEventListener to the graph.  I'm looking at the latter in bug 705371.

Comment 2

[Andrew McCreight [:mccr8]]

The way to implement this is probably to add a CanSkip() to nsGlobalWindow, JSContext, then add one to nsJSEventListener that checks those of its two children.  Hopefully they won't be very expensive.

Comment 3

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

Not sure whether it is about this bug, but currently about 50% of my
CC graph is nsJSEventListener objects. The only reason they are there is that
xpconnect adds pretty much all the mJSHolders to the graph.

I could add CanSkipThis to nsJSEventListener, but something needs to keep the cx alive.

Comment 4

[Andrew McCreight [:mccr8]]

Do the children of the event listeners have children?  In graphs I saw, the global windows and JSContexts that were children of them mostly had no children and thus could be avoided themselves.

Comment 5

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

There is only mContext (since the js objects are marked black before CC).
mContext has only mGlobalObjectRef and mContext, and mGlobalObjectRef has nothing.
So, ok, checking whether mGlobalObjectRef is in CCGeneration or black should be enough.

Comment 6

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

https://tbpl.mozilla.org/?tree=Try&rev=3e698ab57508 removes jseventlisteners from the CC graph.
Unfortunately I don't see much difference in CC times. (Though, I've been opening more and more
tabs, currently 190+)

Comment 7

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

Hmm, or perhaps there is some improvement. Since after closing some tabs so that there are
about 100~ tabs, I'm getting CC times 15-25ms (when there hasn't been page (un)loads).

Comment 8

[Andrew McCreight [:mccr8]]

CC graphs are pretty small now. I don't think this is a big deal any more.