1358693 - stylo: Too many revalidation selectors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Bobby Holley (:bholley)]

There seem to be a few bugs that are causing us to have way too many revalidation selectors, which ruins performance. At present, even "span div" seems to be tagged as a revalidation selector, which is clearly wrong. But even aside from that, I think we currently include anything that has attribute-dependent style, whereas we should be excluding id and class (since those are already checked by the style sharing cache).

I want to do all these and then see how much revalidation ends up costing. If it's still a lot, then we can optimize it further.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Separate revalidation checking from attribute sensitivity checking. v1

The current code is busted because:
* It unconditionally flags for revalidation if the sensitivities are not yet
  sensitive to attributes.
* It uses is_attr_selector, despite the fact that ID and Class are both handled
  by the style sharing cache and should not trigger revalidation.

MozReview-Commit-ID: C7d8QrdKcFm

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use a different visitor pass for gathering revalidation selectors. v1

This allows us to ignore everything to the left of the rightmost simple selector.

MozReview-Commit-ID: 6I4VzKos22n

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Require Clone for SelectorImpl so that all the types that are parameterized on it can derive(Clone). v1

It's not clear to me why this is a requirement, but it seems to be one.

MozReview-Commit-ID: JM0DKjHHfT

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Store SelectorInner and only revalidate up to the rightmost ancestor combinator. v1

MozReview-Commit-ID: HiTGVhwuvCE

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Check for adjacent duplicates when accumulating revalidation selectors. v1

MozReview-Commit-ID: 4C8L5u1S1sL

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 6 - Add unit tests for revalidation selectors. v1

If these break, our cache may be subtly wrong in certain situations, which may
be hard to detect.

MozReview-Commit-ID: AXG2tpGnQ6k

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 7 - Properly handle -moz-any for revalidation selectors. v1

Note that, while the comment is correct that there is a fair amount of -moz-any
usage in the UA sheet, it's always used as an ancestor selector, and thus ignored
for our purposes. Nevertheless, it's straightforward enough to support properly,
so we do that here.

MozReview-Commit-ID: Kz1yNfPUIaP

Comment 8

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b62271c5d0d3a47812c28455c258555bc6ff744

Comment 9

[Bobby Holley (:bholley)]

The last thing to do here is bug 1360088, after which point we should be able to profile and re-enable the style sharing cache. There's enough in this bug that I want to get it into the tree though.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8862268 [details] [diff] [review]
Part 1 - Separate revalidation checking from attribute sensitivity checking. v1

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

Whoops... Yeah, that was definitely intended to be self.needs_revalidation |= self.sensitivities.attrs. But it's true that that'd include class an id selectors.

This looks good to me.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8862269 [details] [diff] [review]
Part 2 - Use a different visitor pass for gathering revalidation selectors. v1

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

Looks fine, though we could also keep that state in the dependency visitor, fwiw.

Anyway, r=me

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8862270 [details] [diff] [review]
Part 3 - Require Clone for SelectorImpl so that all the types that are parameterized on it can derive(Clone). v1

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

Yeah, this is essentially <https://github.com/rust-lang/rust/issues/26925>.

I'm not too fond of adding the clone bound, but if you feel it's useful, r=me with a comment pointing to that rust issue.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8862271 [details] [diff] [review]
Part 4 - Store SelectorInner and only revalidate up to the rightmost ancestor combinator. v1

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

r=me with that.

::: servo/components/selectors/parser.rs
@@ +165,5 @@
>          }
>      }
>  
> +    /// Creates a clone of this selector with everything to the left of
> +    /// (and including) the rightmost ancestor combinator removed. This is used

You mean everything to the right, right?

i.e., if we have |span foo bar + baz|, this will return |bar + baz|.

Mind adding an example like that to the comment?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8862272 [details] [diff] [review]
Part 5 - Check for adjacent duplicates when accumulating revalidation selectors. v1

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

::: servo/components/style/stylist.rs
@@ +343,5 @@
> +                            // Because of the slicing we do above, we can often end up with
> +                            // adjacent duplicate selectors when we have selectors like
> +                            // body > foo, td > foo, th > foo, etc. Doing a check for
> +                            // adjacent duplicates here reduces the number of revalidation
> +                            // selectors for Gecko's UA sheet by 30%.

oh, wow.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Thanks for fixing this bobby! :)

Comment 16

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> Comment on attachment 8862271 [details] [diff] [review]
> Part 4 - Store SelectorInner and only revalidate up to the rightmost
> ancestor combinator. v1
> 
> Review of attachment 8862271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that.
> 
> ::: servo/components/selectors/parser.rs
> @@ +165,5 @@
> >          }
> >      }
> >  
> > +    /// Creates a clone of this selector with everything to the left of
> > +    /// (and including) the rightmost ancestor combinator removed. This is used
> 
> You mean everything to the right, right?

No, everything to the left _removed_.

> 
> i.e., if we have |span foo bar + baz|, this will return |bar + baz|.
> 
> Mind adding an example like that to the comment?

Sure.

Comment 17

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/16635