1407522 - stylo: tweak style sheet invalidations to bail out to dirtying the whole document less

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Cameron McCormack (:heycam)]

From bug 1405411 comment 53.  I'm going to try (a) adding support for local-name-based invalidation scopes, and (b) adding restyle-the-element-not-its-descendants invalidation scopes, for selectors with no ancestor combinators.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1407522 - stylo: Support more selectors in the style sheet invalidator.

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

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8917272 [details]
Bug 1407522 - stylo: Support more selectors in the style sheet invalidator.

https://reviewboard.mozilla.org/r/188284/#review193484

::: servo/components/style/invalidation/stylesheets.rs:61
(Diff revision 1)
>                      None => false,
>                  }
>              }
> +            Invalidation::LocalName { ref name, ref lower_name } => {
> +                let local_name = element.get_local_name();
> +                local_name == name.borrow() || local_name == lower_name.borrow()

Please tell me how I can avoid calling borrow() here. :-)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8917272 [details]
Bug 1407522 - stylo: Support more selectors in the style sheet invalidator.

https://reviewboard.mozilla.org/r/188284/#review193488

::: commit-message-e765c:1
(Diff revision 1)
> +Bug 1407522 - stylo: Support more selectors in the style sheet invalidator. r?emilio

I'd expand on the commit message adding that not only you're supporting more selectors, but also selectors that affect an element individually.

::: servo/components/style/invalidation/stylesheets.rs:51
(Diff revision 1)
>      fn matches<E>(&self, element: E) -> bool
>          where E: TElement,
>      {
>          match *self {
> -            InvalidationScope::Class(ref class) => {
> +            Invalidation::Class(ref class) => {
>                  element.has_class(class, CaseSensitivity::CaseSensitive)

Hmm, I think this is not quite right in the presence of quirks mode, is it?

I'd expect a test that adds a single class selector to fail in this case, since in quirks mode it need.

This is pre-existing, so we can file a bug and figure it out if you want.

Same for the ID, I think it should be compared case-insensitively in quirks mode documents.

::: servo/components/style/invalidation/stylesheets.rs:61
(Diff revision 1)
>                      None => false,
>                  }
>              }
> +            Invalidation::LocalName { ref name, ref lower_name } => {
> +                let local_name = element.get_local_name();
> +                local_name == name.borrow() || local_name == lower_name.borrow()

Please add a note that we could just check if the element is an HTML element in an HTML document, and if so just test one of those, but it's probably not worth it.

::: servo/components/style/invalidation/stylesheets.rs:225
(Diff revision 1)
> +            for invalidation in &self.invalid_elements {
> +                if invalidation.matches(element) {
> +                    debug!("process_invalidations_in_subtree: {:?} matched self {:?}",
> +                           element, invalidation);
> +                    data.hint.insert(RESTYLE_SELF);
> +                    any_children_invalid = true;

nit: You want to `break;` here, I suppose.

Also, the `any_children_invalid = true` is kind of a lie... Maybe we should keep that just for children, and have a different `invalidated_self`, then return `invalidated_self || any_children_invalid`.

Or, maybe we just want to rename it to `invalidated_self_or_any_children`. Whatever you think it's best.

::: servo/components/style/invalidation/stylesheets.rs:329
(Diff revision 1)
> -                debug!(" > Scope not found");
> -
> -                // If we didn't find a scope, any element could match this, so
> -                // let's just bail out.
> +            self.invalid_elements.insert(s);
> +        } else {
> +            // The selector was of a form that we can't handle. Any element
> +            // could match it, so let's just bail out.
> +            debug!(" > Can't handle selector, marking fully invalid");
> -                self.fully_invalid = true;
> +            self.fully_invalid = true;

nit: Maybe we should clear the sets at this point? Not a big deal I guess (plus, preexisting).

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8917272 [details]
Bug 1407522 - stylo: Support more selectors in the style sheet invalidator.

https://reviewboard.mozilla.org/r/188284/#review193484

> Please tell me how I can avoid calling borrow() here. :-)

Oh, was about to mention it, sorry. I think you should be able to do something with `&*local_name` and `&*lower_name`, since there's an `impl Deref<Target=WeakAtom> for Atom`.

Comment 5

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a7a9d45b3afd419ea1a40986c443b97fe8b0a86

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8917272 [details]
Bug 1407522 - stylo: Support more selectors in the style sheet invalidator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188284/diff/1-2/

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Servo PR

Comment 8

[Cameron McCormack (:heycam)]

This merged.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8917272 [details]
Bug 1407522 - stylo: Support more selectors in the style sheet invalidator.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: poor performance on YouTube subscriptions page (and probably other YouTube pages) when the new material design is enabled; without this patch this page performs better with stylo disabled
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a straightforward extension of Servo's existing invalidation code to handle some more cases that YouTube's style sheets were hitting
[String changes made/needed]: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Youtube performance issues with Stylo enabled.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8917272 [details]
Bug 1407522 - stylo: Support more selectors in the style sheet invalidator.

Severe recent regression with Stylo, Beta57+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b74c46c59b20

Comment 13

[Andrei Vaida [:avaida]]

(In reply to Cameron McCormack (:heycam) (away 24 Oct – 12 Nov) from comment #9)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Cameron's assessment on manual testing needs and the fact that this fix has automated coverage.