servo: Merge #19696 - Skip rule node which contains only inherited properties for rule cache (from upsuper:rule-cache-opt); r=emilio

This is one possible fix for [bug 1427681](https://bugzilla.mozilla.org/show_bug.cgi?id=1427681) which tries to skip some rule nodes when using rule cache.

Try push for correctness: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74e3941e2cfc5fba4bce839f2518af8a5a8b7411

It doesn't really show much memory saving on awsy. It only shows several KB save on fresh start memory. But since conceptually it's simple, I guess it's worth taking.

Source-Repo: https://github.com/servo/servo
Source-Revision: 6131371cc230cca45b13ef9a2622c6e602208357

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 1291fd4e9a1c7aa05125ffccac6d81227bc3c221

servo/components/style/animation.rs

@@ -485,6 +485,7 @@ fn compute_style_for_animation_step(context: &SharedStyleContext,
                 properties::apply_declarations(context.stylist.device(),
                                                /* pseudo = */ None,
                                                previous_style.rules(),
+                                               &context.guards,
                                                iter,
                                                Some(previous_style),
                                                Some(previous_style),

servo/components/style/properties/declaration_block.rs

@@ -283,6 +283,12 @@ impl PropertyDeclarationBlock {
         self.longhands.contains(id)
     }
 
+    /// Returns whether this block contains any reset longhand.
+    #[inline]
+    pub fn contains_any_reset(&self) -> bool {
+        self.longhands.contains_any_reset()
+    }
+
     /// Get a declaration for a given property.
     ///
     /// NOTE: This is linear time.

servo/components/style/properties/properties.mako.rs

@@ -309,6 +309,16 @@ impl LonghandIdSet {
         true
     }
 
+    /// Returns whether this set contains any longhand that `other` also contains.
+    pub fn contains_any(&self, other: &Self) -> bool {
+        for (self_cell, other_cell) in self.storage.iter().zip(other.storage.iter()) {
+            if (*self_cell & *other_cell) != 0 {
+                return true;
+            }
+        }
+        false
+    }
+
     /// Create an empty set
     #[inline]
     pub fn new() -> LonghandIdSet {
@@ -322,6 +332,13 @@ impl LonghandIdSet {
         (self.storage[bit / 32] & (1 << (bit % 32))) != 0
     }
 
+    /// Return whether this set contains any reset longhand.
+    #[inline]
+    pub fn contains_any_reset(&self) -> bool {
+        ${static_longhand_id_set("RESET", lambda p: not p.style_struct.inherited)}
+        self.contains_any(&RESET)
+    }
+
     /// Add the given property to the set
     #[inline]
     pub fn insert(&mut self, id: LonghandId) {
@@ -3157,6 +3174,7 @@ pub fn cascade(
         device,
         pseudo,
         rule_node,
+        guards,
         iter_declarations,
         parent_style,
         parent_style_ignoring_first_line,
@@ -3176,6 +3194,7 @@ pub fn apply_declarations<'a, F, I>(
     device: &Device,
     pseudo: Option<<&PseudoElement>,
     rules: &StrongRuleNode,
+    guards: &StylesheetGuards,
     iter_declarations: F,
     parent_style: Option<<&ComputedValues>,
     parent_style_ignoring_first_line: Option<<&ComputedValues>,
@@ -3478,7 +3497,7 @@ where
             % endif
             }
 
-            if let Some(style) = rule_cache.and_then(|c| c.find(&context.builder)) {
+            if let Some(style) = rule_cache.and_then(|c| c.find(guards, &context.builder)) {
                 context.builder.copy_reset_from(style);
                 apply_reset = false;
             }

servo/components/style/rule_cache.rs

@@ -8,9 +8,10 @@
 use fnv::FnvHashMap;
 use logical_geometry::WritingMode;
 use properties::{ComputedValues, StyleBuilder};
-use rule_tree::StrongRuleNode;
+use rule_tree::{StrongRuleNode, StyleSource};
 use selector_parser::PseudoElement;
 use servo_arc::Arc;
+use shared_lock::StylesheetGuards;
 use smallvec::SmallVec;
 use values::computed::NonNegativeLength;
 
@@ -81,12 +82,44 @@ impl RuleCache {
         }
     }
 
+    /// Walk the rule tree and return a rule node for using as the key
+    /// for rule cache.
+    ///
+    /// It currently skips a rule node when it is neither from a style
+    /// rule, nor containing any declaration of reset property. We don't
+    /// skip style rule so that we don't need to walk a long way in the
+    /// worst case. Skipping declarations rule nodes should be enough
+    /// to address common cases that rule cache would fail to share
+    /// when using the rule node directly, like preshint, style attrs,
+    /// and animations.
+    fn get_rule_node_for_cache<'r>(
+        guards: &StylesheetGuards,
+        mut rule_node: Option<&'r StrongRuleNode>
+    ) -> Option<&'r StrongRuleNode> {
+        while let Some(node) = rule_node {
+            match *node.style_source() {
+                StyleSource::Declarations(ref decls) => {
+                    let cascade_level = node.cascade_level();
+                    let decls = decls.read_with(cascade_level.guard(guards));
+                    if decls.contains_any_reset() {
+                        break;
+                    }
+                }
+                StyleSource::None => {}
+                StyleSource::Style(_) => break,
+            }
+            rule_node = node.parent();
+        }
+        rule_node
+    }
+
     /// Finds a node in the properties matched cache.
     ///
     /// This needs to receive a `StyleBuilder` with the `early` properties
     /// already applied.
     pub fn find(
         &self,
+        guards: &StylesheetGuards,
         builder_with_early_props: &StyleBuilder,
     ) -> Option<&ComputedValues> {
         if builder_with_early_props.is_style_if_visited() {
@@ -102,7 +135,8 @@ impl RuleCache {
             return None;
         }
 
-        let rules = builder_with_early_props.rules.as_ref()?;
+        let rules = builder_with_early_props.rules.as_ref();
+        let rules = Self::get_rule_node_for_cache(guards, rules)?;
         let cached_values = self.map.get(rules)?;
 
         for &(ref conditions, ref values) in cached_values.iter() {
@@ -119,6 +153,7 @@ impl RuleCache {
     /// Returns whether the style was inserted into the cache.
     pub fn insert_if_possible(
         &mut self,
+        guards: &StylesheetGuards,
         style: &Arc<ComputedValues>,
         pseudo: Option<&PseudoElement>,
         conditions: &RuleCacheConditions,
@@ -138,8 +173,9 @@ impl RuleCache {
             return false;
         }
 
-        let rules = match style.rules {
-            Some(ref r) => r.clone(),
+        let rules = style.rules.as_ref();
+        let rules = match Self::get_rule_node_for_cache(guards, rules) {
+            Some(r) => r.clone(),
             None => return false,
         };
 

servo/components/style/rule_tree/mod.rs

@@ -864,7 +864,8 @@ impl StrongRuleNode {
         WeakRuleNode::from_ptr(self.ptr())
     }
 
-    fn parent(&self) -> Option<&StrongRuleNode> {
+    /// Get the parent rule node of this rule node.
+    pub fn parent(&self) -> Option<&StrongRuleNode> {
         self.get().parent.as_ref()
     }
 

servo/components/style/style_resolver.rs

@@ -597,7 +597,12 @@ where
         self.context
             .thread_local
             .rule_cache
-            .insert_if_possible(&values, pseudo, &conditions);
+            .insert_if_possible(
+                &self.context.shared.guards,
+                &values,
+                pseudo,
+                &conditions
+            );
 
         values
     }