From 825f623b3c372134fb5bca4fba853f633597fc20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 4 Sep 2017 14:50:13 +0200 Subject: [PATCH] style: Stop the cascade when only reset structs change. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: 1395227 Reviewed-by: heycam MozReview-Commit-ID: JCZJl2fmtJ9 Signed-off-by: Emilio Cobos Álvarez --- components/style/gecko/restyle_damage.rs | 21 +++--- components/style/matching.rs | 75 ++++++++++--------- .../style/properties/computed_value_flags.rs | 5 +- .../style/properties/properties.mako.rs | 4 + components/style/servo/restyle_damage.rs | 9 ++- components/style/traversal.rs | 56 +++++++++----- 6 files changed, 101 insertions(+), 69 deletions(-) diff --git a/components/style/gecko/restyle_damage.rs b/components/style/gecko/restyle_damage.rs index caea907eea6..f325e7916ed 100644 --- a/components/style/gecko/restyle_damage.rs +++ b/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 - /// (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. + /// given an old and a new style. 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) } diff --git a/components/style/matching.rs b/components/style/matching.rs index 6bc55eee0d6..dd198aba9f1 100644 --- a/components/style/matching.rs +++ b/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. - /// - /// 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, + MustCascadeChildren = 2, /// 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(&self, - shared_context: &SharedStyleContext, - restyle: &mut RestyleData, - old_values: &ComputedValues, - new_values: &ComputedValues, - pseudo: Option<&PseudoElement>) - -> ChildCascadeRequirement { + fn accumulate_damage_for( + &self, + shared_context: &SharedStyleContext, + restyle: &mut RestyleData, + old_values: &ComputedValues, + new_values: &ComputedValues, + pseudo: Option<&PseudoElement> + ) -> 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, - } - } - - /// Computes and applies restyle damage unless we've already maxed it out. - #[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, + StyleChange::Changed { reset_only } => { + if reset_only { + ChildCascadeRequirement::MustCascadeChildrenIfInheritResetStyle + } else { + ChildCascadeRequirement::MustCascadeChildren + } + } } } diff --git a/components/style/properties/computed_value_flags.rs b/components/style/properties/computed_value_flags.rs index 83a12618872..3ee32cd89de 100644 --- a/components/style/properties/computed_value_flags.rs +++ b/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, } } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index cfb7e04c1f3..7565353653a 100644 --- a/components/style/properties/properties.mako.rs +++ b/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 diff --git a/components/style/servo/restyle_damage.rs b/components/style/servo/restyle_damage.rs index fcec01bbe64..fd62a146ef7 100644 --- a/components/style/servo/restyle_damage.rs +++ b/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) } diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 0fac4d22117..4c066f87684 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -453,7 +453,9 @@ where D: DomTraversal, 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) { - ChildCascadeRequirement::MustCascadeChildren => { - hint |= RECASCADE_SELF; - } - ChildCascadeRequirement::MustCascadeDescendants => { - hint |= RECASCADE_SELF | RECASCADE_DESCENDANTS; - } - ChildCascadeRequirement::CanSkipCascade => {} - }; + child_cascade_requirement = + compute_style(traversal_data, context, element, data); - // 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 - // somewhere. - propagated_hint.insert(hint); - - trace!("propagated_hint={:?} \ + trace!("propagated_hint={:?}, cascade_requirement={:?}, \ 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( 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.