I have patches to optimize some of this stuff.

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.

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

MozReview-Commit-ID: HhVi72K4yp0

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

Rebased. MozReview-Commit-ID: GdFtgrbYCIu

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 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 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 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 :)

(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.

(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.

MozReview-Commit-ID: 7OuCc2aQ7jH

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 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 { ... };

(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.

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