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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 2

[Cameron McCormack (:heycam)]

(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.

Comment 3

[Cameron McCormack (:heycam)]

(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.

Comment 4

[Cameron McCormack (:heycam)]

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.

Comment 5

[Cameron McCormack (:heycam)]

(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.

Comment 6

[Cameron McCormack (:heycam)]

I'm going to try this out.  Boris/David, do you have any comments on this plan?

Comment 7

[Cameron McCormack (:heycam)]

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.

Comment 8

[Cameron McCormack (:heycam)]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 10

[Cameron McCormack (:heycam)]

(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.

Comment 11

[Cameron McCormack (:heycam)]

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.

Comment 12

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Comment 13

[Cameron McCormack (:heycam)]

(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.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: WIP v0.2

Comment 15

[Cameron McCormack (:heycam)]

A few test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd3f0b08b200

Comment 16

[Cameron McCormack (:heycam)]

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

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add a method to match a single nsCSSSelector (without pseudos) against an Element.

Comment 18

[Cameron McCormack (:heycam)]

Attachment: Part 2: Add eRestyle_SomeDescendants restyle hint and pass associated restyle hint data into restyle methods.

Not sure about the name "SomeDescendants".  Let me know if you think of something better.

Comment 19

[Cameron McCormack (:heycam)]

Attachment: Part 3: Convert eRestyle_SomeDescendants into eRestyle_Self for elements that match selectors.

Comment 20

[Cameron McCormack (:heycam)]

Attachment: Part 4: Store pointer to the first selector for class, ID and attribute selectors in the rule cascade.

Comment 21

[Cameron McCormack (:heycam)]

Attachment: Part 5: Add a RestyleHintData outparam to HasAttributeDependentStyle for use with eRestyle_SomeDescendants.

Comment 22

[Cameron McCormack (:heycam)]

Attachment: Part 6: Return eRestyle_SomeDescendants from HasAttributeDependentStyle where appropriate.

Comment 23

[Cameron McCormack (:heycam)]

Attachment: Part 7: Split out FindClosestRestyleRoot and allow passing in a pre-computed restyle root to AddPendingRestyle.

Comment 24

[Cameron McCormack (:heycam)]

Attachment: Part 8: Keep track of the closest restyle root in AddPendingRestylesForDescendantsMatchingSelectors.

Comment 25

[Cameron McCormack (:heycam)]

Attachment: Part 9: Clear nsCSSSelector pointers in the pending restyle tracker if they might be stale.

Comment 26

[Cameron McCormack (:heycam)]

Attachment: Part 10: Logging.

Comment 27

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11eee0ff1128

Comment 28

[Cameron McCormack (:heycam)]

Attachment: 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 29

[Cameron McCormack (:heycam)]

Attachment: Part 11: Use ReparentStyleContext even if eRestyle_SomeDescendants is used.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Also, should this thing be in the mozilla::css namespace or the mozilla namespace?

Comment 33

[Boris Zbarsky [:bzbarsky]]

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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

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?

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640246 [details] [diff] [review]
Part 5: Add a RestyleHintData outparam to HasAttributeDependentStyle for use with eRestyle_SomeDescendants.

r=me

Comment 36

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 37

[Boris Zbarsky [:bzbarsky]]

And perhaps add asserts that the first selector is non-null and if the two selectors are equal the first has no combinator?

Comment 38

[Boris Zbarsky [:bzbarsky]]

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.

Comment 39

[Boris Zbarsky [:bzbarsky]]

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

Comment 40

[Boris Zbarsky [:bzbarsky]]

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?

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640250 [details] [diff] [review]
Part 9: Clear nsCSSSelector pointers in the pending restyle tracker if they might be stale.

r=me

Comment 42

[Boris Zbarsky [:bzbarsky]]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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.

Comment 44

[Cameron McCormack (:heycam)]

(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.

Comment 45

[Cameron McCormack (:heycam)]

(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.

Comment 46

[Cameron McCormack (:heycam)]

(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.

Comment 47

[Cameron McCormack (:heycam)]

(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.

Comment 48

[Cameron McCormack (:heycam)]

(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.

Comment 49

[Cameron McCormack (:heycam)]

(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.

Comment 50

[Cameron McCormack (:heycam)]

(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.

Comment 51

[Cameron McCormack (:heycam)]

(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.

Comment 52

[Cameron McCormack (:heycam)]

(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.)

Comment 53

[Cameron McCormack (:heycam)]

(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.

Comment 54

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20f9deac0744

Comment 55

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 56

[Cameron McCormack (:heycam)]

(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.

Comment 57

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef165b896cf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/20984dfa4302
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a7b79980353
https://hg.mozilla.org/integration/mozilla-inbound/rev/b99c358a6fea
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcc3233f3c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfeeae42d514
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ec8d4d2d7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/37493f6eef20
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b41cd9f2bc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/15ad6049b940
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a3e2205388

Comment 58

[Cameron McCormack (:heycam)]

(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.

Comment 59

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5ebadc3fc9

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c65d298d7cfa

Comment 61

[Carsten Book [:Tomcat]]

sorry had to back this out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=12466216&repo=mozilla-inbound

Comment 62

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01576b408ea7

Comment 63

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da34a4032794

Comment 64

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b8dc7443e3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5ff8a35475
https://hg.mozilla.org/integration/mozilla-inbound/rev/d69928567288
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1a39719f3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f73362274d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/13729747e3c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c45ba0178caa
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7367a66c33a
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f7deabf4068
https://hg.mozilla.org/integration/mozilla-inbound/rev/68bef2d8c8bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f97ff142c89

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7b8dc7443e3a
https://hg.mozilla.org/mozilla-central/rev/fc5ff8a35475
https://hg.mozilla.org/mozilla-central/rev/d69928567288
https://hg.mozilla.org/mozilla-central/rev/cf1a39719f3b
https://hg.mozilla.org/mozilla-central/rev/2f73362274d6
https://hg.mozilla.org/mozilla-central/rev/13729747e3c2
https://hg.mozilla.org/mozilla-central/rev/c45ba0178caa
https://hg.mozilla.org/mozilla-central/rev/b7367a66c33a
https://hg.mozilla.org/mozilla-central/rev/9f7deabf4068
https://hg.mozilla.org/mozilla-central/rev/68bef2d8c8bf
https://hg.mozilla.org/mozilla-central/rev/6f97ff142c89

Comment 66

[Julien Wajsberg [:julienw]]

Hey Cameron,

do you think it's possible to create a Talos test for this ?

Comment 67

[Cameron McCormack (:heycam)]

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.

Comment 68

[Julien Wajsberg [:julienw]]

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 !

Comment 69

[Florian Bender]

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?)

Comment 70

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with Florian's wording.
Once more, thanks for your release notes submission, it is really appreciated!

Comment 71

[Julien Wajsberg [:julienw]]

(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.

Comment 72

[Fabio Rios [:frios]]

(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?

Comment 73

[Cameron McCormack (:heycam)]

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.

Comment 74

[Julien Wajsberg [:julienw]]

Maybe: "eliminates some sources of perceived lag on very dynamic websites".