1360767 - stylo: Speed up stylist rebuilds

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

I have patches to optimize some of this stuff.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Factor out the various things stylist does per-selector into helpers. v1

This makes things more readable, and easier to profile (since the Gecko
profiler doesn't have any intra-function granularity, so I tend to make
helpers non-inline when drilling down into what's expensive).

It allows us to iterate the selector list once, which could help cache locality
for rules with many selectors.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use a single rule hash for both lower_name and name. v1

Right now in the common case we're doing twice the work during stylist update,
and also checking is_html_element_in_html_document twice during lookup. This
patch aligns us with what Gecko does.

MozReview-Commit-ID: D4TyG30BP8C

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Clean up note_selector a bit and stop handling combinators in two places. v1

MozReview-Commit-ID: HhVi72K4yp0

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Make note_selector more efficient. v1

My measurements are inconclusive as to whether this really moves the
needle, but it seems like the right way to do it in any case.

MozReview-Commit-ID: 7OuCc2aQ7jH

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Factor out the various things stylist does per-selector into helpers. v2

Rebased.

MozReview-Commit-ID: GdFtgrbYCIu

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863086 [details] [diff] [review]
Part 2 - Use a single rule hash for both lower_name and name. v1

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

I'm not sure this patch is correct, mind to explain why it is?

::: servo/components/style/stylist.rs
@@ +1262,5 @@
> +            // of whether it's an html element in an html document (in which case
> +            // we match against lower_name) or not (in which case we match against
> +            // name).
> +            if name != lower_name {
> +                find_push(&mut self.local_name_hash, lower_name, rule.clone());

Hmmm... I may be missing something, but what prevents us to match incorrectly?

i.e., if I have an non-HTML element, with a local name which is all lowercase, and a selector that is _not_ lowercase, we won't be matching it before this patch, but we would with it, right?

 * Can that happen?
 * Do we have tests for this?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863087 [details] [diff] [review]
Part 3 - Clean up note_selector a bit and stop handling combinators in two places. v1

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

Now we don't have combinators in :not this is fine, but if we ever do that we need to revert this patch... I think a comment would be nice, at least.

r=me if you think this is worth.

::: servo/components/style/restyle_hints.rs
@@ -490,5 @@
> -
> -    fn visit_complex_selector(&mut self,
> -                              _: SelectorIter<SelectorImpl>,
> -                              combinator: Option<Combinator>) -> bool {
> -        self.hint |= combinator_to_restyle_hint(combinator);

So the difference with this is that this handled correctly combinators inside :not, etc.

I think we've removed that, but perhaps it's worth to keep it anyway? (or a comment, perhaps?)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863088 [details] [diff] [review]
Part 4 - Make note_selector more efficient. v1

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

::: servo/components/style/restyle_hints.rs
@@ +545,4 @@
>              if !visitor.sensitivities.is_empty() {
> +                let mut hint = combinator_to_restyle_hint(combinator);
> +                let dep_selector;
> +                if index == 0 {

I'm having trouble to see how this is right, too.

So, let's say we have a selector like:

.foo::before.

Before this patch, we're inserting the descendants hint because the selector is a pseudo-element, so we need to arrive to ::before.

After this, we'd traverse `.foo`, then set `index` to 1, and then never reach this code. Is this the intention? I think this only works now because we still restyle all descendants, but...

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863089 [details] [diff] [review]
Part 1 - Factor out the various things stylist does per-selector into helpers. v2

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

This is quite nice, thanks for cleaning this up :)

Comment 10

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Comment on attachment 8863086 [details] [diff] [review]
> Part 2 - Use a single rule hash for both lower_name and name. v1
> 
> Review of attachment 8863086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure this patch is correct, mind to explain why it is?
> 
> ::: servo/components/style/stylist.rs
> @@ +1262,5 @@
> > +            // of whether it's an html element in an html document (in which case
> > +            // we match against lower_name) or not (in which case we match against
> > +            // name).
> > +            if name != lower_name {
> > +                find_push(&mut self.local_name_hash, lower_name, rule.clone());
> 
> Hmmm... I may be missing something, but what prevents us to match
> incorrectly?
> 
> i.e., if I have an non-HTML element, with a local name which is all
> lowercase, and a selector that is _not_ lowercase, we won't be matching it
> before this patch, but we would with it, right?

We will find it in the rule hash where we didn't before, but selector matching will still reject it. That's a tradeoff we have to make in order to avoid calling is_html_element_in_html_document() twice inserting into the rule hash twice.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> Comment on attachment 8863088 [details] [diff] [review]
> Part 4 - Make note_selector more efficient. v1
> 
> Review of attachment 8863088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/restyle_hints.rs
> @@ +545,4 @@
> >              if !visitor.sensitivities.is_empty() {
> > +                let mut hint = combinator_to_restyle_hint(combinator);
> > +                let dep_selector;
> > +                if index == 0 {
> 
> I'm having trouble to see how this is right, too.
> 
> So, let's say we have a selector like:
> 
> .foo::before.
> 
> Before this patch, we're inserting the descendants hint because the selector
> is a pseudo-element, so we need to arrive to ::before.
> 
> After this, we'd traverse `.foo`, then set `index` to 1, and then never
> reach this code. Is this the intention? I think this only works now because
> we still restyle all descendants, but...

Sorry, this should have been sequence_start. I'll attach a patch.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 4 - Make note_selector more efficient. v2

MozReview-Commit-ID: 7OuCc2aQ7jH

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863086 [details] [diff] [review]
Part 2 - Use a single rule hash for both lower_name and name. v1

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

Ok, makes sense, thanks for explaining.

r=me with a note saying that either where we insert the map, or where the member is defined.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863104 [details] [diff] [review]
Part 4 - Make note_selector more efficient. v2

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

ok, r=me

::: servo/components/style/restyle_hints.rs
@@ +545,4 @@
>              if !visitor.sensitivities.is_empty() {
> +                let mut hint = combinator_to_restyle_hint(combinator);
> +                let dep_selector;
> +                if sequence_start == 0 {

nit: perhaps:

let dep_selector = if sequence_start == 0 { ... } else { ... };

Comment 15

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> Comment on attachment 8863104 [details] [diff] [review]
> Part 4 - Make note_selector more efficient. v2
> 
> Review of attachment 8863104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ok, r=me
> 
> ::: servo/components/style/restyle_hints.rs
> @@ +545,4 @@
> >              if !visitor.sensitivities.is_empty() {
> > +                let mut hint = combinator_to_restyle_hint(combinator);
> > +                let dep_selector;
> > +                if sequence_start == 0 {
> 
> nit: perhaps:
> 
> let dep_selector = if sequence_start == 0 { ... } else { ... };

I did this at first, but then decided that the side-effect mutation to |hint| in the first branch made this less understandable.

Comment 16

[Bobby Holley (:bholley)]

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

Comment 17

[Bobby Holley (:bholley)]

Final try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2442f4154724142554b6bf81414f23ca293df9