From dd4575c23a46aaaf0764705ba9424bee491f902e Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 24 Apr 2017 12:21:47 -0700 Subject: [PATCH 1/7] Separate revalidation checking from attribute sensitivity checking. The current code is busted because: * It unconditionally flags for revalidation if the sensitivities are not yet sensitive to attributes. * It uses is_attr_selector, despite the fact that ID and Class are both handled by the style sharing cache and should not trigger revalidation. MozReview-Commit-ID: C7d8QrdKcFm --- components/style/restyle_hints.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index cbe10f3ed41..65ee8b22a6c 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -420,11 +420,17 @@ fn is_attr_selector(sel: &Component) -> bool { /// Whether a selector containing this simple selector needs to be explicitly /// matched against both the style sharing cache entry and the candidate. /// -/// /// We use this for selectors that can have different matching behavior between /// siblings that are otherwise identical as far as the cache is concerned. fn needs_cache_revalidation(sel: &Component) -> bool { match *sel { + Component::AttrExists(_) | + Component::AttrEqual(_, _, _) | + Component::AttrIncludes(_, _) | + Component::AttrDashMatch(_, _) | + Component::AttrPrefixMatch(_, _) | + Component::AttrSubstringMatch(_, _) | + Component::AttrSuffixMatch(_, _) | Component::Empty | Component::FirstChild | Component::LastChild | @@ -529,7 +535,6 @@ impl SelectorVisitor for SensitivitiesVisitor { if !self.sensitivities.attrs { self.sensitivities.attrs = is_attr_selector(s); - self.needs_revalidation = true; } if !self.needs_revalidation { From 2df9dc2b030b7f3e1421320969c1b1eeea825470 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 24 Apr 2017 13:06:44 -0700 Subject: [PATCH 2/7] Use a different visitor pass for gathering revalidation selectors. This allows us to ignore everything to the left of the rightmost simple selector. MozReview-Commit-ID: 6I4VzKos22n --- components/style/restyle_hints.rs | 49 +------------------ components/style/stylist.rs | 79 +++++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 51 deletions(-) diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 65ee8b22a6c..9d3d8814ac7 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -417,38 +417,6 @@ fn is_attr_selector(sel: &Component) -> bool { } } -/// Whether a selector containing this simple selector needs to be explicitly -/// matched against both the style sharing cache entry and the candidate. -/// -/// We use this for selectors that can have different matching behavior between -/// siblings that are otherwise identical as far as the cache is concerned. -fn needs_cache_revalidation(sel: &Component) -> bool { - match *sel { - Component::AttrExists(_) | - Component::AttrEqual(_, _, _) | - Component::AttrIncludes(_, _) | - Component::AttrDashMatch(_, _) | - Component::AttrPrefixMatch(_, _) | - Component::AttrSubstringMatch(_, _) | - Component::AttrSuffixMatch(_, _) | - Component::Empty | - Component::FirstChild | - Component::LastChild | - Component::OnlyChild | - Component::NthChild(..) | - Component::NthLastChild(..) | - Component::NthOfType(..) | - Component::NthLastOfType(..) | - Component::FirstOfType | - Component::LastOfType | - Component::OnlyOfType => true, - // FIXME(emilio): This sets the "revalidation" flag for :any, which is - // probably expensive given we use it a lot in UA sheets. - Component::NonTSPseudoClass(ref p) => p.state_flag().is_empty(), - _ => false, - } -} - fn combinator_to_restyle_hint(combinator: Option) -> RestyleHint { match combinator { None => RESTYLE_SELF, @@ -515,7 +483,6 @@ struct Dependency { struct SensitivitiesVisitor { sensitivities: Sensitivities, hint: RestyleHint, - needs_revalidation: bool, } impl SelectorVisitor for SensitivitiesVisitor { @@ -525,7 +492,6 @@ impl SelectorVisitor for SensitivitiesVisitor { _: SelectorIter, combinator: Option) -> bool { self.hint |= combinator_to_restyle_hint(combinator); - self.needs_revalidation |= self.hint.contains(RESTYLE_LATER_SIBLINGS); true } @@ -537,10 +503,6 @@ impl SelectorVisitor for SensitivitiesVisitor { self.sensitivities.attrs = is_attr_selector(s); } - if !self.needs_revalidation { - self.needs_revalidation = needs_cache_revalidation(s); - } - true } } @@ -575,22 +537,18 @@ impl DependencySet { } } - /// Adds a selector to this `DependencySet`, and returns whether it may need - /// cache revalidation, that is, whether two siblings of the same "shape" - /// may have different style due to this selector. - pub fn note_selector(&mut self, selector: &Selector) -> bool { + /// Adds a selector to this `DependencySet`. + pub fn note_selector(&mut self, selector: &Selector) { let mut is_pseudo_element = selector.pseudo_element.is_some(); let mut next = Some(selector.inner.complex.clone()); let mut combinator = None; - let mut needs_revalidation = false; while let Some(current) = next.take() { // Set up our visitor. let mut visitor = SensitivitiesVisitor { sensitivities: Sensitivities::new(), hint: combinator_to_restyle_hint(combinator), - needs_revalidation: false, }; if is_pseudo_element { @@ -623,7 +581,6 @@ impl DependencySet { } // Note what we found. - needs_revalidation |= visitor.needs_revalidation; if !visitor.sensitivities.is_empty() { self.add_dependency(Dependency { sensitivities: visitor.sensitivities, @@ -633,8 +590,6 @@ impl DependencySet { } } - - needs_revalidation } /// Create an empty `DependencySet`. diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 2ce855b1121..08e87601fbe 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -26,7 +26,8 @@ use selectors::Element; use selectors::bloom::BloomFilter; use selectors::matching::{AFFECTED_BY_STYLE_ATTRIBUTE, AFFECTED_BY_PRESENTATIONAL_HINTS}; use selectors::matching::{ElementSelectorFlags, StyleRelations, matches_selector}; -use selectors::parser::{Component, Selector, SelectorInner, LocalName as LocalNameSelector}; +use selectors::parser::{Component, Selector, SelectorInner, SelectorMethods, LocalName as LocalNameSelector}; +use selectors::visitor::SelectorVisitor; use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards}; use sink::Push; use smallvec::VecLike; @@ -332,9 +333,9 @@ impl Stylist { self.rules_source_order += 1; for selector in &style_rule.selectors.0 { - let needs_cache_revalidation = - self.dependencies.note_selector(selector); - if needs_cache_revalidation { + self.dependencies.note_selector(selector); + + if needs_revalidation(selector) { self.selectors_for_cache_revalidation.push(selector.clone()); } } @@ -902,6 +903,76 @@ impl Drop for Stylist { } } +/// Visitor determine whether a selector requires cache revalidation. +/// +/// Note that we just check simple selectors and eagerly return when the first +/// need for revalidation is found, so we don't need to store state on the visitor. +struct RevalidationVisitor; + +impl SelectorVisitor for RevalidationVisitor { + type Impl = SelectorImpl; + + /// Check whether a rightmost sequence of simple selectors containing this + /// simple selector to be explicitly matched against both the style sharing + /// cache entry and the candidate. + /// + /// We use this for selectors that can have different matching behavior between + /// siblings that are otherwise identical as far as the cache is concerned. + fn visit_simple_selector(&mut self, s: &Component) -> bool { + match *s { + Component::AttrExists(_) | + Component::AttrEqual(_, _, _) | + Component::AttrIncludes(_, _) | + Component::AttrDashMatch(_, _) | + Component::AttrPrefixMatch(_, _) | + Component::AttrSubstringMatch(_, _) | + Component::AttrSuffixMatch(_, _) | + Component::Empty | + Component::FirstChild | + Component::LastChild | + Component::OnlyChild | + Component::NthChild(..) | + Component::NthLastChild(..) | + Component::NthOfType(..) | + Component::NthLastOfType(..) | + Component::FirstOfType | + Component::LastOfType | + Component::OnlyOfType => { + false + }, + // FIXME(emilio): This sets the "revalidation" flag for :any, which is + // probably expensive given we use it a lot in UA sheets. + Component::NonTSPseudoClass(ref p) if p.state_flag().is_empty() => { + false + }, + _ => { + true + } + } + } +} + +/// Returns true if the given selector needs cache revalidation. +pub fn needs_revalidation(selector: &Selector) -> bool { + let mut visitor = RevalidationVisitor; + + // We only need to consider the rightmost sequence of simple selectors, so + // we can stop at the first combinator. This is because: + // * If it's an ancestor combinator, we can ignore everything to the left + // because matching won't differ between siblings. + // * If it's a sibling combinator, then we know we need revalidation. + 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. + iter.next_sequence().map_or(false, |c| c.is_sibling()) +} /// Map that contains the CSS rules for a specific PseudoElement /// (or lack of PseudoElement). From 32c624e5852a91fbcd2fcc495cacd00a63452c6a Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 26 Apr 2017 14:01:36 -0700 Subject: [PATCH 3/7] Require Clone for SelectorImpl so that all the types that are parameterized on it can derive(Clone). It's not clear to me why this is a requirement, but it seems to be one. MozReview-Commit-ID: JM0DKjHHfT --- components/selectors/parser.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 65bd0944b1a..3d05f7ec734 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -42,7 +42,11 @@ macro_rules! with_all_bounds { /// This trait allows to define the parser implementation in regards /// of pseudo-classes/elements - pub trait SelectorImpl: Sized { + /// + /// NB: We need Clone so that we can derive(Clone) on struct with that + /// are parameterized on SelectorImpl. See + /// https://github.com/rust-lang/rust/issues/26925 + pub trait SelectorImpl: Clone + Sized { type AttrValue: $($InSelector)*; type Identifier: $($InSelector)* + PrecomputedHash; type ClassName: $($InSelector)* + PrecomputedHash; @@ -321,22 +325,12 @@ impl ComplexSelector { } } +#[derive(Clone)] pub struct SelectorIter<'a, Impl: 'a + SelectorImpl> { iter: Rev>>, next_combinator: Option, } -// NB: Deriving this doesn't work for some reason, because it expects Impl to -// implement Clone. -impl<'a, Impl: 'a + SelectorImpl> Clone for SelectorIter<'a, Impl> { - fn clone(&self) -> Self { - SelectorIter { - iter: self.iter.clone(), - next_combinator: self.next_combinator.clone(), - } - } -} - impl<'a, Impl: 'a + SelectorImpl> SelectorIter<'a, Impl> { /// Prepares this iterator to point to the next sequence to the left, /// returning the combinator if the sequence was found. From 7e69cf82be31a38db8f5a1b3c6454fdf89c28b35 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 26 Apr 2017 13:48:13 -0700 Subject: [PATCH 4/7] Store SelectorInner and only revalidate up to the rightmost ancestor combinator. MozReview-Commit-ID: HiTGVhwuvCE --- components/selectors/parser.rs | 37 ++++++++++++++++++++++++++++++++++ components/style/stylist.rs | 6 +++--- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 3d05f7ec734..2f83d43c893 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -169,6 +169,27 @@ impl SelectorInner { } } + /// Creates a clone of this selector with everything to the left of + /// (and including) the rightmost ancestor combinator removed. So + /// the selector |span foo > bar + baz| will become |bar + baz|. + /// This is used for revalidation selectors in servo. + /// + /// The bloom filter hashes are copied, even though they correspond to + /// parts of the selector that have been stripped out, because they are + /// still useful for fast-rejecting the reduced selectors. + pub fn slice_to_first_ancestor_combinator(&self) -> Self { + let maybe_pos = self.complex.iter_raw() + .position(|s| s.as_combinator() + .map_or(false, |c| c.is_ancestor())); + match maybe_pos { + None => self.clone(), + Some(index) => SelectorInner { + complex: self.complex.slice_to(index), + ancestor_hashes: self.ancestor_hashes.clone(), + }, + } + } + /// Creates a SelectorInner from a Vec of Components. Used in tests. pub fn from_vec(vec: Vec>) -> Self { let complex = ComplexSelector::from_vec(vec); @@ -319,6 +340,14 @@ impl ComplexSelector { ComplexSelector(self.0.clone().slice_to(self.0.len() - index)) } + /// Returns a ComplexSelector identical to |self| but with the leftmost + /// |len() - index| entries removed. + pub fn slice_to(&self, index: usize) -> Self { + // Note that we convert the slice_to to slice_from because selectors are + // stored left-to-right but logical order is right-to-left. + ComplexSelector(self.0.clone().slice_from(self.0.len() - index)) + } + /// Creates a ComplexSelector from a vec of Components. Used in tests. pub fn from_vec(vec: Vec>) -> Self { ComplexSelector(ArcSlice::new(vec.into_boxed_slice())) @@ -499,6 +528,14 @@ impl Component { pub fn is_combinator(&self) -> bool { matches!(*self, Component::Combinator(_)) } + + /// Returns the value as a combinator if applicable, None otherwise. + pub fn as_combinator(&self) -> Option { + match *self { + Component::Combinator(c) => Some(c), + _ => None, + } + } } #[derive(Eq, PartialEq, Clone, Hash, Copy, Debug)] diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 08e87601fbe..78a77266800 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -115,7 +115,7 @@ pub struct Stylist { /// on state that is not otherwise visible to the cache, like attributes or /// tree-structural state like child index and pseudos). #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] - selectors_for_cache_revalidation: Vec>, + selectors_for_cache_revalidation: Vec>, /// The total number of selectors. num_selectors: usize, @@ -336,7 +336,7 @@ impl Stylist { self.dependencies.note_selector(selector); if needs_revalidation(selector) { - self.selectors_for_cache_revalidation.push(selector.clone()); + self.selectors_for_cache_revalidation.push(selector.inner.slice_to_first_ancestor_combinator()); } } } @@ -837,7 +837,7 @@ impl Stylist { for (i, ref selector) in self.selectors_for_cache_revalidation .iter().enumerate() { - results.set(i, matches_selector(&selector.inner, + results.set(i, matches_selector(selector, element, Some(bloom), &mut StyleRelations::empty(), From 25832b962f4b969e88b441ed4a1cf5fdbb579ada Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 26 Apr 2017 17:25:43 -0700 Subject: [PATCH 5/7] Check for adjacent duplicates when accumulating revalidation selectors. MozReview-Commit-ID: 4C8L5u1S1sL --- components/style/stylist.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 78a77266800..62f1795f25c 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -336,7 +336,20 @@ impl Stylist { self.dependencies.note_selector(selector); if needs_revalidation(selector) { - self.selectors_for_cache_revalidation.push(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); + } } } } From 6e413653e72fd1eb471d779be63bccfa05d7c47f Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 26 Apr 2017 18:16:10 -0700 Subject: [PATCH 6/7] Add unit tests for revalidation selectors. If these break, our cache may be subtly wrong in certain situations, which may be hard to detect. MozReview-Commit-ID: AXG2tpGnQ6k --- tests/unit/style/stylist.rs | 107 +++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/tests/unit/style/stylist.rs b/tests/unit/style/stylist.rs index 475eb7bc144..4f7ba303e08 100644 --- a/tests/unit/style/stylist.rs +++ b/tests/unit/style/stylist.rs @@ -4,15 +4,17 @@ use html5ever_atoms::LocalName; use selectors::parser::LocalName as LocalNameSelector; +use selectors::parser::Selector; use servo_atoms::Atom; use std::sync::Arc; use style::properties::{PropertyDeclarationBlock, PropertyDeclaration}; use style::properties::{longhands, Importance}; use style::rule_tree::CascadeLevel; -use style::selector_parser::SelectorParser; +use style::selector_parser::{SelectorImpl, SelectorParser}; use style::shared_lock::SharedRwLock; use style::stylesheets::StyleRule; use style::stylist::{Rule, SelectorMap}; +use style::stylist::needs_revalidation; use style::thread_state; /// Helper method to get some Rules from selector strings. @@ -56,6 +58,109 @@ fn get_mock_map(selectors: &[&str]) -> (SelectorMap, SharedRwLock) { (map, shared_lock) } +fn parse_selectors(selectors: &[&str]) -> Vec> { + selectors.iter() + .map(|x| SelectorParser::parse_author_origin_no_namespace(x).unwrap().0 + .into_iter() + .nth(0) + .unwrap()) + .collect() +} + +#[test] +fn test_revalidation_selectors() { + let test = parse_selectors(&[ + // Not revalidation selectors. + "div", + "#bar", + "div:not(.foo)", + "div span", + "div > span", + + // Attribute selectors. + "div[foo]", + "div:not([foo])", + "div[foo = \"bar\"]", + "div[foo ~= \"bar\"]", + "div[foo |= \"bar\"]", + "div[foo ^= \"bar\"]", + "div[foo $= \"bar\"]", + "div[foo *= \"bar\"]", + "*|div[foo][bar = \"baz\"]", + + // Non-state-based pseudo-classes. + "div:empty", + "div:first-child", + "div:last-child", + "div:only-child", + "div:nth-child(2)", + "div:nth-last-child(2)", + "div:nth-of-type(2)", + "div:nth-last-of-type(2)", + "div:first-of-type", + "div:last-of-type", + "div:only-of-type", + + // Note: it would be nice to test :moz-any and the various other non-TS + // pseudo classes supported by gecko, but we don't have access to those + // in these unit tests. :-( + + // Sibling combinators. + "span + div", + "span ~ div", + + // Revalidation selectors that will get sliced. + "td > h1[dir]", + "td > span + h1[dir]", + "table td > span + div ~ h1[dir]", + ]).into_iter() + .filter(|s| needs_revalidation(&s)) + .map(|s| s.inner.slice_to_first_ancestor_combinator().complex) + .collect::>(); + + let reference = parse_selectors(&[ + // Attribute selectors. + "div[foo]", + "div:not([foo])", + "div[foo = \"bar\"]", + "div[foo ~= \"bar\"]", + "div[foo |= \"bar\"]", + "div[foo ^= \"bar\"]", + "div[foo $= \"bar\"]", + "div[foo *= \"bar\"]", + "*|div[foo][bar = \"baz\"]", + + // Non-state-based pseudo-classes. + "div:empty", + "div:first-child", + "div:last-child", + "div:only-child", + "div:nth-child(2)", + "div:nth-last-child(2)", + "div:nth-of-type(2)", + "div:nth-last-of-type(2)", + "div:first-of-type", + "div:last-of-type", + "div:only-of-type", + + // Sibling combinators. + "span + div", + "span ~ div", + + // Revalidation selectors that got sliced. + "h1[dir]", + "span + h1[dir]", + "span + div ~ h1[dir]", + ]).into_iter() + .map(|s| s.inner.complex) + .collect::>(); + + assert_eq!(test.len(), reference.len()); + for (t, r) in test.into_iter().zip(reference.into_iter()) { + assert_eq!(t, r) + } +} + #[test] fn test_rule_ordering_same_specificity() { let (rules_list, _) = get_mock_rules(&["a.intro", "img.sidebar"]); From f4d81b010b621ceb47a35c921f8cdf1a7959dff9 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 26 Apr 2017 18:34:29 -0700 Subject: [PATCH 7/7] Properly handle -moz-any for revalidation selectors. Note that, while the comment is correct that there is a fair amount of -moz-any usage in the UA sheet, it's always used as an ancestor selector, and thus ignored for our purposes. Nevertheless, it's straightforward enough to support properly, so we do that here. MozReview-Commit-ID: Kz1yNfPUIaP --- components/style/gecko/selector_parser.rs | 6 ++++++ components/style/servo/selector_parser.rs | 5 +++++ components/style/stylist.rs | 4 +--- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index b87824fc33f..4add99e36e8 100644 --- a/components/style/gecko/selector_parser.rs +++ b/components/style/gecko/selector_parser.rs @@ -342,6 +342,12 @@ impl NonTSPseudoClass { apply_non_ts_list!(pseudo_class_state) } + /// Returns true if the given pseudoclass should trigger style sharing cache revalidation. + pub fn needs_cache_revalidation(&self) -> bool { + self.state_flag().is_empty() && + !matches!(*self, NonTSPseudoClass::MozAny(_)) + } + /// Convert NonTSPseudoClass to Gecko's CSSPseudoClassType. pub fn to_gecko_pseudoclasstype(&self) -> Option { macro_rules! gecko_type { diff --git a/components/style/servo/selector_parser.rs b/components/style/servo/selector_parser.rs index e0277b31480..0335b249cb1 100644 --- a/components/style/servo/selector_parser.rs +++ b/components/style/servo/selector_parser.rs @@ -235,6 +235,11 @@ impl NonTSPseudoClass { ServoNonZeroBorder => ElementState::empty(), } } + + /// Returns true if the given pseudoclass should trigger style sharing cache revalidation. + pub fn needs_cache_revalidation(&self) -> bool { + self.state_flag().is_empty() + } } /// The abstract struct we implement the selector parser implementation on top diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 62f1795f25c..a990dc253b9 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -953,9 +953,7 @@ impl SelectorVisitor for RevalidationVisitor { Component::OnlyOfType => { false }, - // FIXME(emilio): This sets the "revalidation" flag for :any, which is - // probably expensive given we use it a lot in UA sheets. - Component::NonTSPseudoClass(ref p) if p.state_flag().is_empty() => { + Component::NonTSPseudoClass(ref p) if p.needs_cache_revalidation() => { false }, _ => {