From 8859aa004fbd8021edfaac11420aefe76a52eb7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 31 Jan 2017 18:50:32 +0100 Subject: [PATCH] style: Avoid selector-matching when only the style attribute is changed. --- components/style/data.rs | 72 ++++++++++++---- components/style/matching.rs | 48 ++++++++--- components/style/restyle_hints.rs | 8 ++ components/style/rule_tree/mod.rs | 133 +++++++++++++++++++++++++++--- components/style/traversal.rs | 15 +++- 5 files changed, 235 insertions(+), 41 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index 0c097b103a14..43c2a045d795 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -9,7 +9,7 @@ use dom::TElement; use properties::ComputedValues; use properties::longhands::display::computed_value as display; -use restyle_hints::{RESTYLE_LATER_SIBLINGS, RestyleHint}; +use restyle_hints::{RESTYLE_LATER_SIBLINGS, RESTYLE_SELF, RESTYLE_STYLE_ATTRIBUTE, RestyleHint}; use rule_tree::StrongRuleNode; use selector_parser::{PseudoElement, RestyleDamage, Snapshot}; use std::collections::HashMap; @@ -44,8 +44,8 @@ impl ComputedStyle { } } -// We manually implement Debug for ComputedStyle so tht we can avoid the verbose -// stringification of ComputedValues for normal logging. +// We manually implement Debug for ComputedStyle so that we can avoid the +// verbose stringification of ComputedValues for normal logging. impl fmt::Debug for ComputedStyle { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "ComputedStyle {{ rules: {:?}, values: {{..}} }}", self.rules) @@ -55,6 +55,13 @@ impl fmt::Debug for ComputedStyle { type PseudoStylesInner = HashMap>; +/// The rule nodes for each of the pseudo-elements of an element. +/// +/// TODO(emilio): Probably shouldn't be a `HashMap` by default, but a smaller +/// array. +pub type PseudoRuleNodes = HashMap>; + /// A set of styles for a given element's pseudo-elements. /// /// This is a map from pseudo-element to `ComputedStyle`. @@ -69,6 +76,19 @@ impl PseudoStyles { pub fn empty() -> Self { PseudoStyles(HashMap::with_hasher(Default::default())) } + + /// Gets the rules that the different pseudo-elements matched. + /// + /// FIXME(emilio): We could in theory avoid creating these when we have + /// support for just re-cascading an element. Then the input to + /// `cascade_node` could be `MatchResults` or just `UseExistingStyle`. + pub fn get_rules(&self) -> PseudoRuleNodes { + let mut rules = HashMap::with_hasher(Default::default()); + for (pseudo, style) in &self.0 { + rules.insert(pseudo.clone(), style.rules.clone()); + } + rules + } } impl Deref for PseudoStyles { @@ -144,8 +164,11 @@ impl DescendantRestyleHint { /// to provide more type safety while propagating restyle hints down the tree. #[derive(Clone, Debug)] pub struct StoredRestyleHint { - /// Whether this element should be restyled during the traversal. - pub restyle_self: bool, + /// Whether this element should be restyled during the traversal, and how. + /// + /// This hint is stripped down, and only contains hints that are a subset of + /// RestyleHint::for_single_element(). + pub for_self: RestyleHint, /// Whether the descendants of this element need to be restyled. pub descendants: DescendantRestyleHint, } @@ -154,7 +177,11 @@ impl StoredRestyleHint { /// Propagates this restyle hint to a child element. pub fn propagate(&self) -> Self { StoredRestyleHint { - restyle_self: self.descendants != DescendantRestyleHint::Empty, + for_self: if self.descendants == DescendantRestyleHint::Empty { + RestyleHint::empty() + } else { + RESTYLE_SELF + }, descendants: self.descendants.propagate(), } } @@ -162,7 +189,7 @@ impl StoredRestyleHint { /// Creates an empty `StoredRestyleHint`. pub fn empty() -> Self { StoredRestyleHint { - restyle_self: false, + for_self: RestyleHint::empty(), descendants: DescendantRestyleHint::Empty, } } @@ -171,29 +198,31 @@ impl StoredRestyleHint { /// including the element. pub fn subtree() -> Self { StoredRestyleHint { - restyle_self: true, + for_self: RESTYLE_SELF, descendants: DescendantRestyleHint::Descendants, } } /// Whether the restyle hint is empty (nothing requires to be restyled). pub fn is_empty(&self) -> bool { - !self.restyle_self && self.descendants == DescendantRestyleHint::Empty + !self.needs_restyle_self() && self.descendants == DescendantRestyleHint::Empty + } + + /// Whether the element holding the hint needs to be restyled on some way. + pub fn needs_restyle_self(&self) -> bool { + !self.for_self.is_empty() } /// Insert another restyle hint, effectively resulting in the union of both. pub fn insert(&mut self, other: &Self) { - self.restyle_self = self.restyle_self || other.restyle_self; + self.for_self |= other.for_self; self.descendants = self.descendants.union(other.descendants); } } impl Default for StoredRestyleHint { fn default() -> Self { - StoredRestyleHint { - restyle_self: false, - descendants: DescendantRestyleHint::Empty, - } + StoredRestyleHint::empty() } } @@ -203,7 +232,7 @@ impl From for StoredRestyleHint { use self::DescendantRestyleHint::*; debug_assert!(!hint.contains(RESTYLE_LATER_SIBLINGS), "Caller should apply sibling hints"); StoredRestyleHint { - restyle_self: hint.contains(RESTYLE_SELF) || hint.contains(RESTYLE_STYLE_ATTRIBUTE), + for_self: hint & RestyleHint::for_single_element(), descendants: if hint.contains(RESTYLE_DESCENDANTS) { Descendants } else { Empty }, } } @@ -319,7 +348,9 @@ impl RestyleData { /// Returns true if this RestyleData might invalidate the current style. pub fn has_invalidations(&self) -> bool { - self.hint.restyle_self || self.recascade || self.snapshot.is_some() + self.hint.needs_restyle_self() || + self.recascade || + self.snapshot.is_some() } } @@ -359,6 +390,15 @@ impl ElementData { self.restyle.as_ref().map_or(true, |r| !r.has_invalidations()) } + /// Returns true if this element and its pseudo elements do not need + /// selector matching, and can just go on with a style attribute update in + /// the rule tree plus re-cascade. + pub fn needs_only_style_attribute_update(&self) -> bool { + debug_assert!(!self.has_current_styles(), "Should've stopped earlier"); + self.has_styles() && + self.restyle.as_ref().map_or(false, |r| r.hint.for_self == RESTYLE_STYLE_ATTRIBUTE) + } + /// Gets the element styles, if any. pub fn get_styles(&self) -> Option<&ElementStyles> { self.styles.as_ref() diff --git a/components/style/matching.rs b/components/style/matching.rs index 51f05b8bdec9..41df10f190ef 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -13,19 +13,18 @@ use atomic_refcell::AtomicRefMut; use cache::LRUCache; use cascade_info::CascadeInfo; use context::{SharedStyleContext, StyleContext}; -use data::{ComputedStyle, ElementData, ElementStyles, PseudoStyles}; +use data::{ComputedStyle, ElementData, ElementStyles, PseudoRuleNodes, PseudoStyles}; use dom::{SendElement, TElement, TNode}; use properties::{CascadeFlags, ComputedValues, SHAREABLE, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, cascade}; use properties::longhands::display::computed_value as display; -use rule_tree::StrongRuleNode; +use rule_tree::{CascadeLevel, StrongRuleNode}; use selector_parser::{PseudoElement, RestyleDamage, SelectorImpl}; use selectors::MatchAttr; use selectors::bloom::BloomFilter; -use selectors::matching::{AFFECTED_BY_PSEUDO_ELEMENTS, MatchingReason, StyleRelations}; +use selectors::matching::{AFFECTED_BY_PSEUDO_ELEMENTS, AFFECTED_BY_STYLE_ATTRIBUTE, MatchingReason, StyleRelations}; use servo_config::opts; use sink::ForgetfulSink; use std::collections::HashMap; -use std::hash::BuildHasherDefault; use std::slice::IterMut; use std::sync::Arc; use stylist::ApplicableDeclarationBlock; @@ -60,13 +59,6 @@ fn create_common_style_affecting_attributes_from_element(element: & flags } -/// The rule nodes for each of the pseudo-elements of an element. -/// -/// TODO(emilio): Probably shouldn't be a `HashMap` by default, but a smaller -/// array. -type PseudoRuleNodes = HashMap>; - /// The results of selector matching on an element. pub struct MatchResults { /// The rule node reference that represents the rules matched by the @@ -635,6 +627,40 @@ pub trait MatchMethods : TElement { } } + /// Updates the style attribute rule nodes without re-running selector + /// matching, using just the rule tree. + fn update_style_attribute(&self, + context: &StyleContext, + data: &mut AtomicRefMut) + -> MatchResults { + let style_attribute = self.style_attribute(); + + let mut new_rule_node = data.styles().primary.rules.clone(); + + new_rule_node = context.shared.stylist.rule_tree + .replace_rule_at_level_if_applicable(CascadeLevel::StyleAttributeNormal, + style_attribute, + new_rule_node); + + new_rule_node = context.shared.stylist.rule_tree + .replace_rule_at_level_if_applicable(CascadeLevel::StyleAttributeImportant, + style_attribute, + new_rule_node); + + MatchResults { + primary: new_rule_node, + // FIXME(emilio): This is ok, for now at least, because it disables + // style sharing. That being said, it's not ideal, probably should + // be optional? + relations: AFFECTED_BY_STYLE_ATTRIBUTE, + // The per-pseudo rule-nodes haven't changed, but still need to be + // recomputed. + // + // TODO(emilio): We could probably optimize this quite a bit. + per_pseudo: data.styles().pseudos.get_rules(), + } + } + /// Attempts to share a style with another node. This method is unsafe /// because it depends on the `style_sharing_candidate_cache` having only /// live nodes in it, and we have no way to guarantee that at the type diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 8528ef116b6f..d1458b41646f 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -50,6 +50,14 @@ bitflags! { } } +impl RestyleHint { + /// The subset hints that affect the styling of a single element during the + /// traversal. + pub fn for_single_element() -> Self { + RESTYLE_SELF | RESTYLE_STYLE_ATTRIBUTE + } +} + #[cfg(feature = "gecko")] impl From for RestyleHint { fn from(raw: nsRestyleHint) -> Self { diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index ee5ac080b497..7e044dab1185 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -153,9 +153,20 @@ impl RuleTree { pub fn insert_ordered_rules<'a, I>(&self, iter: I) -> StrongRuleNode where I: Iterator, { - let mut current = self.root.clone(); + self.insert_ordered_rules_from(self.root.clone(), iter) + } + + fn insert_ordered_rules_from<'a, I>(&self, + from: StrongRuleNode, + iter: I) -> StrongRuleNode + where I: Iterator, + { + let mut current = from; + let mut last_level = current.get().level; for (source, level) in iter { + debug_assert!(last_level <= level, "Not really ordered"); current = current.ensure_child(self.root.downgrade(), source, level); + last_level = level; } current } @@ -169,6 +180,92 @@ impl RuleTree { pub unsafe fn maybe_gc(&self) { self.root.maybe_gc(); } + + /// Replaces a rule in a given level (if present) for another rule. + /// + /// Returns the resulting node that represents the new path. + /// + /// TODO(emilio): Maybe this should be in `StrongRuleNode`? + pub fn replace_rule_at_level_if_applicable(&self, + level: CascadeLevel, + pdb: Option<&Arc>>, + path: StrongRuleNode) + -> StrongRuleNode { + debug_assert!(level.is_unique_per_element()); + // TODO(emilio): Being smarter with lifetimes we could avoid a bit of + // the refcount churn. + let mut current = path.clone(); + + // First walk up until the first less-or-equally specific rule. + let mut children = vec![]; + while current.get().level > level { + children.push((current.get().source.clone().unwrap(), current.get().level)); + current = current.parent().unwrap().clone(); + } + + // Then remove the one at the level we want to replace, if any. + // + // NOTE: Here we assume that only one rule can be at the level we're + // replacing. + // + // This is certainly true for HTML style attribute rules, animations and + // transitions, but could not be so for SMIL animations, which we'd need + // to special-case (isn't hard, it's just about removing the `if` and + // special cases, and replacing them for a `while` loop, avoiding the + // optimizations). + if current.get().level == level { + if let Some(pdb) = pdb { + // If the only rule at the level we're replacing is exactly the + // same as `pdb`, we're done, and `path` is still valid. + // + // TODO(emilio): Another potential optimization is the one where + // we can just replace the rule at that level for `pdb`, and + // then we don't need to re-create the children, and `path` is + // also equally valid. This is less likely, and would require an + // in-place mutation of the source, which is, at best, fiddly, + // so let's skip it for now. + let is_here_already = match current.get().source.as_ref() { + Some(&StyleSource::Declarations(ref already_here)) => { + arc_ptr_eq(pdb, already_here) + }, + _ => unreachable!("Replacing non-declarations style?"), + }; + + if is_here_already { + debug!("Picking the fast path in rule replacement"); + return path; + } + } + current = current.parent().unwrap().clone(); + } + debug_assert!(current.get().level != level, + "Multiple rules should've been replaced?"); + + // Insert the rule if it's relevant at this level in the cascade. + // + // These optimizations are likely to be important, because the levels + // where replacements apply (style and animations) tend to trigger + // pretty bad styling cases already. + if let Some(pdb) = pdb { + if level.is_important() { + if pdb.read().any_important() { + current = current.ensure_child(self.root.downgrade(), + StyleSource::Declarations(pdb.clone()), + level); + } + } else { + if pdb.read().any_normal() { + current = current.ensure_child(self.root.downgrade(), + StyleSource::Declarations(pdb.clone()), + level); + } + } + } + + // Now the rule is in the relevant place, push the children as + // necessary. + self.insert_ordered_rules_from(current, children.into_iter().rev()) + } } /// The number of RuleNodes added to the free list before we will consider @@ -183,7 +280,7 @@ const RULE_TREE_GC_INTERVAL: usize = 300; /// /// [1]: https://drafts.csswg.org/css-cascade/#cascade-origin #[repr(u8)] -#[derive(Eq, PartialEq, Copy, Clone, Debug)] +#[derive(Eq, PartialEq, Copy, Clone, Debug, PartialOrd)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub enum CascadeLevel { /// Normal User-Agent rules. @@ -211,6 +308,18 @@ pub enum CascadeLevel { } impl CascadeLevel { + /// Returns whether this cascade level is unique per element, in which case + /// we can replace the path in the cascade without fear. + pub fn is_unique_per_element(&self) -> bool { + match *self { + CascadeLevel::Transitions | + CascadeLevel::Animations | + CascadeLevel::StyleAttributeNormal | + CascadeLevel::StyleAttributeImportant => true, + _ => false, + } + } + /// Returns whether this cascade level represents important rules of some /// sort. #[inline] @@ -248,7 +357,7 @@ struct RuleNode { source: Option, /// The cascade level this rule is positioned at. - cascade_level: CascadeLevel, + level: CascadeLevel, refcount: AtomicUsize, first_child: AtomicPtr, @@ -274,13 +383,13 @@ impl RuleNode { fn new(root: WeakRuleNode, parent: StrongRuleNode, source: StyleSource, - cascade_level: CascadeLevel) -> Self { + level: CascadeLevel) -> Self { debug_assert!(root.upgrade().parent().is_none()); RuleNode { root: Some(root), parent: Some(parent), source: Some(source), - cascade_level: cascade_level, + level: level, refcount: AtomicUsize::new(1), first_child: AtomicPtr::new(ptr::null_mut()), next_sibling: AtomicPtr::new(ptr::null_mut()), @@ -295,7 +404,7 @@ impl RuleNode { root: None, parent: None, source: None, - cascade_level: CascadeLevel::UANormal, + level: CascadeLevel::UANormal, refcount: AtomicUsize::new(1), first_child: AtomicPtr::new(ptr::null_mut()), next_sibling: AtomicPtr::new(ptr::null_mut()), @@ -444,10 +553,10 @@ impl StrongRuleNode { fn ensure_child(&self, root: WeakRuleNode, source: StyleSource, - cascade_level: CascadeLevel) -> StrongRuleNode { + level: CascadeLevel) -> StrongRuleNode { let mut last = None; for child in self.get().iter_children() { - if child .get().cascade_level == cascade_level && + if child .get().level == level && child.get().source.as_ref().unwrap().ptr_equals(&source) { return child; } @@ -455,9 +564,9 @@ impl StrongRuleNode { } let mut node = Box::new(RuleNode::new(root, - self.clone(), - source.clone(), - cascade_level)); + self.clone(), + source.clone(), + level)); let new_ptr: *mut RuleNode = &mut *node; loop { @@ -521,7 +630,7 @@ impl StrongRuleNode { /// Get the importance that this rule node represents. pub fn importance(&self) -> Importance { - self.get().cascade_level.importance() + self.get().level.importance() } /// Get an iterator for this rule node and its ancestors. diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 47d33dd72693..959caf812b8c 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -466,7 +466,11 @@ fn compute_style(_traversal: &D, { context.thread_local.statistics.elements_styled += 1; let shared_context = context.shared; + // Ensure the bloom filter is up to date. + // + // TODO(emilio): In theory we could avoid a bit of this work in the style + // attribute case. let dom_depth = context.thread_local.bloom_filter .insert_parents_recovering(element, traversal_data.current_dom_depth); @@ -493,8 +497,15 @@ fn compute_style(_traversal: &D, let shareable_element = { // Perform the CSS selector matching. context.thread_local.statistics.elements_matched += 1; - let filter = context.thread_local.bloom_filter.filter(); - match_results = element.match_element(context, Some(filter)); + + // TODO(emilio): Here we'll need to support animation-only hints + // and similar. + match_results = if data.needs_only_style_attribute_update() { + element.update_style_attribute(context, data) + } else { + let filter = context.thread_local.bloom_filter.filter(); + element.match_element(context, Some(filter)) + }; if match_results.primary_is_shareable() { Some(element) } else {