1358694 - stylo: Fix up the style sharing cache

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

There are various broken and suboptimal bits that I have patches for.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Don't skip the style sharing cache if an element is affected by state. v1

All elements end up with AFFECTED_BY_STATE right now, presumably due to the UA
sheet, and some non-TS pseudo-classes we've added, which means that we currently
never insert anything into the cache.

MozReview-Commit-ID: 5IU4qrjeJFy

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove unused StyleRelations. v1

MozReview-Commit-ID: ed0NiuY9Ek

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Reorganize bits and make the flags usize. v1

These are never stored persistently anywhere, and I'm pretty sure it's slower
for the compiler/CPU to operate on non-word-sized flags.

MozReview-Commit-ID: LQNsJbUsw85

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Make it clearer that we never insert elements with preshints into the cache. v1

MozReview-Commit-ID: 9bdc0sNZeEy

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Share styles between cousins if the parent styles were shared. v1

MozReview-Commit-ID: 1QFCSzC5xvk

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8860584 [details] [diff] [review]
Part 1 - Don't skip the style sharing cache if an element is affected by state. v1

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

Without this, what prevents an element matching :hover to share style with a sibling that doesn't, for example?

I'd rather see what's going on (presumably the state flags are those of the :dir stuff, or similar), and explicitly whitelist/check those are the same instead.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8860585 [details] [diff] [review]
Part 2 - Remove unused StyleRelations. v1

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

r=me

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8860584 [details] [diff] [review]
Part 1 - Don't skip the style sharing cache if an element is affected by state. v1

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

Oh, right, we have the get_state() != candidate.get_state() check, so this is fine. We can probably make it more granular if need be.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8860586 [details] [diff] [review]
Part 3 - Reorganize bits and make the flags usize. v1

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

I don't think making them usize should be a big deal performance-wise btw, you'd just use smaller registers, which are as fast as word-sized ones.

Anyway, this is fine.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8860587 [details] [diff] [review]
Part 4 - Make it clearer that we never insert elements with preshints into the cache. v1

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

::: servo/components/style/matching.rs
@@ +289,5 @@
>          }
>  
> +        // Make sure we noted any presentational hints in the StyleRelations.
> +        if cfg!(debug_assertions) {
> +            let mut hints = vec![];

You can use ForgetfulSink here too. No big deal though.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8860588 [details] [diff] [review]
Part 5 - Share styles between cousins if the parent styles were shared. v1

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

::: servo/components/style/matching.rs
@@ +107,5 @@
> +        (Some(f), Some(s)) => (f, s),
> +        _ => return false,
> +    };
> +
> +    let eq = ::arc_ptr_eq(a.borrow_data().unwrap().styles().primary.values(),

I'd just remove this variable, I don't think it provides anything specially useful.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8860588 [details] [diff] [review]
> Part 5 - Share styles between cousins if the parent styles were shared. v1
> 
> Review of attachment 8860588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/matching.rs
> @@ +107,5 @@
> > +        (Some(f), Some(s)) => (f, s),
> > +        _ => return false,
> > +    };
> > +
> > +    let eq = ::arc_ptr_eq(a.borrow_data().unwrap().styles().primary.values(),
> 
> I'd just remove this variable, I don't think it provides anything specially
> useful.

It's needed to avoid borrow checker issues.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> You can use ForgetfulSink here too. No big deal though.

Good point.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 6 - Disable the style sharing cache on opt builds to avoid regressing Talos by matching too many revalidation selectors. v1

I plan to fix bug 1358693 next, but want to get this landed in the mean time
without regressing our performance numbers (which would happen, because we
currently don't use the setyle sharing cache at all right now because of the
issue fixed by Part 1).

MozReview-Commit-ID: 10qSho1gXjw

Comment 16

[Bobby Holley (:bholley)]

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

Comment 17

[Bobby Holley (:bholley)]

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