1369620 - stylo: Don't clear the style sharing cache as soon as we hit a parent mismatch

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

When doing cousin sharing, we may have some cousins who have a parent that shares style with ours, and others that do not.  We don't want the latter to cause the whole style sharing cache to be flushed out.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1369620. Stop clearing stylo's sharing cache on parent mismatches.

Instead we keep track of the level we're at right now and clear if that level changes.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874041 [details]
Bug 1369620.  Stop clearing stylo's sharing cache on parent mismatches.

https://reviewboard.mozilla.org/r/145486/#review149486

Looks good. I think given we always pass a properly set up bloom filter there now, it may not be worth the extra arguments.

::: servo/components/style/matching.rs:800
(Diff revision 1)
>      /// eager pseudos.
>      fn match_and_cascade(&self,
>                           context: &mut StyleContext<Self>,
>                           data: &mut ElementData,
> -                         sharing: StyleSharingBehavior)
> +                         sharing: StyleSharingBehavior,
> +                         dom_depth: usize)

I wonder if the extra argument is worth it given we have access to the `context` here (and thus the bloom filter depth, given it is setup).

Is it just to avoid poking at the bloom filter unnecessarily? I don't think it's a big deal, given the length is stored inline.

::: servo/components/style/sharing/mod.rs:340
(Diff revision 1)
>          let bloom_filter = &context.thread_local.bloom_filter;
>  
>          debug_assert_eq!(bloom_filter.current_parent(),
>                           self.element.parent_element());
> +        debug_assert_eq!(bloom_filter.matching_depth(),
> +                         dom_depth);

Similarly, even though the assertion is nice, perhaps we could just avoid passing down the depth and just use `bloom_filter.matching_depth()`?

Asserting the parent will make it clear we're using the correct one.

::: servo/components/style/sharing/mod.rs:409
(Diff revision 1)
>  pub struct StyleSharingCandidateCache<E: TElement> {
>      cache: LRUCache<StyleSharingCandidate<E>>,
> +    /// The DOM depth we're currently at.  This is used as an optimization to
> +    /// clear the cache when we change depths, since we know at that point
> +    /// nothing in the cache will match.
> +    pub dom_depth: usize,

Does this need to be `pub`? I don't see it used outside of this file.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8874041 [details]
Bug 1369620.  Stop clearing stylo's sharing cache on parent mismatches.

https://reviewboard.mozilla.org/r/145486/#review149494

::: servo/components/style/matching.rs:800
(Diff revision 1)
>      /// eager pseudos.
>      fn match_and_cascade(&self,
>                           context: &mut StyleContext<Self>,
>                           data: &mut ElementData,
> -                         sharing: StyleSharingBehavior)
> +                         sharing: StyleSharingBehavior,
> +                         dom_depth: usize)

Right, the idea was to avoid extra reads from context.thread_local, which I assumed was a TLS get.  But if it's not, then recovering the depth from the bloom filter is fine.

::: servo/components/style/sharing/mod.rs:340
(Diff revision 1)
>          let bloom_filter = &context.thread_local.bloom_filter;
>  
>          debug_assert_eq!(bloom_filter.current_parent(),
>                           self.element.parent_element());
> +        debug_assert_eq!(bloom_filter.matching_depth(),
> +                         dom_depth);

Again, this was a matter of whether context.thread_local.bloom_filter is a TLS operation or not.  If it's just member gets all the way, then I agree that we can just get this info out of the bloom filter.

::: servo/components/style/sharing/mod.rs:409
(Diff revision 1)
>  pub struct StyleSharingCandidateCache<E: TElement> {
>      cache: LRUCache<StyleSharingCandidate<E>>,
> +    /// The DOM depth we're currently at.  This is used as an optimization to
> +    /// clear the cache when we change depths, since we know at that point
> +    /// nothing in the cache will match.
> +    pub dom_depth: usize,

Ah, "pub" is on a per-file level, not per-struct level?  In that case, I'll take it out.  The member is used only by other structs in this file.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8874041 [details]
Bug 1369620.  Stop clearing stylo's sharing cache on parent mismatches.

https://reviewboard.mozilla.org/r/145486/#review149486

> I wonder if the extra argument is worth it given we have access to the `context` here (and thus the bloom filter depth, given it is setup).
> 
> Is it just to avoid poking at the bloom filter unnecessarily? I don't think it's a big deal, given the length is stored inline.

Right, the idea was to avoid extra reads from context.thread_local, which I assumed was a TLS get.  But if it's not, then recovering the depth from the bloom filter is fine.

> Similarly, even though the assertion is nice, perhaps we could just avoid passing down the depth and just use `bloom_filter.matching_depth()`?
> 
> Asserting the parent will make it clear we're using the correct one.

Again, this was a matter of whether context.thread_local.bloom_filter is a TLS operation or not.  If it's just member gets all the way, then I agree that we can just get this info out of the bloom filter.

> Does this need to be `pub`? I don't see it used outside of this file.

Ah, "pub" is on a per-file level, not per-struct level?  In that case, I'll take it out.  The member is used only by other structs in this file.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8874041 [details]
Bug 1369620.  Stop clearing stylo's sharing cache on parent mismatches.

https://reviewboard.mozilla.org/r/145486/#review149486

> Right, the idea was to avoid extra reads from context.thread_local, which I assumed was a TLS get.  But if it's not, then recovering the depth from the bloom filter is fine.

It's not a TLS get, just member gets, so there should be no worries :)

See the `components/style/scoped_tls.rs` to see how it's implemented. Borrowing the context does a TLS get eventually (to get the thread index), but that happens only once, and we already have the reference back here, so we don't pay the cost of it twice.

> Ah, "pub" is on a per-file level, not per-struct level?  In that case, I'll take it out.  The member is used only by other structs in this file.

Yeah, it's on a per-module level (in this case, the module is the whole file).

Comment 6

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/autoland/rev/2d7a5edd9351