make nsComputedDOMStyle a cycle-collector skippable class

RESOLVED FIXED in mozilla13

Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 596359 [details] [diff] [review]
this is the basic idea, WIP
Assignee: nobody → continuation
(Assignee)

Comment 2

5 years ago
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.
Depends on: 726252
(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

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(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 ...
(Assignee)

Comment 7

5 years ago
(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.
Component: Layout → DOM: CSS Object Model
QA Contact: layout → style-system
(Assignee)

Comment 8

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

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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...
(Assignee)

Updated

5 years ago
Blocks: 726442
(Assignee)

Updated

5 years ago
No longer blocks: 716598
(Assignee)

Comment 12

5 years ago
nsDOMCSSAttributeDeclaration also shows up fairly frequently as a childless node in the purple buffer, on TC.
(Assignee)

Comment 13

5 years ago
Created attachment 597252 [details] [diff] [review]
add a null check, still add it to the graph
Attachment #596359 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 727313
(Assignee)

Comment 14

5 years ago
Created attachment 597295 [details] [diff] [review]
minor cleanup
Attachment #597252 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #597295 - Flags: review?(bugs)
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.
Attachment #597295 - Flags: review?(bugs) → review+
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
Created attachment 597529 [details] [diff] [review]
remove CDS from purple buffer if it has no children

Addressed review comments, carrying forward smaug's r+.
Attachment #597295 - Attachment is obsolete: true
Attachment #597529 - Flags: review+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/620edbd08124
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/620edbd08124
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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?
You need to log in before you can comment on or make changes to this bug.