stylo: Change the semantics of the IS_DIRTY flag to improve incremental restyle performance

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

imply that all descendants are dirty.

We're probably going to be a lot more smarter than this in the future, but since
there is work in progress to figure out how should we avoid running
selector-matching on the elements, this helps a lot with perf in the meantime.

Review commit: https://reviewboard.mozilla.org/r/66622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66622/
Attachment #8773995 - Flags: review?(cam)
Attachment #8773996 - Flags: review?(cam)
Attachment #8773997 - Flags: review?(cam)
This allows preventing the creation of the StyleContext in the Servo side, which
is a non-trivial amount of work.

Review commit: https://reviewboard.mozilla.org/r/66626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66626/
https://reviewboard.mozilla.org/r/66622/#review63464

::: layout/base/ServoRestyleManager.cpp:154
(Diff revision 1)
> -    // NB: Using Servo's style system marking the subtree as dirty is necessary
> -    // so we inherit correctly the style structs.
> -    aHint |= eRestyle_Subtree;
> +    // NB: For Servo, at least for now, restyling and running selector-matching
> +    // against the subtree is necessary as part of restyling the element, so
> +    // eRestyle_Self is a strict superset of eRestyle_Subtree.

I guess not always "strict" superset, since it can imply exactly the same restyling work.  Maybe "processing eRestyle_Self will perform at least as much work as eRestyle_Subtree".

::: layout/base/ServoRestyleManager.cpp:158
(Diff revision 1)
> -    // so we inherit correctly the style structs.
> -    aHint |= eRestyle_Subtree;
> -  }
> +    // against the subtree is necessary as part of restyling the element, so
> +    // eRestyle_Self is a strict superset of eRestyle_Subtree.
> +  } else if (aHint & eRestyle_Subtree) {
> -
> -  if (aHint & eRestyle_Subtree) {
>      DirtyTree(aElement, /* aIncludingRoot = */ false);

Do I understand correctly that given how Servo currently processes RESTYLE_SELF, we really only need to mark aElement's children as dirty, and not the entire subtree excluding aElement (as DirtyTree does)?  If so, are we keeping the DirtyTree call here since at some point we'll improve how Servo processes RESTYLE_SELF and we'll need to have marked these subtrees dirty?
Comment on attachment 8773996 [details]
Bug 1288873: stylo: Improve the error message when we don't handle a restyle hint.

https://reviewboard.mozilla.org/r/66624/#review63466

Please update the comment at http://searchfox.org/mozilla-central/rev/496904277ce0143bc1a952f2eb2c7e6a81aa3d4d/layout/base/nsChangeHint.h#209-211 to point to RestyleManagerBase.
Attachment #8773996 - Flags: review?(cam) → review+
https://reviewboard.mozilla.org/r/66622/#review63464

> I guess not always "strict" superset, since it can imply exactly the same restyling work.  Maybe "processing eRestyle_Self will perform at least as much work as eRestyle_Subtree".

You're right, I arguably was way too sleepy when writing that :)

> Do I understand correctly that given how Servo currently processes RESTYLE_SELF, we really only need to mark aElement's children as dirty, and not the entire subtree excluding aElement (as DirtyTree does)?  If so, are we keeping the DirtyTree call here since at some point we'll improve how Servo processes RESTYLE_SELF and we'll need to have marked these subtrees dirty?

I keeped it for simplicity's sake, in the sense of the amount of code I needed to change in order for it to work and have a proof of concept.

If this approach seems fine to you, I can take rid of this function and just mark explicitly as dirty the root / XBL root / XUL root / children when appropriate before landing this.
https://reviewboard.mozilla.org/r/66626/#review63472

::: layout/style/ServoStyleSet.cpp:399
(Diff revision 1)
>    if (aForce) {
>      MOZ_ASSERT(aNode->IsContent());
>      ServoRestyleManager::DirtyTree(aNode->AsContent());
>    }
>  
> +  if (aNode->IsDirtyForServo() || aNode->HasDirtyDescendantsForServo()) {

I wonder if we will ever fail this condition.  Currently, the only time we call RestyleSubtree with aForce = false is from ServoRestyleManager::ProcessPendingRestyles, and at the top of that function, we return early if !HasPendingRestyles().  If that's true, does it make sense to change this to an assertion, and just require that RestyleSubtree be called only on nodes that are dirty or have dirty descendants?
Comment on attachment 8773995 [details]
Bug 1288873: Don't propagate the IS_DIRTY flag down the whole tree, just make it

https://reviewboard.mozilla.org/r/66622/#review63478

::: layout/base/ServoRestyleManager.cpp:158
(Diff revision 1)
> -    // so we inherit correctly the style structs.
> -    aHint |= eRestyle_Subtree;
> -  }
> +    // against the subtree is necessary as part of restyling the element, so
> +    // eRestyle_Self is a strict superset of eRestyle_Subtree.
> +  } else if (aHint & eRestyle_Subtree) {
> -
> -  if (aHint & eRestyle_Subtree) {
>      DirtyTree(aElement, /* aIncludingRoot = */ false);

Ah, DirtyTree no longer traverses the entire tree anyway.  Maybe rename it to MarkChildrenDirty or something?
Attachment #8773995 - Flags: review?(cam) → review+
Comment on attachment 8773995 [details]
Bug 1288873: Don't propagate the IS_DIRTY flag down the whole tree, just make it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66622/diff/1-2/
Comment on attachment 8773996 [details]
Bug 1288873: stylo: Improve the error message when we don't handle a restyle hint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66624/diff/1-2/
Attachment #8773997 - Attachment is obsolete: true
Attachment #8773997 - Flags: review?(cam)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b54cf953d8c4
Don't propagate the IS_DIRTY flag down the whole tree, just make it
https://hg.mozilla.org/integration/mozilla-inbound/rev/872ec4f743e5
stylo: Improve the error message when we don't handle a restyle hint. r=heycam
https://hg.mozilla.org/mozilla-central/rev/b54cf953d8c4
https://hg.mozilla.org/mozilla-central/rev/872ec4f743e5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.