servo: Merge #19696 - Skip rule node which contains only inherited properties for rule cache (from upsuper:rule-cache-opt); r=emilio
authorXidorn Quan <me@upsuper.org>
Fri, 05 Jan 2018 03:35:04 -0600
changeset 398033 d11c6c13f5a4c72faefbaaaad65cd9c2ad20841f
parent 398032 7fc6865ed3f2c5e6c864929c6e9be8852c962321
child 398034 1eb22728208e6fc9c785e5cf11bb68929d0a555d
push id98666
push userarchaeopteryx@coole-files.de
push dateSat, 06 Jan 2018 00:02:40 +0000
treeherdermozilla-inbound@04af74d5cdba [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1427681
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
servo/components/style/animation.rs
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/rule_cache.rs
servo/components/style/rule_tree/mod.rs
servo/components/style/style_resolver.rs
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -480,16 +480,17 @@ fn compute_style_for_animation_step(cont
             };
 
             // This currently ignores visited styles, which seems acceptable,
             // as existing browsers don't appear to animate visited styles.
             let computed =
                 properties::apply_declarations(context.stylist.device(),
                                                /* pseudo = */ None,
                                                previous_style.rules(),
+                                               &context.guards,
                                                iter,
                                                Some(previous_style),
                                                Some(previous_style),
                                                Some(previous_style),
                                                /* visited_style = */ None,
                                                font_metrics_provider,
                                                CascadeFlags::empty(),
                                                context.quirks_mode(),
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -278,16 +278,22 @@ impl PropertyDeclarationBlock {
         !self.declarations_importance.all_true()
     }
 
     /// Returns whether this block contains a declaration of a given longhand.
     pub fn contains(&self, id: LonghandId) -> bool {
         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.
     pub fn get(&self, property: PropertyDeclarationId) -> Option<(&PropertyDeclaration, Importance)> {
         self.declarations.iter().enumerate().find(|&(_, decl)| decl.id() == property).map(|(i, decl)| {
             let importance = if self.declarations_importance.get(i as u32) {
                 Importance::Important
             } else {
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -304,29 +304,46 @@ impl LonghandIdSet {
         for (self_cell, other_cell) in self.storage.iter().zip(other.storage.iter()) {
             if (*self_cell & *other_cell) != *other_cell {
                 return false;
             }
         }
         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 {
         LonghandIdSet { storage: [0; (${len(data.longhands)} - 1 + 32) / 32] }
     }
 
     /// Return whether the given property is in the set
     #[inline]
     pub fn contains(&self, id: LonghandId) -> bool {
         let bit = id as usize;
         (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) {
         let bit = id as usize;
         self.storage[bit / 32] |= 1 << (bit % 32);
     }
 
     /// Remove the given property from the set
@@ -3152,16 +3169,17 @@ pub fn cascade(
                 })
         })
     };
 
     apply_declarations(
         device,
         pseudo,
         rule_node,
+        guards,
         iter_declarations,
         parent_style,
         parent_style_ignoring_first_line,
         layout_parent_style,
         visited_style,
         font_metrics_provider,
         flags,
         quirks_mode,
@@ -3171,16 +3189,17 @@ pub fn cascade(
 }
 
 /// NOTE: This function expects the declaration with more priority to appear
 /// first.
 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>,
     layout_parent_style: Option<<&ComputedValues>,
     visited_style: Option<Arc<ComputedValues>>,
     font_metrics_provider: &FontMetricsProvider,
     flags: CascadeFlags,
     quirks_mode: QuirksMode,
@@ -3473,17 +3492,17 @@ where
                 let discriminant = LonghandId::FontSize as usize;
                 let size = PropertyDeclaration::CSSWideKeyword(
                     LonghandId::FontSize, CSSWideKeyword::Inherit);
 
                 (CASCADE_PROPERTY[discriminant])(&size, &mut context);
             % 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;
             }
         % endif // category == "early"
     % endfor
 
     let mut builder = context.builder;
 
--- a/servo/components/style/rule_cache.rs
+++ b/servo/components/style/rule_cache.rs
@@ -3,19 +3,20 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! A cache from rule node to computed values, in order to cache reset
 //! properties.
 
 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;
 
 /// The conditions for caching and matching a style in the rule cache.
 #[derive(Clone, Debug, Default)]
 pub struct RuleCacheConditions {
     uncacheable: bool,
     font_size: Option<NonNegativeLength>,
@@ -76,54 +77,88 @@ pub struct RuleCache {
 impl RuleCache {
     /// Creates an empty `RuleCache`.
     pub fn new() -> Self {
         Self {
             map: FnvHashMap::default(),
         }
     }
 
+    /// 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() {
             // FIXME(emilio): We can probably do better, does it matter much?
             return None;
         }
 
         // A pseudo-element with property restrictions can result in different
         // computed values if it's also used for a non-pseudo.
         if builder_with_early_props.pseudo
            .and_then(|p| p.property_restriction())
            .is_some() {
             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() {
             if conditions.matches(builder_with_early_props) {
                 debug!("Using cached reset style with conditions {:?}", conditions);
                 return Some(&**values)
             }
         }
         None
     }
 
     /// Inserts a node into the rules cache if possible.
     ///
     /// 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,
     ) -> bool {
         if !conditions.cacheable() {
             return false;
         }
 
@@ -133,18 +168,19 @@ impl RuleCache {
         }
 
         // A pseudo-element with property restrictions can result in different
         // computed values if it's also used for a non-pseudo.
         if pseudo.and_then(|p| p.property_restriction()).is_some() {
             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,
         };
 
         debug!("Inserting cached reset style with conditions {:?}", conditions);
         self.map
             .entry(rules)
             .or_insert_with(SmallVec::new)
             .push((conditions.clone(), style.clone()));
--- a/servo/components/style/rule_tree/mod.rs
+++ b/servo/components/style/rule_tree/mod.rs
@@ -859,17 +859,18 @@ impl StrongRuleNode {
             p: NonZeroPtrMut::new(ptr)
         }
     }
 
     fn downgrade(&self) -> WeakRuleNode {
         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()
     }
 
     fn ensure_child(
         &self,
         root: WeakRuleNode,
         source: StyleSource,
         level: CascadeLevel
--- a/servo/components/style/style_resolver.rs
+++ b/servo/components/style/style_resolver.rs
@@ -592,13 +592,18 @@ where
                 self.context.shared.quirks_mode(),
                 Some(&self.context.thread_local.rule_cache),
                 &mut conditions,
             );
 
         self.context
             .thread_local
             .rule_cache
-            .insert_if_possible(&values, pseudo, &conditions);
+            .insert_if_possible(
+                &self.context.shared.guards,
+                &values,
+                pseudo,
+                &conditions
+            );
 
         values
     }
 }