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

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Depends on 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57+ fixed, firefox58+ fixed)

Details

Attachments

(2 attachments)

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 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 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).
Attachment #8917272 - Flags: review?(emilio) → review+
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`.
Posted file Servo PR
Priority: -- → P2
This merged.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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
Attachment #8917272 - Flags: approval-mozilla-beta?
Depends on: 1408351
[Tracking Requested - why for this release]: Youtube performance issues with Stylo enabled.
Target Milestone: --- → mozilla58
Comment on attachment 8917272 [details]
Bug 1407522 - stylo: Support more selectors in the style sheet invalidator.

Severe recent regression with Stylo, Beta57+
Attachment #8917272 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.