From ac40cc629bb8905d8d4fe8d3d9aa802a8ebb6cd2 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 3 May 2017 16:38:42 -0700 Subject: [PATCH] Use a rulehash for DependencySet. MozReview-Commit-ID: GXu6O4kiBE6 --- components/style/restyle_hints.rs | 137 ++++++++++++------------------ tests/unit/style/restyle_hints.rs | 7 +- 2 files changed, 58 insertions(+), 86 deletions(-) diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 0e7dcd19adc..1ac3774a66f 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -20,7 +20,10 @@ use selectors::matching::matches_selector; use selectors::parser::{AttrSelector, Combinator, Component, Selector}; use selectors::parser::{SelectorInner, SelectorMethods}; use selectors::visitor::SelectorVisitor; +use smallvec::SmallVec; +use std::borrow::Borrow; use std::clone::Clone; +use stylist::SelectorMap; bitflags! { /// When the ElementState of an element (like IN_HOVER_STATE) changes, @@ -429,7 +432,7 @@ fn combinator_to_restyle_hint(combinator: Option) -> RestyleHint { } } -#[derive(Debug)] +#[derive(Clone, Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] /// The aspects of an selector which are sensitive. pub struct Sensitivities { @@ -450,6 +453,10 @@ impl Sensitivities { attrs: false, } } + + fn sensitive_to(&self, attrs: bool, states: ElementState) -> bool { + (attrs && self.attrs) || self.states.intersects(states) + } } /// Mapping between (partial) CompoundSelectors (and the combinator to their @@ -470,7 +477,7 @@ impl Sensitivities { /// This allows us to quickly scan through the dependency sites of all style /// rules and determine the maximum effect that a given state or attribute /// change may have on the style of elements in the document. -#[derive(Debug)] +#[derive(Clone, Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct Dependency { #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] @@ -481,6 +488,11 @@ pub struct Dependency { pub sensitivities: Sensitivities, } +impl Borrow> for Dependency { + fn borrow(&self) -> &SelectorInner { + &self.selector + } +} /// The following visitor visits all the simple selectors for a given complex /// selector, taking care of :not and :any combinators, collecting whether any @@ -500,32 +512,17 @@ impl SelectorVisitor for SensitivitiesVisitor { /// A set of dependencies for a given stylist. /// -/// Note that there are measurable perf wins from storing them separately -/// depending on what kind of change they affect, and its also not a big deal to -/// do it, since the dependencies are per-document. +/// Note that we can have many dependencies, often more than the total number +/// of selectors given that we can get multiple partial selectors for a given +/// selector. As such, we want all the usual optimizations, including the +/// SelectorMap and the bloom filter. #[derive(Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] -pub struct DependencySet { - /// Dependencies only affected by state. - state_deps: Vec, - /// Dependencies only affected by attributes. - attr_deps: Vec, - /// Dependencies affected by both. - common_deps: Vec, -} +pub struct DependencySet(pub SelectorMap); impl DependencySet { fn add_dependency(&mut self, dep: Dependency) { - let affected_by_attribute = dep.sensitivities.attrs; - let affects_states = !dep.sensitivities.states.is_empty(); - - if affected_by_attribute && affects_states { - self.common_deps.push(dep) - } else if affected_by_attribute { - self.attr_deps.push(dep) - } else { - self.state_deps.push(dep) - } + self.0.insert(dep); } /// Adds a selector to this `DependencySet`. @@ -592,23 +589,17 @@ impl DependencySet { /// Create an empty `DependencySet`. pub fn new() -> Self { - DependencySet { - state_deps: vec![], - attr_deps: vec![], - common_deps: vec![], - } + DependencySet(SelectorMap::new()) } /// Return the total number of dependencies that this set contains. pub fn len(&self) -> usize { - self.common_deps.len() + self.attr_deps.len() + self.state_deps.len() + self.0.len() } /// Clear this dependency set. pub fn clear(&mut self) { - self.common_deps.clear(); - self.attr_deps.clear(); - self.state_deps.clear(); + self.0 = SelectorMap::new(); } /// Compute a restyle hint given an element and a snapshot, per the rules @@ -631,18 +622,41 @@ impl DependencySet { let mut hint = RestyleHint::empty(); let snapshot_el = ElementWrapper::new_with_snapshot(el.clone(), snapshot); - Self::compute_partial_hint(&self.common_deps, el, &snapshot_el, - &state_changes, attrs_changed, &mut hint); + // Compute whether the snapshot has any different id or class attributes + // from the element. If it does, we need to pass those to the lookup, so + // that we get all the possible applicable selectors from the rulehash. + let mut additional_id = None; + let mut additional_classes = SmallVec::<[Atom; 8]>::new(); + if snapshot.has_attrs() { + let id = snapshot.id_attr(); + if id.is_some() && id != el.get_id() { + additional_id = id; + } - if !state_changes.is_empty() { - Self::compute_partial_hint(&self.state_deps, el, &snapshot_el, - &state_changes, attrs_changed, &mut hint); + snapshot.each_class(|c| if !el.has_class(c) { additional_classes.push(c.clone()) }); } - if attrs_changed { - Self::compute_partial_hint(&self.attr_deps, el, &snapshot_el, - &state_changes, attrs_changed, &mut hint); - } + self.0.lookup_with_additional(*el, additional_id, &additional_classes, &mut |dep| { + if !dep.sensitivities.sensitive_to(attrs_changed, state_changes) || hint.contains(dep.hint) { + return true; + } + + // We can ignore the selector flags, since they would have already been set during + // original matching for any element that might change its matching behavior here. + let matched_then = + matches_selector(&dep.selector, &snapshot_el, None, + &mut StyleRelations::empty(), + &mut |_, _| {}); + let matches_now = + matches_selector(&dep.selector, el, None, + &mut StyleRelations::empty(), + &mut |_, _| {}); + if matched_then != matches_now { + hint.insert(dep.hint); + } + + !hint.is_all() + }); debug!("Calculated restyle hint: {:?}. (Element={:?}, State={:?}, Snapshot={:?}, {} Deps)", hint, el, current_state, snapshot, self.len()); @@ -650,45 +664,4 @@ impl DependencySet { hint } - - fn compute_partial_hint(deps: &[Dependency], - element: &E, - snapshot: &ElementWrapper, - state_changes: &ElementState, - attrs_changed: bool, - hint: &mut RestyleHint) - where E: TElement, - { - if hint.is_all() { - return; - } - for dep in deps { - debug_assert!((!state_changes.is_empty() && !dep.sensitivities.states.is_empty()) || - (attrs_changed && dep.sensitivities.attrs), - "Testing a known ineffective dependency?"); - if (attrs_changed || state_changes.intersects(dep.sensitivities.states)) && !hint.contains(dep.hint) { - // We can ignore the selector flags, since they would have already been set during - // original matching for any element that might change its matching behavior here. - let matched_then = - matches_selector(&dep.selector, snapshot, None, - &mut StyleRelations::empty(), - &mut |_, _| {}); - let matches_now = - matches_selector(&dep.selector, element, None, - &mut StyleRelations::empty(), - &mut |_, _| {}); - if matched_then != matches_now { - hint.insert(dep.hint); - } - if hint.is_all() { - break; - } - } - } - } - - /// Get the dependencies affected by state. - pub fn get_state_deps(&self) -> &[Dependency] { - &self.state_deps - } } diff --git a/tests/unit/style/restyle_hints.rs b/tests/unit/style/restyle_hints.rs index 9a050e05fb5..96f2d21877c 100644 --- a/tests/unit/style/restyle_hints.rs +++ b/tests/unit/style/restyle_hints.rs @@ -24,8 +24,7 @@ fn smoke_restyle_hints() { let selector = (selectors.0).first().unwrap(); dependencies.note_selector(selector); assert_eq!(dependencies.len(), 1); - let state_deps = dependencies.get_state_deps(); - assert_eq!(state_deps.len(), 1); - assert!(!state_deps[0].sensitivities.states.is_empty()); - assert!(state_deps[0].hint.contains(RESTYLE_LATER_SIBLINGS)); + let dep = &dependencies.0.other[0]; + assert!(!dep.sensitivities.states.is_empty()); + assert!(dep.hint.contains(RESTYLE_LATER_SIBLINGS)); }