diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 65bd0944b1a..2f83d43c893 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; @@ -165,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); @@ -315,28 +340,26 @@ 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())) } } +#[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. @@ -505,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/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/restyle_hints.rs b/components/style/restyle_hints.rs index cbe10f3ed41..9d3d8814ac7 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -417,32 +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::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, @@ -509,7 +483,6 @@ struct Dependency { struct SensitivitiesVisitor { sensitivities: Sensitivities, hint: RestyleHint, - needs_revalidation: bool, } impl SelectorVisitor for SensitivitiesVisitor { @@ -519,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 } @@ -529,11 +501,6 @@ impl SelectorVisitor for SensitivitiesVisitor { if !self.sensitivities.attrs { self.sensitivities.attrs = is_attr_selector(s); - self.needs_revalidation = true; - } - - if !self.needs_revalidation { - self.needs_revalidation = needs_cache_revalidation(s); } true @@ -570,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 { @@ -618,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, @@ -628,8 +590,6 @@ impl DependencySet { } } - - needs_revalidation } /// Create an empty `DependencySet`. 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 2ce855b1121..a990dc253b9 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; @@ -114,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, @@ -332,10 +333,23 @@ 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.selectors_for_cache_revalidation.push(selector.clone()); + 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(); + + // 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); + } } } } @@ -836,7 +850,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(), @@ -902,6 +916,74 @@ 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 + }, + Component::NonTSPseudoClass(ref p) if p.needs_cache_revalidation() => { + 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). 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"]);