1329361 - stylo: Allow styles with pseudo-elements in the StyleSharingCache

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

Emilio and I discussed this, and we think there's no reason pseudo-elements should prevent caching. Pseudos do inherit from the primary style, but if the primary style was different it would be wrong to cache anyway.

We basically want to remove the check for AFFECTED_BY_PSEUDO_ELEMENTS in relations_are_shareable, and then store ElementStyles in the cache instead of ComputedValues.

This may help our cache performance a little bit in situations with pseudo-elements. Super non-urgent, so we can do this later.

Comment 1

[Bobby Holley (:bholley)]

Bumping priority per bug 1369952. Also note that we don't actually put styles per se in the cache, we just put in Elements.

So the change here would be to change [1] to return the ElementStyles instead of the ComputedStyle, and then remove the business at [2]. If all goes well, that should be it.

[1] http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/servo/components/style/sharing/mod.rs#593
[2] http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/servo/components/style/sharing/checks.rs#26

Comment 3

[Bobby Holley (:bholley)]

Boris, want to give this a shot? The code change is trivial (see above), the rest is just debugging any weirdness that might appear.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, I was going to look at this.  I've been trying to figure out whether we can still do any sharing if the actual styles match but the pseudo styles don't, but maybe I shouldn't worry about that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1329361. Share styles for elements with eager pseudo-elements attached to them.

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8875573 [details]
Bug 1329361.  Share styles for elements with eager pseudo-elements attached to them.

https://reviewboard.mozilla.org/r/146994/#review151112

::: servo/components/style/data.rs:303
(Diff revision 1)
> +    /// Returns whether this EagerPseudoStyles has the same set of
> +    /// pseudos as the given one.
> +    pub fn has_same_pseudos_as(&self, other: &EagerPseudoStyles) -> bool {
> +        // We could probably just compare self.keys() to other.keys(), but that
> +        // seems like it'll involve a bunch more moving stuff around and
> +        // whatnot.

You can:

```
match (&self.0, &other.0) {
    (&Some(ref our_arr), &Some(ref other_arr)) {
        // ..
    }
    (&None, &None) => true,
    _ => false,
}
```

Which may be a bit more readable

::: servo/components/style/sharing/mod.rs:547
(Diff revision 1)
> +                    // 1) We matched a different set of eager pseudos (which
> +                    //    should cause a reconstruct).
> +                    // 2) We have restyle damage from the eager pseudo computed
> +                    //    styles.
> +                    // 3) We have restyle damage from our own computed styles.
> +                    if data.get_styles().is_some() {

nit: You can use `has_styles`.

It may be worth to move the accumulate_damage bits out to a helper function, to keep it a bit more readable, but no big deal.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Made those changes, and https://github.com/servo/servo/pull/17245

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/autoland/rev/c608727501c7c6c4c11904b68a3b6963a23af464