From 5b0d4de10cf492c030571cdc94caae7e8d9ba9ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 2 Aug 2017 17:35:57 -0500 Subject: [PATCH] servo: Merge #17939 - style: Avoid looping through every selector more than twice (from emilio:multiple-walk-selector); r=bzbarsky I've left the Invalidation stuff on its own since that's more complex, but I think this may help a bit (perhaps not too much though?) with the slow rebuild times. Source-Repo: https://github.com/servo/servo Source-Revision: ba9bec64126b5284467c0e13e7c85d9ea4b2c084 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : d7597d59fb81d6ce1e404233bfd963b9f355589c --- servo/components/style/stylist.rs | 334 +++++++++++++++--------------- servo/tests/unit/style/stylist.rs | 4 +- 2 files changed, 167 insertions(+), 171 deletions(-) diff --git a/servo/components/style/stylist.rs b/servo/components/style/stylist.rs index e70a0e2fc01b..db444a538116 100644 --- a/servo/components/style/stylist.rs +++ b/servo/components/style/stylist.rs @@ -502,19 +502,22 @@ impl Stylist { self.quirks_mode); self.invalidation_map.note_selector(selector, self.quirks_mode); - if needs_revalidation(&selector) { - self.selectors_for_cache_revalidation.insert( - RevalidationSelectorAndHashes::new(&selector, &hashes), - self.quirks_mode); - } - selector.visit(&mut AttributeAndStateDependencyVisitor { + let mut visitor = StylistSelectorVisitor { + needs_revalidation: false, + passed_rightmost_selector: false, attribute_dependencies: &mut self.attribute_dependencies, style_attribute_dependency: &mut self.style_attribute_dependency, state_dependencies: &mut self.state_dependencies, - }); - selector.visit(&mut MappedIdVisitor { mapped_ids: &mut self.mapped_ids, - }); + }; + + selector.visit(&mut visitor); + + if visitor.needs_revalidation { + self.selectors_for_cache_revalidation.insert( + RevalidationSelectorAndHashes::new(selector.clone(), hashes), + self.quirks_mode); + } } self.rules_source_order += 1; } @@ -1421,20 +1424,128 @@ impl Stylist { } } -/// Visitor to collect names that appear in attribute selectors and any -/// dependencies on ElementState bits. -struct AttributeAndStateDependencyVisitor<'a> { +/// SelectorMapEntry implementation for use in our revalidation selector map. +#[derive(Clone, Debug)] +struct RevalidationSelectorAndHashes { + selector: Selector, + selector_offset: usize, + hashes: AncestorHashes, +} + +impl RevalidationSelectorAndHashes { + fn new(selector: Selector, hashes: AncestorHashes) -> Self { + let selector_offset = { + // We basically want to check whether the first combinator is a + // pseudo-element combinator. If it is, we want to use the offset + // one past it. Otherwise, our offset is 0. + let mut index = 0; + let mut iter = selector.iter(); + + // First skip over the first ComplexSelector. + // + // We can't check what sort of what combinator we have until we do + // that. + for _ in &mut iter { + index += 1; // Simple selector + } + + match iter.next_sequence() { + Some(Combinator::PseudoElement) => index + 1, // +1 for the combinator + _ => 0 + } + }; + + RevalidationSelectorAndHashes { selector, selector_offset, hashes, } + } +} + +impl SelectorMapEntry for RevalidationSelectorAndHashes { + fn selector(&self) -> SelectorIter { + self.selector.iter_from(self.selector_offset) + } +} + +/// A selector visitor implementation that collects all the state the Stylist +/// cares about a selector. +struct StylistSelectorVisitor<'a> { + /// Whether the selector needs revalidation for the style sharing cache. + needs_revalidation: bool, + /// Whether we've past the rightmost compound selector, not counting + /// pseudo-elements. + passed_rightmost_selector: bool, + /// The filter with all the id's getting referenced from rightmost + /// selectors. + mapped_ids: &'a mut BloomFilter, + /// The filter with the local names of attributes there are selectors for. attribute_dependencies: &'a mut BloomFilter, + /// Whether there's any attribute selector for the [style] attribute. style_attribute_dependency: &'a mut bool, + /// All the states selectors in the page reference. state_dependencies: &'a mut ElementState, } -impl<'a> SelectorVisitor for AttributeAndStateDependencyVisitor<'a> { +fn component_needs_revalidation(c: &Component) -> bool { + match *c { + Component::AttributeInNoNamespaceExists { .. } | + Component::AttributeInNoNamespace { .. } | + Component::AttributeOther(_) | + Component::Empty | + // FIXME(bz) We really only want to do this for some cases of id + // selectors. See + // https://bugzilla.mozilla.org/show_bug.cgi?id=1369611 + Component::ID(_) | + Component::FirstChild | + Component::LastChild | + Component::OnlyChild | + Component::NthChild(..) | + Component::NthLastChild(..) | + Component::NthOfType(..) | + Component::NthLastOfType(..) | + Component::FirstOfType | + Component::LastOfType | + Component::OnlyOfType => { + true + }, + Component::NonTSPseudoClass(ref p) => { + p.needs_cache_revalidation() + }, + _ => { + false + } + } +} + +impl<'a> SelectorVisitor for StylistSelectorVisitor<'a> { type Impl = SelectorImpl; - fn visit_attribute_selector(&mut self, _ns: &NamespaceConstraint<&Namespace>, - name: &LocalName, lower_name: &LocalName) - -> bool { + fn visit_complex_selector( + &mut self, + _: SelectorIter, + combinator: Option + ) -> bool { + self.needs_revalidation = + self.needs_revalidation || combinator.map_or(false, |c| c.is_sibling()); + + // NOTE(emilio): This works properly right now because we can't store + // complex selectors in nested selectors, otherwise we may need to + // rethink this. + // + // Also, note that this call happens before we visit any of the simple + // selectors in the next ComplexSelector, so we can use this to skip + // looking at them. + self.passed_rightmost_selector = + self.passed_rightmost_selector || + !matches!(combinator, None | Some(Combinator::PseudoElement)); + + true + } + + fn visit_attribute_selector( + &mut self, + _ns: &NamespaceConstraint<&Namespace>, + name: &LocalName, + lower_name: &LocalName + ) -> bool { if *lower_name == local_name!("style") { *self.style_attribute_dependency = true; } else { @@ -1445,165 +1556,32 @@ impl<'a> SelectorVisitor for AttributeAndStateDependencyVisitor<'a> { } fn visit_simple_selector(&mut self, s: &Component) -> bool { - if let Component::NonTSPseudoClass(ref p) = *s { - self.state_dependencies.insert(p.state_flag()); - } - true - } -} + self.needs_revalidation = + self.needs_revalidation || component_needs_revalidation(s); -/// Visitor to collect ids that appear in the rightmost portion of selectors. -struct MappedIdVisitor<'a> { - mapped_ids: &'a mut BloomFilter, -} - -impl<'a> SelectorVisitor for MappedIdVisitor<'a> { - type Impl = SelectorImpl; - - /// We just want to insert all the ids we find into mapped_ids. - fn visit_simple_selector(&mut self, s: &Component) -> bool { - if let Component::ID(ref id) = *s { - self.mapped_ids.insert_hash(id.get_hash()); - } - true - } - - /// We want to stop as soon as we've moved off the rightmost ComplexSelector - /// that is not a psedo-element. That can be detected by a - /// visit_complex_selector call with a combinator other than None and - /// PseudoElement. Importantly, this call happens before we visit any of - /// the simple selectors in that ComplexSelector. - fn visit_complex_selector(&mut self, - _: SelectorIter, - combinator: Option) -> bool { - match combinator { - None | Some(Combinator::PseudoElement) => true, - _ => false, - } - } -} - -/// SelectorMapEntry implementation for use in our revalidation selector map. -#[derive(Clone, Debug)] -struct RevalidationSelectorAndHashes { - selector: Selector, - selector_offset: usize, - hashes: AncestorHashes, -} - -impl RevalidationSelectorAndHashes { - fn new(selector: &Selector, hashes: &AncestorHashes) -> Self { - // We basically want to check whether the first combinator is a - // pseudo-element combinator. If it is, we want to use the offset one - // past it. Otherwise, our offset is 0. - let mut index = 0; - let mut iter = selector.iter(); - - // First skip over the first ComplexSelector. We can't check what sort - // of combinator we have until we do that. - for _ in &mut iter { - index += 1; // Simple selector - } - - let offset = match iter.next_sequence() { - Some(Combinator::PseudoElement) => index + 1, // +1 for the combinator - _ => 0 - }; - - RevalidationSelectorAndHashes { - selector: selector.clone(), - selector_offset: offset, - hashes: hashes.clone(), - } - } -} - -impl SelectorMapEntry for RevalidationSelectorAndHashes { - fn selector(&self) -> SelectorIter { - self.selector.iter_from(self.selector_offset) - } -} - -/// Visitor to 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. -/// -/// Also, note that it's important to check the whole selector, due to cousins -/// sharing arbitrarily deep in the DOM, not just the rightmost part of it -/// (unfortunately, though). -/// -/// With cousin sharing, we not only need to care about selectors in stuff like -/// foo:first-child, but also about selectors like p:first-child foo, since the -/// two parents may have shared style, and in that case we can test cousins -/// whose matching depends on the selector up in the chain. -/// -/// TODO(emilio): We can optimize when matching only siblings to only match the -/// rightmost selector until a descendant combinator is found, I guess, and in -/// general when we're sharing at depth `n`, to the `n + 1` sequences of -/// descendant combinators. -/// -/// I don't think that in presence of the bloom filter it's worth it, though. -struct RevalidationVisitor; - -impl SelectorVisitor for RevalidationVisitor { - type Impl = SelectorImpl; - - - fn visit_complex_selector(&mut self, - _: SelectorIter, - combinator: Option) -> bool { - let is_sibling_combinator = - combinator.map_or(false, |c| c.is_sibling()); - - !is_sibling_combinator - } - - - /// Check whether 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::AttributeInNoNamespaceExists { .. } | - Component::AttributeInNoNamespace { .. } | - Component::AttributeOther(_) | - Component::Empty | - // FIXME(bz) We really only want to do this for some cases of id - // selectors. See - // https://bugzilla.mozilla.org/show_bug.cgi?id=1369611 - Component::ID(_) | - 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) => { - !p.needs_cache_revalidation() - }, - _ => { - true + self.state_dependencies.insert(p.state_flag()); } + Component::ID(ref id) if !self.passed_rightmost_selector => { + // We want to stop storing mapped ids as soon as we've moved off + // the rightmost ComplexSelector that is not a pseudo-element. + // + // That can be detected by a visit_complex_selector call with a + // combinator other than None and PseudoElement. + // + // Importantly, this call happens before we visit any of the + // simple selectors in that ComplexSelector. + // + // NOTE(emilio): See the comment regarding on when this may + // break in visit_complex_selector. + self.mapped_ids.insert_hash(id.get_hash()); + } + _ => {}, } - } -} -/// Returns true if the given selector needs cache revalidation. -pub fn needs_revalidation(selector: &Selector) -> bool { - let mut visitor = RevalidationVisitor; - !selector.visit(&mut visitor) + true + } } /// Map that contains the CSS rules for a specific PseudoElement @@ -1701,3 +1679,21 @@ impl Rule { } } } + +/// A function to be able to test the revalidation stuff. +pub fn needs_revalidation_for_testing(s: &Selector) -> bool { + let mut attribute_dependencies = BloomFilter::new(); + let mut mapped_ids = BloomFilter::new(); + let mut style_attribute_dependency = false; + let mut state_dependencies = ElementState::empty(); + let mut visitor = StylistSelectorVisitor { + needs_revalidation: false, + passed_rightmost_selector: false, + attribute_dependencies: &mut attribute_dependencies, + style_attribute_dependency: &mut style_attribute_dependency, + state_dependencies: &mut state_dependencies, + mapped_ids: &mut mapped_ids, + }; + s.visit(&mut visitor); + visitor.needs_revalidation +} diff --git a/servo/tests/unit/style/stylist.rs b/servo/tests/unit/style/stylist.rs index da9a9478957a..53269b405227 100644 --- a/servo/tests/unit/style/stylist.rs +++ b/servo/tests/unit/style/stylist.rs @@ -20,7 +20,7 @@ use style::selector_parser::{SelectorImpl, SelectorParser}; use style::shared_lock::SharedRwLock; use style::stylesheets::StyleRule; use style::stylist::{Stylist, Rule}; -use style::stylist::needs_revalidation; +use style::stylist::needs_revalidation_for_testing; use style::thread_state; /// Helper method to get some Rules from selector strings. @@ -126,7 +126,7 @@ fn test_revalidation_selectors() { // Selectors in the ancestor chain (needed for cousin sharing). "p:first-child span", ]).into_iter() - .filter(|s| needs_revalidation(&s)) + .filter(|s| needs_revalidation_for_testing(&s)) .collect::>(); let reference = parse_selectors(&[