Closed Bug 977991 Opened 10 years ago Closed 9 years ago

don't rerun selector matching when style attribute (inline style) changes

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35
Tracking Flags:
Tracking Status
relnote-firefox --- 35+

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: perf)

Attachments

(8 files)

patch 0 - Add bitwise operators to nsRestyleHint
2.64 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 1 - Expose variant of RulesMatching on nsHTMLCSSStyleSheet that is less work to call
4.52 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 2 - Add mCheckForImportantRules to the information nsStyleSet::RuleNodeWithReplacement keeps about cascade levels
4.27 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 3 - Add ability for RuleNodeWithReplacement to replace the style attribute rule and its important rule
7.18 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 4 - Ensure we don't try to use the style attribute optimization on pseudo-elements
1.86 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 5 - Add eRestyle_ChangeAnimationPhase to switch between the with-animation and without-animation styles
8.65 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 6 - Use the faster eRestyle_StyleAttribute path for style attribute changes
2.83 KB, patch
birtles
: review+
Details | Diff | Splinter Review
patch 7 - Use the faster eRestyle_SVGAnimations hint from bug 1058346 for SMIL-animated SVG attribute changes
1.63 KB, patch
birtles
: review+
Details | Diff | Splinter Review 
This is effectively the next step of optimization following bug 494117.  It was part of the plan in bug 479655 comment 5, but we never did it.  Bug 479655 comment 14 even alleged that this bug was already filed as bug 534335, but that bug is about something different.

We should make it so that dynamic changes to style attributes (inline style) no longer rerun selector matching, even on the element whose style attribute has changed.  Instead, we should just rebuild the path in the rule tree, replacing, adding, or removing the style attribute as needed.  (We could add similar paths for other things as needed, such as the transition and animation style.)

This involves adding a new nsRestyleHint type for each such optimization (style attributes, animations, transitions -- although maybe it's just a single optimization for all three), setting it correctly, and then rebuilding the rule tree path instead of running selector matching as appropriate.
The transition and animation bits would probably be easier after bug 960465 (which, coincidentally, I started this morning).
Oh, and bug 920508 has a testcase this should help.
The code we'd need for this would look a lot like CommonAnimationManager::UpdateThrottledStyle.  Enough like it, in fact, that we should share the code.  (I probably want to re-investigate bug 828173 first, though.)
Severity: normal → enhancement
Depends on: 996796
I have patches for this in my patch queue, but need to test them (both for performance and correctness).

( https://hg.mozilla.org/users/dbaron_mozilla.com/patches/ )
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
And, for the record, I split the part of bug 960465 that this depends on into (part of) bug 996796.
I have patches for this.

In a debug build, without the patches, attachment 809984 [details] from bug 920508 seems to stabilize around 420ms-440ms per frame.

In a debug build with the patches it stabilizes around 360ms-370ms.

That's roughly a 15% improvement, plus or minus a few percent given the variation.

Given bug 920508 comment 11 (suggesting a max improvement on that testcase of 16%), I think that's pretty reasonable.

(Admittedly, I'm profiling a debug build, so the numbers could be off a bit, but the fact that they're faster by about the expected amount suggests that the patches are actually fixing what I think they should be fixing.)
I think it's likely that all the remaining test failures are due to mishandling of (or, rather, lack of handling of) nsPresContext::IsProcessingAnimationStyleChange().  Given that the plan is for that to go away in bug 960465, I might be inclined to just wait for that -- although fixing this up to deal wouldn't be *too* awful.
Depends on: 1057231
Depends on: 1058346
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #9)
> I think it's likely that all the remaining test failures are due to
> mishandling of (or, rather, lack of handling of)
> nsPresContext::IsProcessingAnimationStyleChange().  Given that the plan is
> for that to go away in bug 960465, I might be inclined to just wait for that
> -- although fixing this up to deal wouldn't be *too* awful.

Fixing this up actually wasn't that bad, although we won't get the full performance win until after bug 960465 lands, since the fixup to deal with it is somewhat slower (although it shouldn't be anywhere near as slow as the current behavior).
This avoids having to cast back to nsRestyleHint after using bitwise
operators, and allows |= (etc.).
Attachment #8479498 - Flags: review?(birtles)
This allows nsStyleSet::RuleNodeWithReplacement to call it without
constructing an entire (and unnecessary) ElementRuleProcessorData, which
will happen in patch 3.
Attachment #8479499 - Flags: review?(birtles)
This is needed to support doing replacements of levels that can contain
important rules, which will happen in patch 3.
Attachment #8479500 - Flags: review?(birtles)
This allows posting a restyle that says that only the rule(s) from the
StyleAttrSheet cascade level will be replaced, which avoids running
selector matching.
Attachment #8479501 - Flags: review?(birtles)
This is needed because patch 1 and patch 3 did not add a mechanism that
allows restyling pseudo-elements, which would be substantially more work
and very little use (since the only case in which they have style
attributes is for our internal use on the ::-moz-color-swatch
pseudo-element).
Attachment #8479502 - Flags: review?(birtles)
This is an additional bit on nsRestyleHint that says that the restyling
operation should also perform all the work needed to switch between
style-without-animation and style-with-animation (based on
nsPresContext::IsProcessingAnimationStyleChange).  These concepts will
go away in bug 960465.

Note that we don't want this behavior for the animation-only style
update code (bug 996796, etc.), and I wanted to make this explicit so
that it was clear when it was happening, and so that it was clear what
code should be removed when we git rid of it.

This is the workaround needed to land bug 977991 prior to bug 960465.
(I think there's also a minor dependency in the other direction, so we
need a workaround one way or the other.)

Note that this depends on bug 1057231.
Attachment #8479503 - Flags: review?(birtles)
This couldn't be done in bug 1058346 because it depends on patch 5 in this bug.
Attachment #8479505 - Flags: review?(birtles)
Comment on attachment 8479499 [details] [diff] [review]
patch 1 - Expose variant of RulesMatching on nsHTMLCSSStyleSheet that is less work to call

Review of attachment 8479499 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles
Attachment #8479499 - Flags: review?(birtles) → review+
Comment on attachment 8479500 [details] [diff] [review]
patch 2 - Add mCheckForImportantRules to the information nsStyleSet::RuleNodeWithReplacement keeps about cascade levels

Review of attachment 8479500 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles
Attachment #8479500 - Flags: review?(birtles) → review+
Comment on attachment 8479501 [details] [diff] [review]:
> +        case eRestyle_StyleAttribute: {
> +          if (!level->mIsImportant) {
> +            // First time through, we handle the non-!important rule.
> +            NS_ASSERTION(aPseudoType ==
> +                           nsCSSPseudoElements::ePseudo_NotPseudoElement,
> +                         "this code doesn't know how to replace "
> +                         "pseudo-element rules");
> +            nsHTMLCSSStyleSheet* ruleProcessor =
> +              static_cast<nsHTMLCSSStyleSheet*>(
> +                mRuleProcessors[eStyleAttrSheet].get());
> +            if (ruleProcessor &&
> +                aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {

	
I don't understand this part.

Based on the comment in nsChangeHint.h, eRestyle_StyleAttribute is "supported only for element style contexts and not for pseudo-elements or anonymous boxes, on which it converts to eRestyle_Self."

I suppose I'm not clear on *how* this converts to eRestyle_Self or in what circumstances this will get called with aPseudoType != nsCSSPseudoElements::ePseudo_NotPseudoElement since style attributes only target elements, not pseudo-elements.

If this should never happen we should use NS_ABORT_IF_FALSE and then not check aPseudoType later. If it often happens we should remove the NS_ASSERTION. So I guess this happens only occasionally in which case I don't understand what those circumstances are and how this code falls back to eRestyle_Self.
Flags: needinfo?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #22)
> ...
> I suppose I'm not clear on *how* this converts to eRestyle_Self or in what
> circumstances this will get called with aPseudoType !=
> nsCSSPseudoElements::ePseudo_NotPseudoElement since style attributes only
> target elements, not pseudo-elements.

And then I get to part 4 which explains that:

> This is needed because patch 1 and patch 3 did not add a mechanism that
> allows restyling pseudo-elements, which would be substantially more work
> and very little use (since the only case in which they have style
> attributes is for our internal use on the ::-moz-color-swatch
> pseudo-element).

And also contains the mechanism that answers *how* eRestyle_StyleAttribute is converted to eRestyle_Self.
Flags: needinfo?(dbaron)
Comment on attachment 8479501 [details] [diff] [review]
patch 3 - Add ability for RuleNodeWithReplacement to replace the style attribute rule and its important rule

Review of attachment 8479501 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles but I think the changeset description needs to point out that the "convert to eRestyle_Self for pseudo-elements and anonymous boxes" behavior is implemented in patch 4.

::: layout/style/nsStyleSet.cpp
@@ +1453,5 @@
> +            nsHTMLCSSStyleSheet* ruleProcessor =
> +              static_cast<nsHTMLCSSStyleSheet*>(
> +                mRuleProcessors[eStyleAttrSheet].get());
> +            if (ruleProcessor &&
> +                aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {

Given that in part 4 we explicitly check for pseudo elements and change the restyle hint there (such that doReplace here will become false and we won't execute this code), wouldn't it be sufficient to replace the assertion with a fatal assertion and skip the check below? (If so, that should probably happen in part 4.)

Or is this check still needed for when we get called from ElementRestyler::RestyleUndisplayedChildren (which part 4 doesn't address)?

@@ +1474,2 @@
>          default:
> +          MOZ_ASSERT(false, "should not be reached");

This could possibly be more descriptive, but I'm ok with it as-is.
Attachment #8479501 - Flags: review?(birtles) → review+
Comment on attachment 8479502 [details] [diff] [review]
patch 4 - Ensure we don't try to use the style attribute optimization on pseudo-elements

This patch adjusts the restyle hint passed to RuleNodeWithReplacement when called from ElementRestyler::RestyleSelf (via nsStyleSet::ResolveStyleWithReplacement) but not when called from ElementRestyler::RestyleUndisplayedChildren. Is that ok?

It's possible we never get eRestyle_StyleAttribute from that code path but I don't know enough about the way 'undisplayedRestyleData.mRestyleHint' etc. are used to be sure of that.
Flags: needinfo?(dbaron)
Comment on attachment 8479503 [details] [diff] [review]
patch 5 - Add eRestyle_ChangeAnimationPhase to switch between the with-animation and without-animation styles

>diff --git a/layout/style/nsStyleSet.cpp b/layout/style/nsStyleSet.cpp
>--- a/layout/style/nsStyleSet.cpp
>+++ b/layout/style/nsStyleSet.cpp
>@@ -1347,22 +1347,39 @@ nsRuleNode*
> nsStyleSet::RuleNodeWithReplacement(Element* aElement,
>                                     nsRuleNode* aOldRuleNode,
>                                     nsCSSPseudoElements::Type aPseudoType,
>                                     nsRestyleHint aReplacements)
> {
>   NS_ABORT_IF_FALSE(!(aReplacements & ~(eRestyle_CSSTransitions |
>                                         eRestyle_CSSAnimations |
>                                         eRestyle_SVGAnimations |
>-                                        eRestyle_StyleAttribute)),
>+                                        eRestyle_StyleAttribute |
>+                                        eRestyle_ChangeAnimationPhase)),
>                     // FIXME: Once bug 931668 lands we'll have a better
>                     // way to print these.
>                     nsPrintfCString("unexpected replacement bits 0x%lX",
>                                     uint32_t(aReplacements)).get());
> 
>+  bool skipAnimationRules = false;
>+
>+  // If we're changing animation phase, we have to reconsider what rules
>+  // are in these three levels.
>+  if (aReplacements & eRestyle_ChangeAnimationPhase) {
>+    // FIXME: Need to deal with the SMIL style rule added in
>+    // nsSVGElement::WalkContentStyleRules (in the ePresHintSheet level).
>+    aReplacements |= eRestyle_CSSTransitions |
>+                     eRestyle_CSSAnimations |
>+                     eRestyle_StyleAttribute;

Daniel, since dbaron is away, I wonder if you can help me understand the issue here. I'm not sure I quite understand the two different modes of operation here. Here's what I gather and I hope you can point out where I'm wrong:

A) attributeType="XML" - the mapped attribute case
- The animated values are stores as properties on the element with category SMIL_MAPPED_ATTR_ANIMVAL (see nsSMILMappedAttribute.cpp)
- nsSVGElement::UpdateAnimatedContentStyleRule parses them and stores the result as a style rule on the element, again with category SMIL_MAPPED_ATTR_ANIMVAL but this time under the "style" attribute name
- This is called by nsSVGElement::WalkContentStyleRules. If there is such a rule we insert it into the cascade as a highest-level presentational hint...
- ... or we did until bug 1057231 which makes this its own level in the cascade
- Bug 1058346 adds a restyle hint for replacing style data from these animated mapped attributes: eRestyle_SVGAnimations

B) attributeType="CSS" (and "auto") - the property case
- The animated values are stored in an override style declaration/rule stored on the element (see nsSMILCSSProperty.cpp/nsDOMCSSAttributeDeclaration/Element::GetSMILOverrideStyle etc.)
- This rule gets included in nsHTMLCSSStyleSheet::RulesMatching
- It gets added after the inline style rule (i.e. it's at the override style sheet level in the cascade)

So it seems to me the issue above is about (A). Yet we have a restyle hint for A: eRestyle_SVGAnimations.
Daniel, sorry, comment 26 was for you but Bugzilla doesn't seem to let you set needinfo and comment on a patch at the same time with "edit as comment".
Flags: needinfo?(dholbert)
(In reply to Brian Birtles (:birtles) from comment #26)
> >+  if (aReplacements & eRestyle_ChangeAnimationPhase) {
> >+    // FIXME: Need to deal with the SMIL style rule added in
> >+    // nsSVGElement::WalkContentStyleRules (in the ePresHintSheet level).
> >+    aReplacements |= eRestyle_CSSTransitions |
> >+                     eRestyle_CSSAnimations |
> >+                     eRestyle_StyleAttribute;

I think you're asking about the FIXME here?
Flags: needinfo?(dbaron)
Sorry, bugzilla decided to submit way before I was ready.

I wrote that FIXME before writing bug 1057231, and forgot to actually fix it after doing so.  I need to add the bit from that bug to that |.

(Ugh, now I need to respond to the other needinfo right now too!)
Flags: needinfo?(dholbert)
(In reply to Brian Birtles (:birtles) from comment #25)
> Comment on attachment 8479502 [details] [diff] [review]
> patch 4 - Ensure we don't try to use the style attribute optimization on
> pseudo-elements
> 
> This patch adjusts the restyle hint passed to RuleNodeWithReplacement when
> called from ElementRestyler::RestyleSelf (via
> nsStyleSet::ResolveStyleWithReplacement) but not when called from
> ElementRestyler::RestyleUndisplayedChildren. Is that ok?

I think you're right that RestyleUndisplayedChildren needs the same fix.
Comment on attachment 8479502 [details] [diff] [review]
patch 4 - Ensure we don't try to use the style attribute optimization on pseudo-elements

Review of attachment 8479502 [details] [diff] [review]:
-----------------------------------------------------------------

As per comment 25 and comment 30, ElementRestyler::RestyleUndisplayedChildren probably needs the same fix.

r=birtles with that
Attachment #8479502 - Flags: review?(birtles) → review+
Comment on attachment 8479503 [details] [diff] [review]
patch 5 - Add eRestyle_ChangeAnimationPhase to switch between the with-animation and without-animation styles

Review of attachment 8479503 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here. This confused me a little. Particularly in light of parts 6 and 7 where we pass the eRestyle_ChangeAnimationPhase flag even though I don't *think* we're actually changing phase then. I think I understand it now, and since this is only temporary code to allow us to land one bug before another I think it's fine. r=birtles

::: layout/base/nsChangeHint.h
@@ +296,5 @@
>    // eRestyle_Self.
>    eRestyle_StyleAttribute = (1<<6),
> +
> +  // Additional restyle hint to be used along with CSSTransitions,
> +  // CSSAnimations, or StyleAttribute.  This indicates that along with

or SVGAnimations

::: layout/style/nsStyleSet.cpp
@@ +1367,5 @@
> +    // FIXME: Need to deal with the SMIL style rule added in
> +    // nsSVGElement::WalkContentStyleRules (in the ePresHintSheet level).
> +    aReplacements |= eRestyle_CSSTransitions |
> +                     eRestyle_CSSAnimations |
> +                     eRestyle_StyleAttribute;

As per comment 26 and comment 29, we should probably add eRestyle_SVGAnimations to this list, remove the FIXME, and update the comment above to say "... reconsider what rules are in these four levels."
Attachment #8479503 - Flags: review?(birtles) → review+
Comment on attachment 8479504 [details] [diff] [review]
patch 6 - Use the faster eRestyle_StyleAttribute path for style attribute changes

Review of attachment 8479504 [details] [diff] [review]:
-----------------------------------------------------------------

Does nsSMILMappedAttribute::FlushChangesToTargetAttr() need similar treatment?

r=birtles with comments addressed.

::: layout/style/nsHTMLCSSStyleSheet.cpp
@@ +76,5 @@
>          !aPresContext->IsProcessingAnimationStyleChange()) {
>        // Non-animation restyle -- don't process SMIL override style, because we
>        // don't want SMIL animation to trigger new CSS transitions. Instead,
>        // request an Animation restyle, so we still get noticed.
> +      aPresContext->PresShell()-> RestyleForAnimation(aElement,

Extra space added before RestyleForAnimation.
Attachment #8479504 - Flags: review?(birtles) → review+
Comment on attachment 8479505 [details] [diff] [review]
patch 7 - Use the faster eRestyle_SVGAnimations hint from bug 1058346 for SMIL-animated SVG attribute changes

Review of attachment 8479505 [details] [diff] [review]:
-----------------------------------------------------------------

r=birtles
Attachment #8479505 - Flags: review?(birtles) → review+
Comment on attachment 8479498 [details] [diff] [review]
patch 0 - Add bitwise operators to nsRestyleHint

Review of attachment 8479498 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a bit of work to get enums to behave like integers.

What do you think about converting this to a strongly-typed enum now that we can easily set up bitwise operators on them with MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS (bug 987290)?

Or alternatively templatizing NS_CombineHint/NS_SubtractHint/NS_UpdateHint/NS_IsHintSubset/etc. methods in that same file and making them variadic?

That could happen in a separate bug however since I guess there would be a lot of call sites that would need updating. r=birtles for now.
Attachment #8479498 - Flags: review?(birtles) → review+
(In reply to Brian Birtles (:birtles) from comment #35)
> Comment on attachment 8479498 [details] [diff] [review]

> This seems like a bit of work to get enums to behave like integers.

Yes, although we do it in a few other places (e.g., nsFrameState).  I was thinking we might want to make a set of macros to do it, especially if my trick for the integer type (which isn't in the others; the way they do it would be harder to macro-ize) sticks.  But maybe we just want to use the existing macro below if it works, instead.

> What do you think about converting this to a strongly-typed enum now that we
> can easily set up bitwise operators on them with
> MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS (bug 987290)?

Seems like a reasonable idea, although I'd want to double-check that it supports everything we do with these types.  I'll have a look later in a separate bug.

> Or alternatively templatizing
> NS_CombineHint/NS_SubtractHint/NS_UpdateHint/NS_IsHintSubset/etc. methods in
> that same file and making them variadic?

No, I'd much rather convert the NS_*Hint functions to being operators!


(In reply to Brian Birtles (:birtles) from comment #24)
> Comment on attachment 8479501 [details] [diff] [review]
> patch 3 - Add ability for RuleNodeWithReplacement to replace the style
> attribute rule and its important rule
> 
> ::: layout/style/nsStyleSet.cpp
> @@ +1453,5 @@
> > +            nsHTMLCSSStyleSheet* ruleProcessor =
> > +              static_cast<nsHTMLCSSStyleSheet*>(
> > +                mRuleProcessors[eStyleAttrSheet].get());
> > +            if (ruleProcessor &&
> > +                aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
> 
> Given that in part 4 we explicitly check for pseudo elements and change the
> restyle hint there (such that doReplace here will become false and we won't
> execute this code), wouldn't it be sufficient to replace the assertion with
> a fatal assertion and skip the check below? (If so, that should probably
> happen in part 4.)
> 
> Or is this check still needed for when we get called from
> ElementRestyler::RestyleUndisplayedChildren (which part 4 doesn't address)?

No, I just prefer having the check there because there are various security issues that could happen if we use incorrect style rules, and I'd rather have it there as a belt-and-braces check.  I added a comment saying that, and also changed the assertion to fatal per your earlier comment.

> > +          MOZ_ASSERT(false, "should not be reached");
> 
> This could possibly be more descriptive, but I'm ok with it as-is.

I made it a little more descriptive.

(In reply to Brian Birtles (:birtles) from comment #31)
> Comment on attachment 8479502 [details] [diff] [review]
> patch 4 - Ensure we don't try to use the style attribute optimization on
> pseudo-elements
> 
> As per comment 25 and comment 30,
> ElementRestyler::RestyleUndisplayedChildren probably needs the same fix.

Actually, I remembered why that's not necessary -- RestyleUndisplayedChildren is only used for elements and not for pseudos.  I adjusted the commit message to say that.
(In reply to Brian Birtles (:birtles) from comment #33)
> Comment on attachment 8479504 [details] [diff] [review]
> patch 6 - Use the faster eRestyle_StyleAttribute path for style attribute
> changes
> 
> Does nsSMILMappedAttribute::FlushChangesToTargetAttr() need similar
> treatment?

Well, it doesn't *need* it since this is an optimization.  However, it wouldn't be a bad idea to figure out whether we can use the optimization there as well.

Do you happen to know what styles might have been changed when we hit that function?  Or should I dig into it?

If we can optimize there, it I'd rather put it in a separate bug, though.
Flags: needinfo?(birtles)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #37)
> Do you happen to know what styles might have been changed when we hit that
> function?  Or should I dig into it?

At a high level, I think calls to FlushChangesToTargetAttr() will be expecting changes to any of the properties in this list:
 http://mxr.mozilla.org/mozilla-central/source/dom/smil/nsSMILCSSProperty.cpp#185

(Specifically, it'd be for properties in that list that are exposed as a mapped attribute on the element in question.  (and that depends on the particular element))
(Probably not worth worrying about adding that optimization for nsSMILMappedAttribute::FlushChangesToTargetAttr, though, given that nsSMILMappedAttribute may be going away completely soon, per bug 1062106.)
Depends on: 1066911
Clearing the ni? on me since Daniel answered that in comment 38 and comment 39.
Flags: needinfo?(birtles)
Release Note Request (optional, but appreciated)
[Why is this notable]: speed!
[Suggested wording]: Improved handling of dynamic styling changes to increase responsiveness.
[Links (documentation, blog post, etc)]: Bug 977991
relnote-firefox: --- → ?
Added to the 35 release notes using Florian's wording.
If a blog post is published, we could add it in the release notes.
relnote-firefox: ? → 35+
You need to log in before you can comment on or make changes to this bug.