style: Avoid selector-matching when only the style attribute is changed.

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<PseudoElement, ComputedStyle,
                                  BuildHasherDefault<::fnv::FnvHasher>>;
 
+/// 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<PseudoElement, StrongRuleNode,
+                                   BuildHasherDefault<::fnv::FnvHasher>>;
+
 /// 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<RestyleHint> 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()

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<E: TElement>(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<PseudoElement, StrongRuleNode,
-                               BuildHasherDefault<::fnv::FnvHasher>>;
-
 /// 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<Self>,
+                              data: &mut AtomicRefMut<ElementData>)
+                              -> 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

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<nsRestyleHint> for RestyleHint {
     fn from(raw: nsRestyleHint) -> Self {

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<Item=(StyleSource, CascadeLevel)>,
     {
-        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<Item=(StyleSource, CascadeLevel)>,
+    {
+        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<RwLock<PropertyDeclarationBlock>>>,
+                                               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<StyleSource>,
 
     /// The cascade level this rule is positioned at.
-    cascade_level: CascadeLevel,
+    level: CascadeLevel,
 
     refcount: AtomicUsize,
     first_child: AtomicPtr<RuleNode>,
@@ -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.

components/style/traversal.rs

@@ -466,7 +466,11 @@ fn compute_style<E, D>(_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<E, D>(_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 {