Closed Bug 1180118 Opened 4 years ago Closed 4 years ago

avoid restyling entire subtrees when a class name on an ancestor is toggled

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed
relnote-firefox --- 42+

People

(Reporter: heycam, Assigned: heycam)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(12 files, 2 obsolete files)

3.70 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
37.34 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
37.73 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
18.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.00 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.66 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.31 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.85 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.36 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.42 KB, text/html
Details
5.79 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
We currently handle a class="" change on an element by doing an eRestyle_Subtree of that element.  In some cases, we have many descendants that won't change which selectors they match, but some that do.  We could instead post a restyle eRestyle_Self more selectively to the descendants that might change.

See bug 1136893 for a proof of concept patch and some comments.
> by doing an eRestyle_Subtree of that element

Only if the selectors that match that class have a ' ' or '>' combinator after them, right?  A rightmost selector matching the class should only trigger eRestyleSelf.

Anyway, it sounds like you're proposing making precisely the case when we're followed by
' ' faster, right?
(In reply to Boris Zbarsky [:bz] from comment #1)
> Only if the selectors that match that class have a ' ' or '>' combinator
> after them, right?  A rightmost selector matching the class should only
> trigger eRestyleSelf.

Yes!

> Anyway, it sounds like you're proposing making precisely the case when we're
> followed by ' ' faster, right?

Yep.  '>' should already be faster due to the bug 931668 work.
(In reply to Cameron McCormack (:heycam) from comment #2)
> > Anyway, it sounds like you're proposing making precisely the case when we're
> > followed by ' ' faster, right?
> 
> Yep.  '>' should already be faster due to the bug 931668 work.

I was wrong, bug 931668 doesn't help with that at all.  '>' is handled just like ' ' for attribute changes, AFAICT.  We return eRestyle_Subtree for it.

I think it would be good to make '>' changes faster too.  I am not sure yet whether the solution in this bug will catch that case too.
So at the moment, whenever we call HasAttributeDependStyle and look up
the rule cascade for the class that was added or removed, and we get
a selector that has a ' ' or '>' combinator on the right, then we
return eRestyle_Subtree.  This means we have to traverse the whole
subtree and re-run selector matching on all elements in it.

This is bad for two reasons.  First is that we can do selector matching
on elements that have no chance of changing what they matched.  For
example with:

  <style>
    div.x p { opacity: 0.5; }
  </style>
  <div>
    <blockquote>
      <p>
        <span>
          <em>

if we toggle the x class on the div, it's wasteful to restyle the div
and the blockquote.

Second is that we can't avail ourselves of the bug 931668 optimizations.
Since the p changes its opacity, we ought to be able to avoid restyling
its descendants.  (With bug 931668, we still must restyle the span, but
bug 1180120 should let us stop directly after the p.)

So it would be good to turn that into an eRestyle_Self on the p.

We don't know which specific descendants of the element with a modified
class will need to be restyled, but we can at least limit it to elements
that match the selector to the right of the last combinator, in this case,
|p|.  We can make HasAttributeDependentStyle be able to return a list of
selectors to apply an eRestyle_Self restyle hint to and the element that
roots the subtree in which to look for those selector-matching elements
(the subtree root being the element that had the class change), and
somehow track these appropriately in the RestyleTracker.  Then, once we
come to process restyles in RestyleTracker::DoProcessRestyles, we can do a
pass over the subtrees that we recorded and do the equivalent of
AddPendingRestyle(..., eRestyle_Self, NS_STYLE_HINT_NONE) on the elements
that match.  We might not want to call AddPendingRestyle exactly, since
that does a lot of traversal up the tree looking for restyle root bits,
which we won't want to do each time.

There is a risk here that the traversal of the subtrees with this
restyle-descendants-that-match-this-selector hint will take longer than
the eRestyle_Subtree processing that we currently do.  We should be
able to ensure that we visit each subtree element at most once.  I'm
hoping that for big subtrees, where we are spending a lot of time
running the eRestyle_Subtree processing, we end up needing to restyle
only a small proportion of those descendants.
(In reply to Cameron McCormack (:heycam) from comment #4)
> There is a risk here that the traversal of the subtrees with this
> restyle-descendants-that-match-this-selector hint will take longer than
> the eRestyle_Subtree processing that we currently do.  We should be
> able to ensure that we visit each subtree element at most once.  I'm
> hoping that for big subtrees, where we are spending a lot of time
> running the eRestyle_Subtree processing, we end up needing to restyle
> only a small proportion of those descendants.

Also of note is that with this plan we will end up matching the selector to the rhs of the last combinator against the matching element twice: first to determine whether we should call AddPendingRestyle for it, and second when we get around to restyling it.  Still, matching a single selector like that shouldn't be that costly.
I'm going to try this out.  Boris/David, do you have any comments on this plan?
Flags: needinfo?(dbaron)
Flags: needinfo?(bzbarsky)
Adding a new eRestyle_SomeDescendants hint for this and storing the selector in the mPendingRestyles table entry might be a better way to handle this.  Then instead of explicitly traversing the subtree to find the elements that match the selector and calling AddPendingRestyle for the matching elements just before we process restyles, we can instead handle it as part of RestyleManager's work as it goes down the tree.
To simplify things, we should limit the types of selectors that can be used with eRestyle_SomeDescendants.  I'm thinking for the moment that if the right-most selector has any pseudo-class or pseudo-element, then we should just fall back to returning eRestyle_Subtree.  Many of the pseudo-classes would be easy to test for without having to accumulate state in ElementRestyler, but let's think about relaxing the restrictions later.

Also if there is a ~ or + combinator we should continue returning eRestyle_LaterSiblings.
> Still, matching a single selector like that shouldn't be that costly.

As long as we just do a SelectorMatches without following combinators, should be pretty fast, yes.  Even more so if we restrict what selectors can end up in here.

In general this does sound at least worth trying, esp. if we then measure it on both what we think are best and worst cases.

One thing to worry about: selectorsa are not refcounted.  So if you just store references to them in some long-lived data structure you'll need to make sure you never examine those after the sheets for the document have changed, or any other operation that can cause selectors to be freed.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #9)
> One thing to worry about: selectorsa are not refcounted.  So if you just
> store references to them in some long-lived data structure you'll need to
> make sure you never examine those after the sheets for the document have
> changed, or any other operation that can cause selectors to be freed.

To avoid making them refcounted or otherwise long lived, maybe we can just set a boolean on the RestyleManager if we clear rule cascades.  If the boolean is set, then when restyling we don't look at the selectors and just treat eRestyle_SomeDescendants as eRestyle_Subtree.
I have some WIP patches, and these are the timings I'm getting on the bug 1172239 restyle testcase (attachment 8629189 [details]) on my machine:

  no patches applied:                            2870ms
  bug 1180118 patches applied:                   2226ms
   " plus a hack to mimic bug 1184842's effect:  1803ms

I still haven't addressed the issue mentioned in bug 1136893 comment 12 where we're spending time looking upwards for a restyle root under AddPendingRestyle, so I reckon I can make more savings.
Attached patch WIP v0.1 (obsolete) — Splinter Review
(In reply to Cameron McCormack (:heycam) from comment #11)
> I still haven't addressed the issue mentioned in bug 1136893 comment 12
> where we're spending time looking upwards for a restyle root under
> AddPendingRestyle, so I reckon I can make more savings.

Doing that didn't help any.
Attached patch WIP v0.2 (obsolete) — Splinter Review
Attachment #8636433 - Attachment is obsolete: true
Tests passing (apart from test_additional_sheets.html timeouts, which is just due to the expensive style context destructor assertions being turned on): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9be1d12d87c2
Not sure about the name "SomeDescendants".  Let me know if you think of something better.
Attachment #8640243 - Flags: review?(bzbarsky)
Attachment #8640251 - Flags: review?(bzbarsky)
Attached file benchmark
Here's a simple test to show how the performance changes in a best case and worst case scenario.  The test creates a binary tree of span elements 16 levels deep.  In the best case, the style sheet has a rule that matches a single span element at level 8.  In the worst case, that rule matches every span element.

Without this bug's patches, the number of elements that we restyle is the same for both the best and worst case, although the worst case still takes longer as we end up doing more work for each element restyled.

With these patches, the best case will only restyle the one element that matches the selector, plus its two children.  In the worst case, we do the same work as without the patches, plus the additional overhead of passing around the selectors and converting the eRestyle_SomeDescendants into eRestyle_Selfs for each span.

On my machine, I get these times:

  without patches
    best case   ~900ms
    worst case  ~6000ms
  with patches
    best case   ~9ms
    worst case  ~6070ms
Comment on attachment 8640241 [details] [diff] [review]
Part 1: Add a method to match a single nsCSSSelector (without pseudos) against an Element.

I don't understand why the negations and pseudo-classes restrictions are there.  The code as written should work fine even if those are present, no?

In particular, if that restriction is removed, you could call this method from nsCSSRuleProcessor::SelectorListMatches I would think, right?

r=me with either the restriction removed or _very_ clearly explained.
Attachment #8640241 - Flags: review?(bzbarsky) → review+
Comment on attachment 8640243 [details] [diff] [review]
Part 2: Add eRestyle_SomeDescendants restyle hint and pass associated restyle hint data into restyle methods.

This looks ok, but it did leave me wondering why we can't just have a RestyleHintData argument and move the nsRestyleHint into that struct, since they're always supposed to go together.

r=me either way, I guess, but I think I'd prefer we combine the two arguments, at least in a followup.

Also, I'm assuming the lifetime management of these selector pointers will be handled in later patches somehow.
Attachment #8640243 - Flags: review?(bzbarsky) → review+
Also, should this thing be in the mozilla::css namespace or the mozilla namespace?
Comment on attachment 8640244 [details] [diff] [review]
Part 3: Convert eRestyle_SomeDescendants into eRestyle_Self for elements that match selectors.

>+ElementRestyler::AddPendingRestylesForDescendantsMatchingSelectors(Element* aElement)

This needs some better documentation about why the mRestyleTracker.HasRestyleData(aElement) case doesn't recur on the kids and just modifies the existing restyle data instead (presumably because we're about to process that restyle data anyway).

>@@ -2880,16 +2957,28 @@ ElementRestyler::Restyle(nsRestyleHint aRestyleHint)

Hmm.  I have to admit to being a bit lost here (perhaps better comments about what mSelectorsForDescendants would help?).  How does this not end up recursively going over the descendants over and over again each step down the tree?

>-  else if (!(aRestyleHint & (eRestyle_Self | eRestyle_Subtree))) {

A diff -w would sure have been nice here.  ;)

>+    const bool restyleSelf =

This is computed in multiple places, and the computation is not quite trivial.  Maybe factor it out?

>+   * Returns true iff a selector in mSelectors matches aElement.  This

mSelectorsForDescendants, right?

r=me with the above fixed.
Attachment #8640244 - Flags: review?(bzbarsky) → review+
Comment on attachment 8640245 [details] [diff] [review]
Part 4: Store pointer to the first selector for class, ID and attribute selectors in the rule cascade.

I think s/first/rightmost/ would make this a lot clearer... because the rightmost thing is not usually considered "first".

Another option would be to call it the "selector matching subject" or "subject-matching selector" or some such, since that's the part we _really_ care about, right?

r=me with a better name here and much better documentation on SelectorPair as to what it's storing: the first thing is a selector that might start or stop matching and the second one is a filter on the possible nodes that could have their style affected as a result, right?

Should the second item be null if it's otherwise == to the first item?
Attachment #8640245 - Flags: review?(bzbarsky) → review+
Comment on attachment 8640246 [details] [diff] [review]
Part 5: Add a RestyleHintData outparam to HasAttributeDependentStyle for use with eRestyle_SomeDescendants.

r=me
Attachment #8640246 - Flags: review?(bzbarsky) → review+
> Should the second item be null if it's otherwise == to the first item?

Ah, looks line no, null means something different in part 6.  Please clearly document on SelectorPair what null means.
And perhaps add asserts that the first selector is non-null and if the two selectors are equal the first has no combinator?
Comment on attachment 8640247 [details] [diff] [review]
Part 6: Return eRestyle_SomeDescendants from HasAttributeDependentStyle where appropriate.

I still don't quite understand this no-pseudoclasses restriction.  Why is it really there?  It's not intrinsic to RestrictedSelectorMatches per previous review comments, so is it intrinsic to this optimization somehow?  Whatever the answer is we should get it into the comments here.

>   // If enumData->change already includes all the bits of possibleChange, don't
>   // bother calling SelectorMatches

Please fix this comment to describe the new setup and in particular why we want to call SelectorMatches even if enumData->change already includes eRestyle_SomeDescendants.

>+    if (possibleChange == eRestyle_SomeDescendants) {

Bit-and would make more sense here, since this is in theory a bitfield, right?

r=me with that fixed.
Attachment #8640247 - Flags: review?(bzbarsky) → review+
Comment on attachment 8640248 [details] [diff] [review]
Part 7: Split out FindClosestRestyleRoot and allow passing in a pre-computed restyle root to AddPendingRestyle.

Why not just make the argument Element*, with null representing "call FindClosestRestyleRoot"?  Is there a use case for passing in explicit null?  If so, document, please.

r=me
Attachment #8640248 - Flags: review?(bzbarsky) → review+
Comment on attachment 8640249 [details] [diff] [review]
Part 8: Keep track of the closest restyle root in AddPendingRestylesForDescendantsMatchingSelectors.

r=me.  I guess this can end up passing null as restyle root if FindClosestRestyleRoot() returns null in ElementRestyler::Restyle?
Attachment #8640249 - Flags: review?(bzbarsky) → review+
Comment on attachment 8640250 [details] [diff] [review]
Part 9: Clear nsCSSSelector pointers in the pending restyle tracker if they might be stale.

r=me
Attachment #8640250 - Flags: review?(bzbarsky) → review+
Comment on attachment 8640251 [details] [diff] [review]
Part 10: Logging.

>+  nsAutoString buf;
>+  aElement->NodeInfo()->NameAtom()->ToString(buf);

  nsDependentAtomString buf(aElement->NodeInfo()->NameAtom());

Not that this is terribly performance-sensitive code, but it's a good habit.  ;)

r=me
Attachment #8640251 - Flags: review?(bzbarsky) → review+
Comment on attachment 8640945 [details] [diff] [review]
Part 11: Use ReparentStyleContext even if eRestyle_SomeDescendants is used.

>@@ -3621,34 +3622,36 @@ ElementRestyler::RestyleSelf(nsIFrame* a

Hmm.  The logic in this chunk didn't use to consider aRestyleHint at all. Why do we need to consider it now?  Or was that a preexisting bug?

>+      if (!(aRestyleHint & ~(eRestyle_Force | eRestyle_ForceDescendants |
>+                             eRestyle_SomeDescendants)) &&
>+          !styleSet->IsInRuleTreeReconstruct()) {

This condition is duplicated several times.  Factor it out?

>-      // XXX Should the above condition ignore eRestyle_Force(Descendants)

Seems to me like this part of the change is a new optimization we didn't have before, right?  It may be worth doing that in a separate bug just to mitigate regression risk...

r=me with that.
Attachment #8640945 - Flags: review?(bzbarsky) → review+
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #30)
> Comment on attachment 8640241 [details] [diff] [review]
> Part 1: Add a method to match a single nsCSSSelector (without pseudos)
> against an Element.
> 
> I don't understand why the negations and pseudo-classes restrictions are
> there.  The code as written should work fine even if those are present, no?

I thought :not() and _moz-any() allowed selectors with combinators inside it, but they don't.  My original thinking was that since they could involve expensive checking up the tree of the nested selector, you might not want to do this for every element in the subtree, especially since you need to evaluate the selector twice (first while traversing down the tree processing the eRestyle_SomeDescendants, and second when proceeding to restyle the element if it matched).

If we allow all pseudo-classes I'll have to ensure that NodeMatchContext is set up correctly before matching.
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #31)
> Comment on attachment 8640243 [details] [diff] [review]
> Part 2: Add eRestyle_SomeDescendants restyle hint and pass associated
> restyle hint data into restyle methods.
> 
> This looks ok, but it did leave me wondering why we can't just have a
> RestyleHintData argument and move the nsRestyleHint into that struct, since
> they're always supposed to go together.
> 
> r=me either way, I guess, but I think I'd prefer we combine the two
> arguments, at least in a followup.

Yeah, I almost did that, but changed my mind and I don't recall why now.  I'll file a followup for it.
Blocks: 1190237
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #32)
> Also, should this thing be in the mozilla::css namespace or the mozilla
> namespace?

I think we are trying to avoid mozilla::css at this point and one day it'll be merged down into mozilla.
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #33)
> Comment on attachment 8640244 [details] [diff] [review]
> Part 3: Convert eRestyle_SomeDescendants into eRestyle_Self for elements
> that match selectors.
> 
> >+ElementRestyler::AddPendingRestylesForDescendantsMatchingSelectors(Element* aElement)
> 
> This needs some better documentation about why the
> mRestyleTracker.HasRestyleData(aElement) case doesn't recur on the kids and
> just modifies the existing restyle data instead (presumably because we're
> about to process that restyle data anyway).

OK.  That is the reason, yes.

> >@@ -2880,16 +2957,28 @@ ElementRestyler::Restyle(nsRestyleHint aRestyleHint)
> 
> Hmm.  I have to admit to being a bit lost here (perhaps better comments
> about what mSelectorsForDescendants would help?).  How does this not end up
> recursively going over the descendants over and over again each step down
> the tree?

At this point we've decided to stop restyling, so we need to recurse down the tree to look at descendants that would otherwise be skipped.  That means recursing until we find (a) an element that already has restyle data, which means we'll look at it soon once we get back up to ProcessPendingRestyles, or (b) it matches one of the mSelectorsForDescendants, in which case we need to restyle it.  In both cases we stop our descendant traversal, so we shouldn't visit any descendants to check mSelectorseForDescendants against more than once.

> >-  else if (!(aRestyleHint & (eRestyle_Self | eRestyle_Subtree))) {
> 
> A diff -w would sure have been nice here.  ;)
> 
> >+    const bool restyleSelf =
> 
> This is computed in multiple places, and the computation is not quite
> trivial.  Maybe factor it out?

OK.

> >+   * Returns true iff a selector in mSelectors matches aElement.  This
> 
> mSelectorsForDescendants, right?

Yep.
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #34)
> Comment on attachment 8640245 [details] [diff] [review]
> Part 4: Store pointer to the first selector for class, ID and attribute
> selectors in the rule cascade.
> 
> I think s/first/rightmost/ would make this a lot clearer... because the
> rightmost thing is not usually considered "first".
> 
> Another option would be to call it the "selector matching subject" or
> "subject-matching selector" or some such, since that's the part we _really_
> care about, right?

Yes, thanks, both of those sound clearer than "first".  I like "rightmost".

> r=me with a better name here and much better documentation on SelectorPair
> as to what it's storing: the first thing is a selector that might start or
> stop matching and the second one is a filter on the possible nodes that
> could have their style affected as a result, right?

Yes that's exactly right.  I'll add comments.
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #37)
> And perhaps add asserts that the first selector is non-null and if the two
> selectors are equal the first has no combinator?

Yes that makes sense to assert.
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #38)
> Comment on attachment 8640247 [details] [diff] [review]
> Part 6: Return eRestyle_SomeDescendants from HasAttributeDependentStyle
> where appropriate.
...
> Bit-and would make more sense here, since this is in theory a bitfield,
> right?

Yes in theory, though RestyleHintForSelectorWithAttributeChange only ever returns one bit.  If we ever return more than one bit, then we strictly wouldn't need to append the selector if we got eRestyle_Subtree too, for example.  But we do that kind of optimization further up the call stack once we've finished building up the selector array from all the rule processors, so doing a bit check here should be fine.
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #39)
> Comment on attachment 8640248 [details] [diff] [review]
> Part 7: Split out FindClosestRestyleRoot and allow passing in a pre-computed
> restyle root to AddPendingRestyle.
> 
> Why not just make the argument Element*, with null representing "call
> FindClosestRestyleRoot"?  Is there a use case for passing in explicit null? 
> If so, document, please.

Yes, explicit null means "there was no restyle root found on all ancestors" and Nothing means "we haven't checked for a restyle root yet".  I'll document.
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #43)
> Comment on attachment 8640945 [details] [diff] [review]
> Part 11: Use ReparentStyleContext even if eRestyle_SomeDescendants is used.
> 
> >@@ -3621,34 +3622,36 @@ ElementRestyler::RestyleSelf(nsIFrame* a
> 
> Hmm.  The logic in this chunk didn't use to consider aRestyleHint at all.
> Why do we need to consider it now?  Or was that a preexisting bug?

I don't believe it's a bug, since it's fine to call ResolveStyleWithReplacement instead of ReparentStyleContext, it just does some unnecessary work.  I'll split these changes out into a separate bug since they're not required here.  (I just wanted to make the code paths for restyling the primary frame's style context, its extra style contexts, and restyling undisplayed content to be uniform.)
Blocks: 1190254
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #30)
> Comment on attachment 8640241 [details] [diff] [review]
> Part 1: Add a method to match a single nsCSSSelector (without pseudos)
> against an Element.
...
> In particular, if that restriction is removed, you could call this method
> from nsCSSRuleProcessor::SelectorListMatches I would think, right?

I think in the end we can't do that, since RestrictedSelectorMatches will want to ensure :visited links match properly, which SelectorListMatches doesn't.
> I thought :not() and _moz-any() allowed selectors with combinators inside it

Ah, they don't yet, but eventually they would, I think.  That's a good point.

> If we allow all pseudo-classes I'll have to ensure that NodeMatchContext is set
> up correctly before matching.

This is to get :link and :visited correct (per comment 53)?  I agree that that sounds too annoying.  Perhaps add comments documenting all this to explain why the restriction is there so someone doesn't try to fix it?

> At this point we've decided to stop restyling, so we need to recurse down the
> tree to look at descendants that would otherwise be skipped. [etc]

This might be good as a code comment.

> I don't believe it's a bug, since it's fine to call ResolveStyleWithReplacement
> instead of ReparentStyleContext, it just does some unnecessary work.

Performance bugs are still bugs.  ;)  Splitting out makes sense, as does trying to make the various codepaths be uniform.
(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #55)
> This is to get :link and :visited correct (per comment 53)?  I agree that
> that sounds too annoying.  Perhaps add comments documenting all this to
> explain why the restriction is there so someone doesn't try to fix it?

I think we can handle this without too much trouble by making RestrictedSelectorMatches do the matching for both unvisited and visited handling, if nodeContext.mIsRelevantLink (which we just initialized to nsCSSRuleProcessor::IsLink(aElement)) is true.  That's probably better than restricting the selector to having no pseudo-classes at all or searching for :visited/:link in mPseodoClasses when deciding whether we can generate an eRestyle_SomeDescendants.
(In reply to Cameron McCormack (:heycam) from comment #56)
> I think we can handle this without too much trouble by making
> RestrictedSelectorMatches do the matching for both unvisited and visited
> handling, if nodeContext.mIsRelevantLink (which we just initialized to
> nsCSSRuleProcessor::IsLink(aElement)) is true.  That's probably better than
> restricting the selector to having no pseudo-classes at all or searching for
> :visited/:link in mPseodoClasses when deciding whether we can generate an
> eRestyle_SomeDescendants.

I did this, and added a warning if we detect selectors inside mNegations or mPseudoClasses that have a combinator, so we can re-evaluate what to do with them if we allow that in the future.
sorry had to back this out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=12466216&repo=mozilla-inbound
Flags: needinfo?(cam)
Hey Cameron,

do you think it's possible to create a Talos test for this ?
I've spoken previously to avhi and jmaher about adding small performance test cases like this, but I got the impression that it would be difficult.  I can't remember the details now.

Maybe for Talos a test measuring the time to open the SMS app would be more appropriate?

We could add a test to ensure that we don't regress these specific improvements by adding some counters in debug builds for the number of frames that get restyled (for example), and in a mochitest check that the expected number of frames do get restyled.
I'll discuss with other Gaia folks to see how we can test this from Gaia. I know we can detect reflows, maybe we can detect restyles (with the hint !). Thanks !
Depends on: 1192302
Depends on: 1192633
Release Note Request (optional, but appreciated)
[Why is this notable]: less processor churn, (potentially) more battery life
[Suggested wording]: Improved performance on interactive websites that trigger a lot of restyles
[Links (documentation, blog post, etc)]: (this bug?)
relnote-firefox: --- → ?
No longer depends on: 1192633
Added to the release notes with Florian's wording.
Once more, thanks for your release notes submission, it is really appreciated!
(In reply to Julien Wajsberg [:julienw] from comment #68)
> I'll discuss with other Gaia folks to see how we can test this from Gaia. I
> know we can detect reflows, maybe we can detect restyles (with the hint !).
> Thanks !

FYI I filed bug 1192168 about this.
Depends on: 1202512
(In reply to Florian Bender from comment #69)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: less processor churn, (potentially) more battery life
> [Suggested wording]: Improved performance on interactive websites that
> trigger a lot of restyles
> [Links (documentation, blog post, etc)]: (this bug?)

We are looking to mention on going improvements to performance on the Mozilla blog and it sounds like this one in particular will be notable for users. 

Question
- does the improvement translate into faster page loads? Better page scrolling? Wondering if it's OK for us to be specific on how it helps users. Any more detail you can provide on this that we can include in a blog post?
I'm not sure the best way to describe the improvements here. :-)  Ultimately the improvement is "restyles resulting from certain kinds of dynamic modifications, such as element class name changes, are processed faster".  That could translate to more responsive pages -- for example, performing some interaction on the page that does a class name change could be fast enough now not to miss frames.
Maybe: "eliminates some sources of perceived lag on very dynamic websites".
Depends on: 1222226
No longer depends on: 1222226
You need to log in before you can comment on or make changes to this bug.