Last Comment Bug 726331 - make nsComputedDOMStyle a cycle-collector skippable class
: make nsComputedDOMStyle a cycle-collector skippable class
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on: 726252 727313
Blocks: 725377 726442
  Show dependency treegraph
 
Reported: 2012-02-11 11:15 PST by Andrew McCreight [:mccr8]
Modified: 2012-02-21 07:47 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
this is the basic idea, WIP (4.50 KB, patch)
2012-02-11 11:31 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
add a null check, still add it to the graph (5.72 KB, patch)
2012-02-14 17:23 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Review
minor cleanup (2.93 KB, patch)
2012-02-14 21:26 PST, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Review
remove CDS from purple buffer if it has no children (3.28 KB, patch)
2012-02-15 12:41 PST, Andrew McCreight [:mccr8]
continuation: review+
Details | Diff | Review

Description Andrew McCreight [:mccr8] 2012-02-11 11:15:15 PST
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.
Comment 1 Andrew McCreight [:mccr8] 2012-02-11 11:31:35 PST
Created attachment 596359 [details] [diff] [review]
this is the basic idea, WIP
Comment 2 Andrew McCreight [:mccr8] 2012-02-11 11:37:28 PST
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.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-11 12:20:55 PST
(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?
Comment 4 Olli Pettay [:smaug] 2012-02-11 12:25:18 PST
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.
Comment 5 Andrew McCreight [:mccr8] 2012-02-11 12:26:24 PST
(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.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-02-11 12:29:45 PST
(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 ...
Comment 7 Andrew McCreight [:mccr8] 2012-02-11 12:38:39 PST
(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.
Comment 8 Andrew McCreight [:mccr8] 2012-02-11 12:57:02 PST
This is the changeset that made nsComputedDOMStyle not visit its wrapper, in case anybody is curious: http://hg.mozilla.org/mozilla-central/rev/ed6b195bae6d
Comment 9 Andrew McCreight [:mccr8] 2012-02-11 13:06:05 PST
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.
Comment 10 Andrew McCreight [:mccr8] 2012-02-12 09:31:15 PST
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.
Comment 11 Boris Zbarsky [:bz] 2012-02-12 21:20:44 PST
> 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...
Comment 12 Andrew McCreight [:mccr8] 2012-02-14 10:59:24 PST
nsDOMCSSAttributeDeclaration also shows up fairly frequently as a childless node in the purple buffer, on TC.
Comment 13 Andrew McCreight [:mccr8] 2012-02-14 17:23:33 PST
Created attachment 597252 [details] [diff] [review]
add a null check, still add it to the graph
Comment 14 Andrew McCreight [:mccr8] 2012-02-14 21:26:02 PST
Created attachment 597295 [details] [diff] [review]
minor cleanup
Comment 15 Olli Pettay [:smaug] 2012-02-15 07:01:32 PST
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.
Comment 16 Andrew McCreight [:mccr8] 2012-02-15 07:28:18 PST
(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.
Comment 17 Olli Pettay [:smaug] 2012-02-15 07:45:14 PST
(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.
Comment 18 Andrew McCreight [:mccr8] 2012-02-15 12:41:33 PST
Created attachment 597529 [details] [diff] [review]
remove CDS from purple buffer if it has no children

Addressed review comments, carrying forward smaug's r+.
Comment 19 Andrew McCreight [:mccr8] 2012-02-15 15:32:22 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/620edbd08124
Comment 20 Marco Bonardo [::mak] 2012-02-16 03:04:27 PST
https://hg.mozilla.org/mozilla-central/rev/620edbd08124
Comment 21 Peter Van der Beken [:peterv] 2012-02-21 07:47:57 PST
(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?

Note You need to log in before you can comment on or make changes to this bug.