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

RESOLVED FIXED in mozilla35

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {perf})

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 attachments)

2.25 KB, patch
Details | Diff | Splinter Review
7.10 KB, patch
Details | Diff | Splinter Review
7.82 KB, patch
Details | Diff | Splinter Review
2.42 KB, patch
Details | Diff | Splinter Review
3.55 KB, patch
Details | Diff | Splinter Review
4.85 KB, patch
Details | Diff | Splinter Review
1.31 KB, patch
Details | Diff | Splinter Review
1.03 KB, patch
Details | Diff | Splinter Review
2.07 KB, patch
Details | Diff | Splinter Review
8.23 KB, patch
Details | Diff | Splinter Review
1.58 KB, patch
Details | Diff | Splinter Review
1.07 KB, patch
Details | Diff | Splinter Review
2.54 KB, patch
Details | Diff | Splinter Review
1.37 KB, patch
Details | Diff | Splinter Review
26.81 KB, patch
Details | Diff | Splinter Review
1.67 KB, patch
Details | Diff | Splinter Review
1.03 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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)

Updated

3 years ago
Blocks: 1076361
(Assignee)

Updated

3 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 2

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

Comment 3

3 years ago
Created attachment 8500066 [details] [diff] [review]
patch 1 - Add more comments explaining eRestyle_Subtree vs. eRestyle_ForceDescendants
Attachment #8500066 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

3 years ago
Created attachment 8500067 [details] [diff] [review]
patch 2 - Pass restyle hint to RestyleManager::RebuildAllStyleData

This patch is not intended to contain any changes in behavior.
Attachment #8500067 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

3 years ago
Created attachment 8500068 [details] [diff] [review]
patch 3 - Pass restyle hint to nsPresContext::RebuildAllStyleData

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

Comment 6

3 years ago
Created attachment 8500069 [details] [diff] [review]
patch 4 - Don't rerun selector matching for preference, charset, or system color changes
Attachment #8500069 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

3 years ago
Created attachment 8500070 [details] [diff] [review]
patch 5 - Pass restyle hint to RestyleManager::PostRebuildAllStyleDataEvent

This patch is not intended to contain any changes in behavior.
Attachment #8500070 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

3 years ago
Created attachment 8500071 [details] [diff] [review]
patch 6 - Pass restyle hint to nsPresContext::PostRebuildAllStyleDataEvent

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

Comment 9

3 years ago
Created attachment 8500072 [details] [diff] [review]
patch 7 - Don't rerun selector matching when user font set is updated
Attachment #8500072 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

3 years ago
Created attachment 8500073 [details] [diff] [review]
patch 8 - Don't rerun selector matching when @counter-style rules change
Attachment #8500073 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

3 years ago
Created attachment 8500074 [details] [diff] [review]
patch 9 - Don't rerun selector matching for viewport units
Attachment #8500074 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

3 years ago
Created attachment 8500075 [details] [diff] [review]
patch 10 - Pass restyle hint to nsPresContext::MediaFeatureValuesChanged

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

Comment 13

3 years ago
Created attachment 8500076 [details] [diff] [review]
patch 11 - Fix indentation
Attachment #8500076 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

3 years ago
Created attachment 8500077 [details] [diff] [review]
patch 12 - Optimize nsDocShell::SetDeviceSizeIsPageSize better, since changing results of media queries should not require forcing restyling if media queries don't change
Attachment #8500077 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

3 years ago
Created attachment 8500078 [details] [diff] [review]
patch 13 - Don't rerun selector matching for changes to zoom, min font size, or app units per dev pixel
Attachment #8500078 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

3 years ago
Created attachment 8500079 [details] [diff] [review]
patch 14 - Explain why changes to theme require rerunning selector matching
Attachment #8500079 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

3 years ago
Created attachment 8500080 [details] [diff] [review]
complete diff, patches 1 through 14

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+
(Assignee)

Comment 21

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

Comment 22

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

Comment 24

3 years ago
Created attachment 8501760 [details] [diff] [review]
patch 4 - Don't rerun selector matching for charset or system color changes
Attachment #8501760 - Flags: review?(bzbarsky)
(Assignee)

Comment 25

3 years ago
Created attachment 8501761 [details] [diff] [review]
patch 4a - Explain why we need to rerun selector matching for preference changes
Attachment #8501761 - Flags: review?(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

Updated

3 years ago
Depends on: 1089417
You need to log in before you can comment on or make changes to this bug.