Bug 1360399: Don't deduplicate revalidation selectors. r?bholley draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 28 Apr 2017 02:15:19 +0200
changeset 569891 43d7e6f532bfd9ceccbbd6ae6948fb3585a2bce1
parent 569890 5780512b2a9691f3284ace7fcf89f7209261c145
child 569892 44b2150f2b5c47eb220804647391139f93c18d67
push id56301
push userbmo:emilio+bugs@crisal.io
push dateFri, 28 Apr 2017 00:33:33 +0000
reviewersbholley
bugs1360399
milestone55.0a1
Bug 1360399: Don't deduplicate revalidation selectors. r?bholley It's unfortunate, but it's a correctness issue. I was looking at the expectations update here: * https://hg.mozilla.org/integration/autoland/rev/659cddddd434 And investigating it I realised that it's wrong to coalesce selectors like that, because we keep the bloom filter flags. So in the test cases disabled, we have a selector that looks like this: msub > :not(:first-child), msup > :not(:first-child), msubsup > :not(:first-child), mmultiscripts > :not(:first-child) { -moz-script-level: +1; -moz-math-display: inline; } And an element that looks like this: <msubsup><mi></mi><mi></mi></msubsup> We're only inserting the first selector msub > :not(:first-child) into the set, so when we're going to match the <mi> elements we fast-reject it in both cases due to the bloom filter, so they share style. I can't see an easy way to fix this keeping the deduplication. If we keep it, we need to remove the bloom filter optimization, which means that we'd trash the cache for every first-child in the document (the :not(:first-child) effectively becomes a global rule). MozReview-Commit-ID: 9VPkmdj9zDg
servo/components/selectors/parser.rs
servo/components/style/matching.rs
servo/components/style/stylist.rs
--- a/servo/components/selectors/parser.rs
+++ b/servo/components/selectors/parser.rs
@@ -471,22 +471,22 @@ pub enum Component<Impl: SelectorImpl> {
     AttrPrefixNeverMatch(AttrSelector<Impl>, Impl::AttrValue),  // empty value
     AttrSubstringNeverMatch(AttrSelector<Impl>, Impl::AttrValue),  // empty value
     AttrSuffixNeverMatch(AttrSelector<Impl>, Impl::AttrValue),  // empty value
 
     // Pseudo-classes
     //
     // CSS3 Negation only takes a simple simple selector, but we still need to
     // treat it as a compound selector because it might be a type selector which
-    // we represent as a namespace and and localname.
+    // we represent as a namespace and a localname.
     //
-    // Note: if/when we upgrade this to CSS4, which supports combinators, we need
-    // to think about how this should interact with visit_complex_selector, and
-    // what the consumers of those APIs should do about the presence of combinators
-    // in negation.
+    // Note: if/when we upgrade this to CSS4, which supports combinators, we
+    // need to think about how this should interact with visit_complex_selector,
+    // and what the consumers of those APIs should do about the presence of
+    // combinators in negation.
     Negation(Box<[Component<Impl>]>),
     FirstChild, LastChild, OnlyChild,
     Root,
     Empty,
     NthChild(i32, i32),
     NthLastChild(i32, i32),
     NthOfType(i32, i32),
     NthLastOfType(i32, i32),
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -194,16 +194,18 @@ fn element_matches_candidate<E: TElement
                    shared, bloom, info, selector_flags_map) {
         miss!(Revalidation)
     }
 
     let data = candidate_element.borrow_data().unwrap();
     debug_assert!(data.has_current_styles());
     let current_styles = data.styles();
 
+    debug!("Sharing style between {:?} and {:?}", element, candidate_element);
+
     Ok(current_styles.primary.clone())
 }
 
 fn has_presentational_hints<E: TElement>(element: &E) -> bool {
     let mut hints = ForgetfulSink::new();
     element.synthesize_presentational_hints_for_legacy_attributes(&mut hints);
     !hints.is_empty()
 }
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -331,30 +331,22 @@ impl Stylist {
                                              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();
+                            // For revalidation, we can skip everything left of
+                            // the first ancestor combinator.
+                            let revalidation_sel =
+                                selector.inner.slice_to_first_ancestor_combinator();
 
-                            // Because of the slicing we do above, we can often end up with
-                            // adjacent duplicate selectors when we have selectors like
-                            // body > foo, td > foo, th > foo, etc. Doing a check for
-                            // adjacent duplicates here reduces the number of revalidation
-                            // selectors for Gecko's UA sheet by 30%.
-                            let duplicate = self.selectors_for_cache_revalidation.last()
-                                                .map_or(false, |x| x.complex == revalidation_sel.complex);
-                            if !duplicate {
-                                self.selectors_for_cache_revalidation.push(revalidation_sel);
-                            }
+                            self.selectors_for_cache_revalidation.push(revalidation_sel);
                         }
                     }
                 }
                 CssRule::Import(ref import) => {
                     let import = import.read_with(guard);
                     self.add_stylesheet(&import.stylesheet, guard, extra_data)
                 }
                 CssRule::Keyframes(ref keyframes_rule) => {
@@ -835,17 +827,17 @@ impl Stylist {
     /// Computes the match results of a given element against the set of
     /// revalidation selectors.
     pub fn match_revalidation_selectors<E, F>(&self,
                                               element: &E,
                                               bloom: &BloomFilter,
                                               flags_setter: &mut F)
                                               -> BitVec
         where E: TElement,
-              F: FnMut(&E, ElementSelectorFlags)
+              F: FnMut(&E, ElementSelectorFlags),
     {
         use selectors::matching::StyleRelations;
         use selectors::matching::matches_selector;
 
         let len = self.selectors_for_cache_revalidation.len();
         let mut results = BitVec::from_elem(len, false);
 
         for (i, ref selector) in self.selectors_for_cache_revalidation
@@ -948,18 +940,18 @@ impl SelectorVisitor for RevalidationVis
             Component::NthLastChild(..) |
             Component::NthOfType(..) |
             Component::NthLastOfType(..) |
             Component::FirstOfType |
             Component::LastOfType |
             Component::OnlyOfType => {
                 false
             },
-            Component::NonTSPseudoClass(ref p) if p.needs_cache_revalidation() => {
-                false
+            Component::NonTSPseudoClass(ref p) => {
+                !p.needs_cache_revalidation()
             },
             _ => {
                 true
             }
         }
     }
 }
 
@@ -975,18 +967,18 @@ pub fn needs_revalidation(selector: &Sel
     let mut iter = selector.inner.complex.iter();
     for ss in &mut iter {
         if !ss.visit(&mut visitor) {
             return true;
         }
     }
 
     // If none of the simple selectors in the rightmost sequence required
-    // revalidaiton, we need revalidation if and only if the combinator is
-    // a sibling combinator.
+    // revalidation, we need revalidation if and only if the combinator is a
+    // sibling combinator.
     iter.next_sequence().map_or(false, |c| c.is_sibling())
 }
 
 /// Map that contains the CSS rules for a specific PseudoElement
 /// (or lack of PseudoElement).
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 struct PerPseudoElementSelectorMap {
     /// Rules from user agent stylesheets