Auto merge of #16659 - bholley:faster_stylist_rebuild, r=emilio

Speed up stylist rebuilds

Reviewed in https://bugzilla.mozilla.org/show_bug.cgi?id=1360767

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16659)
<!-- Reviewable:end -->

components/selectors/parser.rs

@@ -1457,7 +1457,7 @@ pub mod tests {
             where V: SelectorVisitor<Impl = Self::Impl> { true }
     }
 
-    #[derive(PartialEq, Debug)]
+    #[derive(Clone, PartialEq, Debug)]
     pub struct DummySelectorImpl;
 
     #[derive(Default)]

components/style/restyle_hints.rs

@@ -18,7 +18,7 @@ use selectors::{Element, MatchAttr};
 use selectors::matching::{ElementSelectorFlags, StyleRelations};
 use selectors::matching::matches_selector;
 use selectors::parser::{AttrSelector, Combinator, Component, Selector};
-use selectors::parser::{SelectorInner, SelectorIter, SelectorMethods};
+use selectors::parser::{SelectorInner, SelectorMethods};
 use selectors::visitor::SelectorVisitor;
 use std::clone::Clone;
 
@@ -482,27 +482,13 @@ struct Dependency {
 /// of them is sensitive to attribute or state changes.
 struct SensitivitiesVisitor {
     sensitivities: Sensitivities,
-    hint: RestyleHint,
 }
 
 impl SelectorVisitor for SensitivitiesVisitor {
     type Impl = SelectorImpl;
-
-    fn visit_complex_selector(&mut self,
-                              _: SelectorIter<SelectorImpl>,
-                              combinator: Option<Combinator>) -> bool {
-        self.hint |= combinator_to_restyle_hint(combinator);
-
-        true
-    }
-
     fn visit_simple_selector(&mut self, s: &Component<SelectorImpl>) -> bool {
         self.sensitivities.states.insert(selector_to_state(s));
-
+        self.sensitivities.attrs |= is_attr_selector(s);
-        if !self.sensitivities.attrs {
-            self.sensitivities.attrs = is_attr_selector(s);
-        }
-
         true
     }
 }
@@ -539,56 +525,63 @@ impl DependencySet {
 
     /// Adds a selector to this `DependencySet`.
     pub fn note_selector(&mut self, selector: &Selector<SelectorImpl>) {
-        let mut is_pseudo_element = selector.pseudo_element.is_some();
-
-        let mut next = Some(selector.inner.complex.clone());
         let mut combinator = None;
+        let mut iter = selector.inner.complex.iter();
+        let mut index = 0;
 
-        while let Some(current) = next.take() {
+        loop {
-            // Set up our visitor.
+            let sequence_start = index;
             let mut visitor = SensitivitiesVisitor {
-                sensitivities: Sensitivities::new(),
+                sensitivities: Sensitivities::new()
-                hint: combinator_to_restyle_hint(combinator),
             };
 
-            if is_pseudo_element {
+            // Visit all the simple selectors in this sequence.
-                // TODO(emilio): use more fancy restyle hints to avoid restyling
+            //
-                // the whole subtree when pseudos change.
+            // Note that this works because we can't have combinators nested
-                //
+            // inside simple selectors (i.e. in :not() or :-moz-any()). If we
-                // We currently need is_pseudo_element to handle eager pseudos
+            // ever support that we'll need to visit complex selectors as well.
-                // (so the style the parent stores doesn't become stale), and
+            for ss in &mut iter {
-                // restyle_descendants to handle all of them (::before and
+                ss.visit(&mut visitor);
-                // ::after, because we find them in the subtree, and other lazy
+                index += 1; // Account for the simple selector.
-                // pseudos for the same reason).
-                visitor.hint |= RESTYLE_SELF | RESTYLE_DESCENDANTS;
-                is_pseudo_element = false;
             }
 
-            {
+            // If we found a sensitivity, add an entry in the dependency set.
-                // Visit all the simple selectors.
-                let mut iter = current.iter();
-                let mut index = 0usize;
-                for ss in &mut iter {
-                    ss.visit(&mut visitor);
-                    index += 1;
-                }
-
-                // Prepare the next sequence of simple selectors.
-                if let Some(next_combinator) = iter.next_sequence() {
-                    next = Some(current.slice_from(index + 1));
-                    combinator = Some(next_combinator);
-                }
-            }
-
-            // Note what we found.
             if !visitor.sensitivities.is_empty() {
+                let mut hint = combinator_to_restyle_hint(combinator);
+                let dep_selector;
+                if sequence_start == 0 {
+                    if selector.pseudo_element.is_some() {
+                        // TODO(emilio): use more fancy restyle hints to avoid
+                        // restyling the whole subtree when pseudos change.
+                        //
+                        // We currently need is_pseudo_element to handle eager
+                        // pseudos (so the style the parent stores doesn't
+                        // become stale), and restyle_descendants to handle all
+                        // of them (::before and ::after, because we find them
+                        // in the subtree, and other lazy pseudos for the same
+                        // reason).
+                        hint |= RESTYLE_SELF | RESTYLE_DESCENDANTS;
+                    }
+
+                    // Reuse the bloom hashes if this is the base selector.
+                    dep_selector = selector.inner.clone();
+                } else {
+                    dep_selector = SelectorInner::new(selector.inner.complex.slice_from(sequence_start));
+                }
+
                 self.add_dependency(Dependency {
                     sensitivities: visitor.sensitivities,
-                    hint: visitor.hint,
+                    hint: hint,
-                    selector: SelectorInner::new(current),
+                    selector: dep_selector,
-                })
+                });
             }
 
+            combinator = iter.next_sequence();
+            if combinator.is_none() {
+                break;
+            }
+
+            index += 1; // Account for the combinator.
         }
     }
 

components/style/stylist.rs

@@ -313,38 +313,13 @@ impl Stylist {
                 CssRule::Style(ref locked) => {
                     let style_rule = locked.read_with(&guard);
                     self.num_declarations += style_rule.block.read_with(&guard).len();
-
                     for selector in &style_rule.selectors.0 {
                         self.num_selectors += 1;
-                        let map = if let Some(ref pseudo) = selector.pseudo_element {
+                        self.add_rule_to_map(guard, selector, locked, stylesheet);
-                            self.pseudos_map
+                        self.dependencies.note_selector(selector);
-                                .entry(pseudo.clone())
+                        self.note_for_revalidation(selector);
-                                .or_insert_with(PerPseudoElementSelectorMap::new)
-                                .borrow_for_origin(&stylesheet.origin)
-                        } else {
-                            self.element_map.borrow_for_origin(&stylesheet.origin)
-                        };
-
-                        map.insert(Rule::new(guard,
-                                             selector.inner.clone(),
-                                             locked.clone(),
-                                             self.rules_source_order,
-                                             selector.specificity));
                     }
                     self.rules_source_order += 1;
-
-                    for selector in &style_rule.selectors.0 {
-                        self.dependencies.note_selector(selector);
-
-                        if needs_revalidation(selector) {
-                            // For revalidation, we can skip everything left of
-                            // the first ancestor combinator.
-                            let revalidation_sel =
-                                selector.inner.slice_to_first_ancestor_combinator();
-
-                            self.selectors_for_cache_revalidation.push(revalidation_sel);
-                        }
-                    }
                 }
                 CssRule::Import(ref import) => {
                     let import = import.read_with(guard);
@@ -374,6 +349,38 @@ impl Stylist {
         });
     }
 
+    #[inline]
+    fn add_rule_to_map(&mut self,
+                       guard: &SharedRwLockReadGuard,
+                       selector: &Selector<SelectorImpl>,
+                       rule: &Arc<Locked<StyleRule>>,
+                       stylesheet: &Stylesheet)
+    {
+        let map = if let Some(ref pseudo) = selector.pseudo_element {
+            self.pseudos_map
+                .entry(pseudo.clone())
+                .or_insert_with(PerPseudoElementSelectorMap::new)
+                .borrow_for_origin(&stylesheet.origin)
+        } else {
+            self.element_map.borrow_for_origin(&stylesheet.origin)
+        };
+
+        map.insert(Rule::new(guard,
+                             selector.inner.clone(),
+                             rule.clone(),
+                             self.rules_source_order,
+                             selector.specificity));
+    }
+
+    #[inline]
+    fn note_for_revalidation(&mut self, selector: &Selector<SelectorImpl>) {
+        if needs_revalidation(selector) {
+            // For revalidation, we can skip everything left of the first ancestor
+            // combinator.
+            let revalidation_sel = selector.inner.slice_to_first_ancestor_combinator();
+            self.selectors_for_cache_revalidation.push(revalidation_sel);
+        }
+    }
 
     /// Computes the style for a given "precomputed" pseudo-element, taking the
     /// universal rules and applying them.
@@ -1043,9 +1050,6 @@ pub struct SelectorMap {
     pub class_hash: FnvHashMap<Atom, Vec<Rule>>,
     /// A hash from local name to rules which contain that local name selector.
     pub local_name_hash: FnvHashMap<LocalName, Vec<Rule>>,
-    /// Same as local_name_hash, but keys are lower-cased.
-    /// For HTML elements in HTML documents.
-    pub lower_local_name_hash: FnvHashMap<LocalName, Vec<Rule>>,
     /// Rules that don't have ID, class, or element selectors.
     pub other_rules: Vec<Rule>,
     /// Whether this hash is empty.
@@ -1064,7 +1068,6 @@ impl SelectorMap {
             id_hash: HashMap::default(),
             class_hash: HashMap::default(),
             local_name_hash: HashMap::default(),
-            lower_local_name_hash: HashMap::default(),
             other_rules: Vec::new(),
             empty: true,
         }
@@ -1113,14 +1116,9 @@ impl SelectorMap {
                                                       cascade_level);
         });
 
-        let local_name_hash = if element.is_html_element_in_html_document() {
-            &self.lower_local_name_hash
-        } else {
-            &self.local_name_hash
-        };
         SelectorMap::get_matching_rules_from_hash(element,
                                                   parent_bf,
-                                                  local_name_hash,
+                                                  &self.local_name_hash,
                                                   element.get_local_name(),
                                                   matching_rules_list,
                                                   relations,
@@ -1253,8 +1251,22 @@ impl SelectorMap {
         }
 
         if let Some(LocalNameSelector { name, lower_name }) = SelectorMap::get_local_name(&rule) {
-            find_push(&mut self.local_name_hash, name, rule.clone());
+            // If the local name in the selector isn't lowercase, insert it into
-            find_push(&mut self.lower_local_name_hash, lower_name, rule);
+            // the rule hash twice. This means that, during lookup, we can always
+            // find the rules based on the local name of the element, regardless
+            // 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).
+            //
+            // In the case of a non-html-element-in-html-document with a
+            // lowercase localname and a non-lowercase selector, the rulehash
+            // lookup may produce superfluous selectors, but the subsequent
+            // selector matching work will filter them out.
+            if name != lower_name {
+                find_push(&mut self.local_name_hash, lower_name, rule.clone());
+            }
+            find_push(&mut self.local_name_hash, name, rule);
+
             return;
         }