When I was investigating why TechCrunch cycle collects every 5 seconds, I noticed that there are around 480 childless nsComputedDOMStyles in the purple buffer, out of around 530 things in the purple buffer total. If we can remove these from the purple buffer, this should reduce how often we trigger a cycle collection on TechCrunch. nsDOMComputedStyle has only a single field that it tells the CC about: nsCOMPtr<nsIContent> mContent Either this field is null, or it points to a live DOM node thing. I was thinking we could do something like saying that DOMStyle can be skipped when its field can be skipped.

I found this using bug 726252. With that in place, I was able to see which nodes were purple when I visualized the graph, then I noticed that the bulk of the purple nodes were of this one class, and that they had no children. With the patch here, the CC basically does not run at all when sitting idle on TechCrunch.com. It was at least two minutes between CCs when I got bored and scrolled the page a bit, which caused a CC.

(In reply to Andrew McCreight [:mccr8] from comment #0) > nsDOMComputedStyle has only a single field that it tells the CC about: > nsCOMPtr<nsIContent> mContent > Either this field is null, or it points to a live DOM node thing. I was > thinking we could do something like saying that DOMStyle can be skipped when > its field can be skipped. How does that imply that the nsComputedDOMStyle is alive?

nsComputedDOMStyle doesn't add anything interesting to graph in that case. This is basically the green-ness check in CC, but done before actual CC run.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3) > How does that imply that the nsComputedDOMStyle is alive? It doesn't imply it is alive, it implies that it isn't part of a garbage cycle. Though the correctness of this optimization may depend on any references nsComputedDOMStyle being unlinked when the object holding onto the style is thrown out by the CC.

(In reply to Andrew McCreight [:mccr8] from comment #5) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3) > > How does that imply that the nsComputedDOMStyle is alive? > > It doesn't imply it is alive, it implies that it isn't part of a garbage > cycle. Except that it doesn't. The nsComputedDOMStyle may be in a garbage cycle with its own wrapper, no? The real question here is why is nsComputedDOMStyle wrapper cached? We return a new one every time ...

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6) > Except that it doesn't. The nsComputedDOMStyle may be in a garbage cycle > with its own wrapper, no? The only out edge of the style is mContent. It does not have an edge to its wrapper. Maybe that's a bug, but I think maybe this is one of those weird classes that is a wrapper cache that doesn't allow wrapper preservation, so that's why it is okay? I think there's some kind of check for that in Debug Builds that Peter added a few years ago. I was hitting it in my patch to use wrapper preservation for all weak map keys that are wrapper caches, and I think this was one of those classes. I'm not sure why that is the case.

This is the changeset that made nsComputedDOMStyle not visit its wrapper, in case anybody is curious: http://hg.mozilla.org/mozilla-central/rev/ed6b195bae6d

If these style nodes are owned by DOM nodes or whatever, then another approach here would be to unpurple them when we unpurple the DOM node, or something like that.

A more conservative option is to modify CanSkip and CanSkipInCC, but leave CanSkipThis alone. That will remove an nsComputedDOMStyle from the purple buffer, but it will still add it to the graph if some other node reaches it. That should avoid keep the style from making the CC run all the time, but avoid problems with incomplete unlinking. I'll see how well that works.

> The real question here is why is nsComputedDOMStyle wrapper cached? Good question. That was added in bug 500850 and I had assumed it was needed because something was making assumptions about CSS declarations all acting the same and we wanted element.style to be wrappercached...

nsDOMCSSAttributeDeclaration also shows up fairly frequently as a childless node in the purple buffer, on TC.

Comment on attachment 597295 [details] [diff] [review] minor cleanup > > NS_IMPL_CYCLE_COLLECTION_1(nsComputedDOMStyle, mContent) Could you add some comment here that if wrappercache handling is changed, also canSkip* handling should be changed. >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsComputedDOMStyle) >+ return !tmp->mContent || nsGenericElement::CanSkip(tmp->mContent); >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END CanSkip takes now 2 parameters. nsGenericElement::CanSkip(tmp->mContent, true) >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_END >+ >+ 1 extra newline.

(In reply to Olli Pettay [:smaug] from comment #15) > > NS_IMPL_CYCLE_COLLECTION_1(nsComputedDOMStyle, mContent) > Could you add some comment here that if wrappercache handling is changed, > also canSkip* handling should be changed. The comment about having only one field was supposed to address that, but sure, I can be more explicit about that. > >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsComputedDOMStyle) > >+ return !tmp->mContent || nsGenericElement::CanSkip(tmp->mContent); > >+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END > CanSkip takes now 2 parameters. nsGenericElement::CanSkip(tmp->mContent, > true) Ah, right, I forgot to update for that. Good point about using true, I was just going to pass through the argument, but I guess we don't need to because we're not currently looking at a purple buffer entry.

(In reply to Andrew McCreight [:mccr8] from comment #16) > (In reply to Olli Pettay [:smaug] from comment #15) > > > NS_IMPL_CYCLE_COLLECTION_1(nsComputedDOMStyle, mContent) > > Could you add some comment here that if wrappercache handling is changed, > > also canSkip* handling should be changed. > > The comment about having only one field was supposed to address that, but > sure, I can be more explicit about that. Well, I'd like to have some comment right before NS_IMPL_CYCLE_COLLECTION_1.

Addressed review comments, carrying forward smaug's r+.

(In reply to Boris Zbarsky (:bz) from comment #11) > > The real question here is why is nsComputedDOMStyle wrapper cached? > > Good question. That was added in bug 500850 and I had assumed it was needed > because something was making assumptions about CSS declarations all acting > the same and we wanted element.style to be wrappercached... I think it was just about performance. It used to be fine to cache a wrapper without ever preserving (and so no need for traversing), I'm not sure if that's ok anymore. We don't do it for preserving expandos, but we might blindly preserve if an object QIs to nsWrapperCache in other code?