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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

This comes by hand with https://github.com/servo/servo/pull/12563

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1288873: Don't propagate the IS_DIRTY flag down the whole tree, just make it

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/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1288873: stylo: Improve the error message when we don't handle a restyle hint.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1288873: stylo: Stop calling Servo_RestyleSubtree if we know it's not effective.

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/

Comment 4

[Cameron McCormack (:heycam)]

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 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 7

[Cameron McCormack (:heycam)]

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 8

[Cameron McCormack (:heycam)]

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?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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 10

[Emilio Cobos Álvarez (:emilio)]

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/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b54cf953d8c4
https://hg.mozilla.org/mozilla-central/rev/872ec4f743e5