1354895 - stylo: Enabling the style sharing cache causes test failures

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

The style sharing cache was effectively disabled before. Turning it on, I get:

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

Presumably because we're sharing styles in some situations where we shouldn't.

Comment 1

[Bobby Holley (:bholley)]

Any obvious ideas emilio?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Yes, we're not counting with :empty as neither a state selector or a sibling-affecting selector, so we can't disambiguate when two elements don't match it.

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

This is green.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Check for empty when determining what selectors to use for revalidation. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Merge style_affecting_attributes_selectors with selectors_for_cache_revalidation. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Cache the results of cache entry revalidation and use the bloom filter. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 4 - Set selector flags when matching elements against revalidation selectors. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 5 - Logging fixes. v1

I had these lying around in the other bug.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856927 [details] [diff] [review]
Part 3 - Cache the results of cache entry revalidation and use the bloom filter. v1

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

::: servo/components/style/matching.rs
@@ +202,5 @@
> +                           candidate: &mut StyleSharingCandidate<E>,
> +                           candidate_element: &E,
> +                           shared: &SharedStyleContext,
> +                           bloom: &BloomFilter,
> +                           info: &mut CurrentElementInfo,

Does this build now? (the trailing comma)

@@ +203,5 @@
> +                           candidate_element: &E,
> +                           shared: &SharedStyleContext,
> +                           bloom: &BloomFilter,
> +                           info: &mut CurrentElementInfo,
> +                           ) -> bool {

Please leave a comment here referencing bug 1354965, which would be a better optimization than the bloom filter itself.

@@ +206,5 @@
> +                           info: &mut CurrentElementInfo,
> +                           ) -> bool {
> +    let stylist = &shared.stylist;
> +
> +    if info.revalidation_match_results.is_none() {

So, it's not totally clear to me this is a strict advantage over what we have, given we still `break` when we fail due to `Revalidation`.

Before this, we'd stop whenever we find the first selector that matches the candidate but not the element or viceversa.

Should we:

 * Check for preshints after this, and don't break if we fail due to revalidation. Otherwise caching the selectors in current_element_info is not such a big win.
 * Compute the candidate selectors first, then pass that to the stylist so it can bail out if there's any selector that didn't match but matched.
 * Both (keeping track of the last matched selector for this element). Something like RevalidationProgress { matched_selectors: BitVec, last_matched_selector: usize, }.

I'm fine if you want to land this for now, as long as there's a followup bug filled re. doing something like what I mention above.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856929 [details] [diff] [review]
Part 4 - Set selector flags when matching elements against revalidation selectors. v1

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

::: servo/components/style/matching.rs
@@ +216,5 @@
> +        // right elements (which may not necessarily be |element|), causing missed
> +        // restyles after future DOM mutations.
> +        //
> +        // Gecko's test_bug534804.html exercises this. A minimal testcase is:
> +        // <style> #e:empty + span { ... }

nit: closing tag is missing here.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8856930 [details] [diff] [review]
Part 5 - Logging fixes. v1

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

r=me with that fixed

::: servo/components/style/matching.rs
@@ +1101,5 @@
>                  }
>              }
>          }
> +
> +        debug!("{:?} Cannot share style: {} cache misses", self, cache.num_entries());

This is not accurate due to the `break` inside the loop above. You'll need to carry the count, or something like that.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Comment on attachment 8856927 [details] [diff] [review]
> Part 3 - Cache the results of cache entry revalidation and use the bloom
> filter. v1
> 
> Review of attachment 8856927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/matching.rs
> @@ +202,5 @@
> > +                           candidate: &mut StyleSharingCandidate<E>,
> > +                           candidate_element: &E,
> > +                           shared: &SharedStyleContext,
> > +                           bloom: &BloomFilter,
> > +                           info: &mut CurrentElementInfo,
> 
> Does this build now? (the trailing comma)

Fixed, but it did build.

> 
> @@ +203,5 @@
> > +                           candidate_element: &E,
> > +                           shared: &SharedStyleContext,
> > +                           bloom: &BloomFilter,
> > +                           info: &mut CurrentElementInfo,
> > +                           ) -> bool {
> 
> Please leave a comment here referencing bug 1354965, which would be a better
> optimization than the bloom filter itself.

Done.

> 
> @@ +206,5 @@
> > +                           info: &mut CurrentElementInfo,
> > +                           ) -> bool {
> > +    let stylist = &shared.stylist;
> > +
> > +    if info.revalidation_match_results.is_none() {
> 
> So, it's not totally clear to me this is a strict advantage over what we
> have, given we still `break` when we fail due to `Revalidation`.
> 
> Before this, we'd stop whenever we find the first selector that matches the
> candidate but not the element or viceversa.
> 
> Should we:
> 
>  * Check for preshints after this, and don't break if we fail due to
> revalidation. Otherwise caching the selectors in current_element_info is not
> such a big win.
>  * Compute the candidate selectors first, then pass that to the stylist so
> it can bail out if there's any selector that didn't match but matched.
>  * Both (keeping track of the last matched selector for this element).
> Something like RevalidationProgress { matched_selectors: BitVec,
> last_matched_selector: usize, }.
> 
> I'm fine if you want to land this for now, as long as there's a followup bug
> filled re. doing something like what I mention above.

Filed bug 1355668.

Comment 14

[Bobby Holley (:bholley)]

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