Closed Bug 1047928 Opened 10 years ago Closed 10 years ago

improve performance of callers of RebuildAllStyleData by passing in appropriate nsRestyleHint for rebuilds that change data but not selectors matched

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(17 files)

2.25 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.10 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.82 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.42 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
3.55 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.85 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.31 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.07 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.23 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.07 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
26.81 KB, patch
Details | Diff | Splinter Review
1.67 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Bug 996796 patch 20 adds the following comment to RestyleManager::RebuildAllStyleData:

  // FIXME: Many of the callers probably don't need eRestyle_Subtree
  // because they're changing things that affect data computation rather
  // than selector matching; we could have a restyle hint passed in, and
  // substantially improve the performance of things like pref changes
  // and the restyling that we do for downloadable font loads.

This bug covers doing this, i.e., improving the performance of callers of RebuildAllStyleData by passing in an appropriate nsRestyleHint for calls that change style data without changing which selectors match.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
I confirmed locally that removing the patch https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/bcaafe07840a/optimize-mathml-sheet from the patch queue fixes the pair of assertions:

###!!! ASSERTION: aElement should be the element and not the pseudo-element: 'pseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement || !elementForAnimation->GetPrimaryFrame() || elementForAnimation->GetPrimaryFrame()->StyleContext()-> GetPseudoType() == nsCSSPseudoElements::ePseudo_NotPseudoElement', file /builds/slave/try-l64-d-00000000000000000000/build/layout/style/nsStyleSet.cpp, line 1602

leading to:
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/374420.xhtml | assertion count 2 is more than expected 0 assertions

I'm just going to remove that patch from the queue since it's among the less important.  (I think the most likely explanation might be that nothing else causes a restyle when we add the sheet -- which in turn means that the current code is correct.)
This patch is not intended to contain any changes in behavior.
Attachment #8500067 - Flags: review?(bzbarsky)
This patch is not intended to contain any changes in behavior.

The behavior changes for these callers are in the following patch.
Attachment #8500068 - Flags: review?(bzbarsky)
This patch is not intended to contain any changes in behavior.
Attachment #8500070 - Flags: review?(bzbarsky)
This patch is not intended to contain any changes in behavior.

The behavior changes for these callers are in the following 2 patches.
Attachment #8500071 - Flags: review?(bzbarsky)
This patch is not intended to contain any changes in behavior.

The behavior changes in these callers are in the following 4 patches.
Attachment #8500075 - Flags: review?(bzbarsky)
To see the end results of which FIXMEs I remove, and which changes I later optimize further.
Attachment #8500066 - Flags: review?(bzbarsky) → review+
Attachment #8500067 - Flags: review?(bzbarsky) → review+
Attachment #8500068 - Flags: review?(bzbarsky) → review+
Comment on attachment 8500069 [details] [diff] [review]
patch 4 - Don't rerun selector matching for preference, charset, or system color changes

>+++ b/layout/base/nsPresContext.cpp
>+  // Preferences don't change the results of selector matching; they
>+  // only change the computed style data.  This means we can pass
>+  // nsRestyleHint(0).

Is that really true?

Say the link color preference gets changed.  This is implemented by blowing away the presshell's mPrefStyleSheet and creating a new one, with rules that are based on the new value of the preference.

Won't we fail to pick that up if we don't rerun selector matching and thus continue to use a rulenode corresponding to the old rule we used to have?

r- for this part of the patch.

For the other two parts, why is nsRestyleHint(0) ok?  Why do we not need eRestyle_ForceDescendants?  It's worth documenting exactly what's going on there, because it's not clear to me.
Attachment #8500069 - Flags: review?(bzbarsky) → review-
Comment on attachment 8500070 [details] [diff] [review]
patch 5 - Pass restyle hint to RestyleManager::PostRebuildAllStyleDataEvent

r=me
Attachment #8500070 - Flags: review?(bzbarsky) → review+
Attachment #8500071 - Flags: review?(bzbarsky) → review+
Attachment #8500072 - Flags: review?(bzbarsky) → review+
Comment on attachment 8500073 [details] [diff] [review]
patch 8 - Don't rerun selector matching when @counter-style rules change

r=me, though perhaps document why this needs to be eRestyle_ForceDescendants,
not nsRestyleHint(0).
Attachment #8500073 - Flags: review?(bzbarsky) → review+
Attachment #8500074 - Flags: review?(bzbarsky) → review+
Attachment #8500075 - Flags: review?(bzbarsky) → review+
Attachment #8500076 - Flags: review?(bzbarsky) → review+
Attachment #8500077 - Flags: review?(bzbarsky) → review+
Attachment #8500078 - Flags: review?(bzbarsky) → review+
Attachment #8500079 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #18)
> Comment on attachment 8500069 [details] [diff] [review]
> patch 4 - Don't rerun selector matching for preference, charset, or system
> color changes


> For the other two parts, why is nsRestyleHint(0) ok?  Why do we not need
> eRestyle_ForceDescendants?  It's worth documenting exactly what's going on
> there, because it's not clear to me.

RebuildAllStyleData adds eRestyle_ForceDescendants internally before passing it off to other code.


The only places where I'm explicitly passing eRestyle_ForceDescendants are to MediaFeatureValuesChanged, which I guess deserves a clearer comment at the declaration of MediaFeatureValuesChanged.  How about:

  The sensible values to pass for aRestyleHint are:
   * nsRestyleHint(0) to rebuild style data, with rerunning of selector matching, only if media features have changed
   * eRestyle_ForceDescendants to force rebuilding of style data (but still only rerun selector matching if media features have changed).  (RebuildAllStyleData always adds eRestyle_ForceDescendants internally, so here we're only using it to distinguish from nsRestyleHint(0) whether we need to call RebuildAllStyleData at all.)
   * eRestyle_Self to force rebuilding of style data with rerunning of selector matching
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #20)
> Comment on attachment 8500073 [details] [diff] [review]
> patch 8 - Don't rerun selector matching when @counter-style rules change
> 
> r=me, though perhaps document why this needs to be eRestyle_ForceDescendants,
> not nsRestyleHint(0).

Oops, it doesn't.  And neither does patch 7.  (I had it as nsRestyleHint(0) originally, then decided to change it, but I should probably change it back.)
> RebuildAllStyleData adds eRestyle_ForceDescendants internally

Ah, in DoRebuildAllStyleData outside the patch context, I see.

I think it's worth documenting that the only values it makes sense to pass to RebuildAllStyleData are nsRestyleHint(0) and eRestyle_Subtree, right?

>   * eRestyle_Self to force rebuilding of style data with rerunning of selector matching

You mean eRestyle_Subtree?  Yes, documenting this for MediaFeatureValuesChanged would make sense.
Flags: needinfo?(bzbarsky)
Comment on attachment 8501760 [details] [diff] [review]
patch 4 - Don't rerun selector matching for charset or system color changes

Do add the docs about what sort of values make sense to be passed to RebuildAllStyleData?
Attachment #8501760 - Flags: review?(bzbarsky) → review+
Comment on attachment 8501761 [details] [diff] [review]
patch 4a - Explain why we need to rerun selector matching for preference changes

r=me
Attachment #8501761 - Flags: review?(bzbarsky) → review+
Depends on: 1086937
Depends on: 1089417
You need to log in before you can comment on or make changes to this bug.