1387116 - stylo: Fix cousin sharing to work on initial styling.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

This is fallout from bug 1381821 (you can read there about the different discussions around this).

The basic problem is that to share style with a cousin we need to prove that the parents of that element not only have shared styles, but also have the same attributes and states (at least the same set that may affect the style output).

The easy way to fix this is to check that both parents have been styled in the same traversal we're currently at... That's the fix bug 1381821 is about to land (checking the WAS_RESTYLED flag), but we don't have any way to tell that two elements have just been initially styled.

My proposed solution is having such a flag set in the traversal, so we can use that _or_ the WAS_RESTYLED flag in order to prove that we've styled this element during the traversal.

Other possible solution, as mentioned by Boris, is storing all attribute, class and id selectors in the revalidation selectors.

The more I think about it, the more I think the revalidation stuff may also be ok (and may allow a bit more sharing on dynamic restyles).

We already have all attribute selectors there, class selectors should be cheap to skip with the bloom filter, and I think state selectors on non-rightmost selectors should be uncommon enough (or, we can also do a cheap-ish walk up the ancestor chain checking states in the parents until reaching the common ancestor, worst-case)

We could split revalidation selectors in two buckets (depending on whether they appear in the rightmost selector or not), and skip the ones with ancestor combinators if we know we've restyled both parents, too.

I'm happy to implement any of the two solutions, we just have to decide which one to go for.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Bobby, Cam, Boris, any opinion on which one of the two solutions do you prefer?

I'm fine with any of both.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Or, alternative solutions are also fine, of course.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK. So if we had a flag set during traversal then the issue would be that we'd need to clear it after the traversal somewhere, because if the element is moved around in the DOM or something the flag certainly needs to go away, right?

I have to admit that I'm a bit partial to the revalidation approach, simply because I think it's more self-contained and easier to get and prove right; it doesn't depend on some other faraway code correctly managing some flags.  It might be worth hacking it up quickly (keep the one bucket) and then seeing how many revalidation selector matches we end up having to do in practice on some of our usual test pages, compared to without the patch.

Comment 4

[Bobby Holley (:bholley)]

So, bz and I discussed this for a bit.

I'm skeptical of the revalidation selector approach. It seems to me that this causes every selector that (a) has an ancestor combinator and (b) has something other than a localname or a namespace to the left of the rightmost ancestor combinator to become a revalidation selector. To me, that seems like "most" selectors (or at least half of them), at which point we're not really avoiding all that much selector matching with the style sharing cache. The two-bucket approach helps, but doesn't help in the initial styling case, which is kinda were perf and sharing matter the most.

I think we can make the INITIALLY_STYLED flag work. The problem raised, IIUC, is the problem of who clears the bit. But I think we can just clear it on the next traversal, if we traverse past the element without restyling it. Or, alternatively, we could call the bit WAS_TRAVERSED_BUT_NOT_RESTYLED, set the bit when we traverse past an element, and unset it when we set WAS_RESTYLED. I think this should give us the behavior that we want.

bz pointed out that the flag approach wouldn't be enough if we did bug 1370604, which is a good point. But I think we should do the flag in this bug, and then, if/when we do bug 1370604, implement the two-bucket approach and use the second bucket when restyling. That way the flag will keep the initial styling case fast.

Make sense? Fixing this is on the urgent side because perf metrics are starting to matter and we don't want to be without cousin sharing for too long. We really need to get this and bug 1368290 into the tree as soon as possible. Thanks for doing all this emilio!

Comment 5

[Bobby Holley (:bholley)]

Attachment: Introduce a new flag and use it to be more permissive about cousin sharing. v1

Like this. I've confirmed that this restores initial style sharing to the
original levels.

MozReview-Commit-ID: BCJg0Ycsy6M

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8894030 [details] [diff] [review]
Introduce a new flag and use it to be more permissive about cousin sharing. v1

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

r=me, but check that this doesn't break with your patches to not traverse from the root.

::: servo/components/style/data.rs
@@ +28,5 @@
> +        /// WAS_RESTYLED is set).
> +        ///
> +        /// This bit always corresponds to the last time the element was
> +        /// traversed, so each traversal simply updates it with the appropriate
> +        /// value.

The reason I didn't want to do something like this is because this is not as easy to reason about if we don't always traverse from the root... But I _think_ it's just fine.

::: servo/components/style/sharing/checks.rs
@@ +21,3 @@
>      where E: TElement,
>  {
> +    debug_assert!(first != second, "Caller checks this");

There's a debug_assert_ne! under this that checks the same, so I don't think this is needed... I guess this checks we don't get None, None inside, but...

Comment 8

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> Comment on attachment 8894030 [details] [diff] [review]
> Introduce a new flag and use it to be more permissive about cousin sharing.
> v1
> 
> Review of attachment 8894030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but check that this doesn't break with your patches to not traverse
> from the root.
> 
> ::: servo/components/style/data.rs
> @@ +28,5 @@
> > +        /// WAS_RESTYLED is set).
> > +        ///
> > +        /// This bit always corresponds to the last time the element was
> > +        /// traversed, so each traversal simply updates it with the appropriate
> > +        /// value.
> 
> The reason I didn't want to do something like this is because this is not as
> easy to reason about if we don't always traverse from the root... But I
> _think_ it's just fine.

I think it's pretty sound. Think of it this way:
* The flag is valid for any element that was traversed in this traversal.
* If two elements with different parents are going to share style, we must have traversed both of their parents in this traversal, since we always start at a single root.

Therefore, the flag is always up to date for the parents in question.

> 
> ::: servo/components/style/sharing/checks.rs
> @@ +21,3 @@
> >      where E: TElement,
> >  {
> > +    debug_assert!(first != second, "Caller checks this");
> 
> There's a debug_assert_ne! under this that checks the same, so I don't think
> this is needed... I guess this checks we don't get None, None inside, but...

Good catch.

Thanks for the fast review!

Comment 9

[Bobby Holley (:bholley)]

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

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e4ef621b23b8cf6b5a562e7ab09b35479542da0b

(Note that the base m-c rev of this try push had some orange).

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #8)
> I think it's pretty sound. Think of it this way:
> * The flag is valid for any element that was traversed in this traversal.
> * If two elements with different parents are going to share style, we must
> have traversed both of their parents in this traversal, since we always
> start at a single root.
> 
> Therefore, the flag is always up to date for the parents in question.

As long as we don't prime the cache with cousins, I suppose, yeah. I like the fix, simple enough :)

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Chris Peterson [:cpeterson]]

status-firefox56 = affected

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

we see a memory improvement from this change:
== Change summary for alert #8603 (as of August 05 2017 09:26 UTC) ==

Improvements:

  3%  Heap Unclassified summary linux64-stylo opt stylo     79,185,728.24 -> 77,204,267.77

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8603

Comment 14

[Bobby Holley (:bholley)]

Great! This basically counter-acts the regression in bug 1381821, and additionally allows bug 1368290 (which landed in the interim) to have an effect.

Comment 15

[Chris Peterson [:cpeterson]]

Blocking bug 1381147 because we'd like to uplift this fix before starting the Stylo experiment on Beta 56.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I just wanted to say, now that I read this patch, that it's very spiffy.   ;)

Comment 17

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #16)
> I just wanted to say, now that I read this patch, that it's very spiffy.   ;)

Thanks! :-)

Comment 18

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.