style: Stop the cascade when only reset structs change.

Bug: 1395227
Reviewed-by: heycam
MozReview-Commit-ID: JCZJl2fmtJ9
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>

components/style/gecko/restyle_damage.rs

@@ -42,27 +42,26 @@ impl GeckoRestyleDamage {
     }
 
     /// Computes the `StyleDifference` (including the appropriate change hint)
-    /// given an old style (in the form of a `nsStyleContext`, and a new style
+    /// given an old and a new style.
-    /// (in the form of `ComputedValues`).
-    ///
-    /// Note that we could in theory just get two `ComputedValues` here and diff
-    /// them, but Gecko has an interesting optimization when they mark accessed
-    /// structs, so they effectively only diff structs that have ever been
-    /// accessed from layout.
     pub fn compute_style_difference(
         old_style: &ComputedValues,
         new_style: &ComputedValues,
     ) -> StyleDifference {
-        let mut any_style_changed: bool = false;
+        let mut any_style_changed = false;
+        let mut reset_only = false;
         let hint = unsafe {
             bindings::Gecko_CalcStyleDifference(
                 old_style,
                 new_style,
-                structs::NS_STYLE_INHERIT_MASK as u64,
+                &mut any_style_changed,
-                &mut any_style_changed
+                &mut reset_only,
             )
         };
-        let change = if any_style_changed { StyleChange::Changed } else { StyleChange::Unchanged };
+        let change = if any_style_changed {
+            StyleChange::Changed { reset_only }
+        } else {
+            StyleChange::Unchanged
+        };
         StyleDifference::new(GeckoRestyleDamage(nsChangeHint(hint)), change)
     }
 

components/style/matching.rs

@@ -21,6 +21,7 @@ use servo_arc::{Arc, ArcBorrow};
 use traversal_flags;
 
 /// Represents the result of comparing an element's old and new style.
+#[derive(Debug)]
 pub struct StyleDifference {
     /// The resulting damage.
     pub damage: RestyleDamage,
@@ -40,12 +41,15 @@ impl StyleDifference {
 }
 
 /// Represents whether or not the style of an element has changed.
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug)]
 pub enum StyleChange {
     /// The style hasn't changed.
     Unchanged,
     /// The style has changed.
-    Changed,
+    Changed {
+        /// Whether only reset structs changed.
+        reset_only: bool,
+    },
 }
 
 /// Whether or not newly computed values for an element need to be cascade
@@ -56,19 +60,23 @@ pub enum ChildCascadeRequirement {
     /// we won't bother recomputing style for children, so we can skip cascading
     /// the new values into child elements.
     CanSkipCascade = 0,
+    /// The same as `MustCascadeChildren`, but we only need to actually
+    /// recascade if the child inherits any explicit reset style.
+    MustCascadeChildrenIfInheritResetStyle = 1,
     /// Old and new computed values were different, so we must cascade the
     /// new values to children.
-    ///
+    MustCascadeChildren = 2,
-    /// FIXME(heycam) Although this is "must" cascade, in the future we should
-    /// track whether child elements rely specifically on inheriting particular
-    /// property values.  When we do that, we can treat `MustCascadeChildren` as
-    /// "must cascade unless we know that changes to these properties can be
-    /// ignored".
-    MustCascadeChildren = 1,
     /// The same as `MustCascadeChildren`, but for the entire subtree.  This is
     /// used to handle root font-size updates needing to recascade the whole
     /// document.
-    MustCascadeDescendants = 2,
+    MustCascadeDescendants = 3,
+}
+
+impl ChildCascadeRequirement {
+    /// Whether we can unconditionally skip the cascade.
+    pub fn can_skip_cascade(&self) -> bool {
+        matches!(*self, ChildCascadeRequirement::CanSkipCascade)
+    }
 }
 
 bitflags! {
@@ -341,16 +349,19 @@ trait PrivateMatchMethods: TElement {
 
 
     /// Computes and applies non-redundant damage.
-    #[cfg(feature = "gecko")]
+    fn accumulate_damage_for(
-    fn accumulate_damage_for(&self,
+        &self,
-                             shared_context: &SharedStyleContext,
+        shared_context: &SharedStyleContext,
-                             restyle: &mut RestyleData,
+        restyle: &mut RestyleData,
-                             old_values: &ComputedValues,
+        old_values: &ComputedValues,
-                             new_values: &ComputedValues,
+        new_values: &ComputedValues,
-                             pseudo: Option<&PseudoElement>)
+        pseudo: Option<&PseudoElement>
-                             -> ChildCascadeRequirement {
+    ) -> ChildCascadeRequirement {
+        debug!("accumulate_damage_for: {:?}", self);
+
         // Don't accumulate damage if we're in a forgetful traversal.
         if shared_context.traversal_flags.contains(traversal_flags::Forgetful) {
+            debug!(" > forgetful traversal");
             return ChildCascadeRequirement::MustCascadeChildren;
         }
 
@@ -371,6 +382,8 @@ trait PrivateMatchMethods: TElement {
             restyle.damage |= difference.damage;
         }
 
+        debug!(" > style difference: {:?}", difference);
+
         // We need to cascade the children in order to ensure the correct
         // propagation of computed value flags.
         //
@@ -379,29 +392,19 @@ trait PrivateMatchMethods: TElement {
         // to handle justify-items: auto correctly when there's a legacy
         // justify-items.
         if old_values.flags != new_values.flags {
+            debug!(" > flags changed: {:?} != {:?}", old_values.flags, new_values.flags);
             return ChildCascadeRequirement::MustCascadeChildren;
         }
 
         match difference.change {
             StyleChange::Unchanged => ChildCascadeRequirement::CanSkipCascade,
-            StyleChange::Changed => ChildCascadeRequirement::MustCascadeChildren,
+            StyleChange::Changed { reset_only } => {
-        }
+                if reset_only {
-    }
+                    ChildCascadeRequirement::MustCascadeChildrenIfInheritResetStyle
-
+                } else {
-    /// Computes and applies restyle damage unless we've already maxed it out.
+                    ChildCascadeRequirement::MustCascadeChildren
-    #[cfg(feature = "servo")]
+                }
-    fn accumulate_damage_for(&self,
+            }
-                             _shared_context: &SharedStyleContext,
-                             restyle: &mut RestyleData,
-                             old_values: &ComputedValues,
-                             new_values: &ComputedValues,
-                             pseudo: Option<&PseudoElement>)
-                             -> ChildCascadeRequirement {
-        let difference = self.compute_style_difference(old_values, new_values, pseudo);
-        restyle.damage |= difference.damage;
-        match difference.change {
-            StyleChange::Changed => ChildCascadeRequirement::MustCascadeChildren,
-            StyleChange::Unchanged => ChildCascadeRequirement::CanSkipCascade,
         }
     }
 

components/style/properties/computed_value_flags.rs

@@ -11,7 +11,7 @@ bitflags! {
     /// anonymous boxes, see StyleBuilder::for_inheritance and its callsites.
     /// If we ever want to add some flags that shouldn't inherit for them,
     /// we might want to add a function to handle this.
-    pub flags ComputedValueFlags: u8 {
+    pub flags ComputedValueFlags: u16 {
         /// Whether the style or any of the ancestors has a text-decoration-line
         /// property that should get propagated to descendants.
         ///
@@ -56,5 +56,8 @@ bitflags! {
         ///
         /// Important because of the same reason.
         const INHERITS_CONTENT = 1 << 7,
+
+        /// Whether the child explicitly inherits any reset property.
+        const INHERITS_RESET_STYLE = 1 << 8,
     }
 }

components/style/properties/properties.mako.rs

@@ -2661,6 +2661,10 @@ impl<'a> StyleBuilder<'a> {
             self.inherited_style_ignoring_first_line.get_${property.style_struct.name_lower}();
         % endif
 
+        % if not property.style_struct.inherited:
+        self.flags.insert(::properties::computed_value_flags::INHERITS_RESET_STYLE);
+        % endif
+
         % if property.ident == "content":
         self.flags.insert(::properties::computed_value_flags::INHERITS_CONTENT);
         % endif

components/style/servo/restyle_damage.rs

@@ -65,7 +65,14 @@ impl ServoRestyleDamage {
         new: &ComputedValues,
     ) -> StyleDifference {
         let damage = compute_damage(old, new);
-        let change = if damage.is_empty() { StyleChange::Unchanged } else { StyleChange::Changed };
+        let change = if damage.is_empty() {
+            StyleChange::Unchanged
+        } else {
+            // FIXME(emilio): Differentiate between reset and inherited
+            // properties here, and set `reset_only` appropriately so the
+            // optimization to skip the cascade in those cases applies.
+            StyleChange::Changed { reset_only: false }
+        };
         StyleDifference::new(damage, change)
     }
 

components/style/traversal.rs

@@ -453,7 +453,9 @@ where
     D: DomTraversal<E>,
     F: FnMut(E::ConcreteNode),
 {
+    use std::cmp;
     use traversal_flags::*;
+
     let flags = context.shared.traversal_flags;
     context.thread_local.begin_element(element, data);
     context.thread_local.statistics.elements_traversed += 1;
@@ -462,28 +464,25 @@ where
                   "Should've handled snapshots here already");
 
     let compute_self = !element.has_current_styles_for_traversal(data, flags);
-    let mut hint = RestyleHint::empty();
 
     debug!("recalc_style_at: {:?} (compute_self={:?}, \
             dirty_descendants={:?}, data={:?})",
            element, compute_self, element.has_dirty_descendants(), data);
 
+    let mut child_cascade_requirement = ChildCascadeRequirement::CanSkipCascade;
+
     // Compute style for this element if necessary.
     if compute_self {
-        match compute_style(traversal_data, context, element, data) {
+        child_cascade_requirement =
-            ChildCascadeRequirement::MustCascadeChildren => {
+            compute_style(traversal_data, context, element, data);
-                hint |= RECASCADE_SELF;
-            }
-            ChildCascadeRequirement::MustCascadeDescendants => {
-                hint |= RECASCADE_SELF | RECASCADE_DESCENDANTS;
-            }
-            ChildCascadeRequirement::CanSkipCascade => {}
-        };
 
-        // We must always cascade native anonymous subtrees, since they inherit
-        // styles from their first non-NAC ancestor.
         if element.is_native_anonymous() {
-            hint |= RECASCADE_SELF;
+            // We must always cascade native anonymous subtrees, since they inherit
+            // styles from their first non-NAC ancestor.
+            child_cascade_requirement = cmp::max(
+                child_cascade_requirement,
+                ChildCascadeRequirement::MustCascadeChildren,
+            );
         }
 
         // If we're restyling this element to display:none, throw away all style
@@ -507,7 +506,7 @@ where
     // those operations and compute the propagated restyle hint (unless we're
     // not processing invalidations, in which case don't need to propagate it
     // and must avoid clearing it).
-    let mut propagated_hint = if flags.contains(UnstyledOnly) {
+    let propagated_hint = if flags.contains(UnstyledOnly) {
         RestyleHint::empty()
     } else {
         debug_assert!(flags.for_animation_only() ||
@@ -517,13 +516,10 @@ where
         data.restyle.hint.propagate(&flags)
     };
 
-    // FIXME(bholley): Need to handle explicitly-inherited reset properties
+    trace!("propagated_hint={:?}, cascade_requirement={:?}, \
-    // somewhere.
-    propagated_hint.insert(hint);
-
-    trace!("propagated_hint={:?} \
             is_display_none={:?}, implementing_pseudo={:?}",
            propagated_hint,
+           child_cascade_requirement,
            data.styles.is_display_none(),
            element.implemented_pseudo_element());
     debug_assert!(element.has_current_styles_for_traversal(data, flags),
@@ -553,6 +549,7 @@ where
     // enumerated in should_cull_subtree().
     let mut traverse_children = has_dirty_descendants_for_this_restyle ||
                                 !propagated_hint.is_empty() ||
+                                !child_cascade_requirement.can_skip_cascade() ||
                                 context.thread_local.is_initial_style() ||
                                 data.restyle.reconstructed_self() ||
                                 is_servo_nonincremental_layout();
@@ -567,6 +564,7 @@ where
             element,
             data,
             propagated_hint,
+            child_cascade_requirement,
             data.restyle.reconstructed_self_or_ancestor(),
             note_child
         );
@@ -778,6 +776,7 @@ fn note_children<E, D, F>(
     element: E,
     data: &ElementData,
     propagated_hint: RestyleHint,
+    cascade_requirement: ChildCascadeRequirement,
     reconstructed_ancestor: bool,
     mut note_child: F,
 )
@@ -816,7 +815,24 @@ where
             // subtree.
             child_data.restyle.set_reconstructed_ancestor(reconstructed_ancestor);
 
-            child_data.restyle.hint.insert(propagated_hint);
+            let mut child_hint = propagated_hint;
+            match cascade_requirement {
+                ChildCascadeRequirement::CanSkipCascade => {}
+                ChildCascadeRequirement::MustCascadeDescendants => {
+                    child_hint |= RECASCADE_SELF | RECASCADE_DESCENDANTS;
+                }
+                ChildCascadeRequirement::MustCascadeChildrenIfInheritResetStyle => {
+                    use properties::computed_value_flags::INHERITS_RESET_STYLE;
+                    if child_data.styles.primary().flags.contains(INHERITS_RESET_STYLE) {
+                        child_hint |= RECASCADE_SELF;
+                    }
+                }
+                ChildCascadeRequirement::MustCascadeChildren => {
+                    child_hint |= RECASCADE_SELF;
+                }
+            }
+
+            child_data.restyle.hint.insert(child_hint);
 
             // Handle element snapshots and invalidation of descendants and siblings
             // as needed.