Closed Bug 1329361 Opened 8 years ago Closed 7 years ago

stylo: Allow styles with pseudo-elements in the StyleSharingCache


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






(Reporter: bholley, Assigned: bzbarsky)


(Blocks 2 open bugs)



(1 file)

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.
Priority: -- → P4
Blocks: 1369952
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.

Priority: P4 → P1
Summary: stylo: Allow styles with pseudo-elements in the StyleSharingCache and store ElementStyles instead of ComputedValues → stylo: Allow styles with pseudo-elements in the StyleSharingCache
Boris, want to give this a shot? The code change is trivial (see above), the rest is just debugging any weirdness that might appear.
Flags: needinfo?(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.
Blocks: 1369902
Assignee: nobody → bzbarsky
Blocks: 1371049
Depends on: 1371112
Flags: needinfo?(bzbarsky)
Comment on attachment 8875573 [details]
Bug 1329361.  Share styles for elements with eager pseudo-elements attached to them.

::: servo/components/style/
(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/
(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.
Attachment #8875573 - Flags: review?(emilio+bugs) → review+
Made those changes, and
Blocks: 1371487
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.