From d9606a4fae7d00abc7ddd0fe36a33ca21edad296 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 15 Feb 2017 19:36:57 -0800 Subject: [PATCH] Implement "handled for descendants" tracking for RestyleDamage. MozReview-Commit-ID: Bbk99ogILXC --- components/style/data.rs | 47 +++++++++++++++------ components/style/gecko/restyle_damage.rs | 43 ++++++++++++++++--- components/style/gecko_bindings/bindings.rs | 4 ++ components/style/matching.rs | 33 ++++++++++++++- components/style/servo/restyle_damage.rs | 19 +++++++++ components/style/traversal.rs | 12 ++++-- 6 files changed, 135 insertions(+), 23 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index ddf9370418a..a50d5a15fa4 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -245,7 +245,7 @@ static NO_SNAPSHOT: Option = None; /// We really want to store an Option here, but we can't drop Gecko /// Snapshots off-main-thread. So we make a convenient little wrapper to provide /// the semantics of Option, while deferring the actual drop. -#[derive(Debug)] +#[derive(Debug, Default)] pub struct SnapshotOption { snapshot: Option, destroyed: bool, @@ -292,7 +292,7 @@ impl Deref for SnapshotOption { /// Transient data used by the restyle algorithm. This structure is instantiated /// either before or during restyle traversal, and is cleared at the end of node /// processing. -#[derive(Debug)] +#[derive(Debug, Default)] pub struct RestyleData { /// The restyle hint, which indicates whether selectors need to be rematched /// for this element, its children, and its descendants. @@ -306,22 +306,21 @@ pub struct RestyleData { /// afte restyling. pub damage: RestyleDamage, + /// The restyle damage that has already been handled by our ancestors, and does + /// not need to be applied again at this element. Only non-empty during the + /// traversal, once ancestor damage has been calculated. + /// + /// Note that this optimization mostly makes sense in terms of Gecko's top-down + /// frame constructor and change list processing model. We don't bother with it + /// for Servo for now. + #[cfg(feature = "gecko")] + pub damage_handled: RestyleDamage, + /// An optional snapshot of the original state and attributes of the element, /// from which we may compute additional restyle hints at traversal time. pub snapshot: SnapshotOption, } -impl Default for RestyleData { - fn default() -> Self { - RestyleData { - hint: StoredRestyleHint::default(), - recascade: false, - damage: RestyleDamage::empty(), - snapshot: SnapshotOption::empty(), - } - } -} - impl RestyleData { /// Expands the snapshot (if any) into a restyle hint. Returns true if later /// siblings must be restyled. @@ -354,6 +353,28 @@ impl RestyleData { self.recascade || self.snapshot.is_some() } + + /// Returns damage handled. + #[cfg(feature = "gecko")] + pub fn damage_handled(&self) -> RestyleDamage { + self.damage_handled + } + + /// Returns damage handled (always empty for servo). + #[cfg(feature = "servo")] + pub fn damage_handled(&self) -> RestyleDamage { + RestyleDamage::empty() + } + + /// Sets damage handled. + #[cfg(feature = "gecko")] + pub fn set_damage_handled(&mut self, d: RestyleDamage) { + self.damage_handled = d; + } + + /// Sets damage handled. No-op for Servo. + #[cfg(feature = "servo")] + pub fn set_damage_handled(&mut self, _: RestyleDamage) {} } /// Style system data associated with an Element. diff --git a/components/style/gecko/restyle_damage.rs b/components/style/gecko/restyle_damage.rs index 074259920cd..4d6c1269eb8 100644 --- a/components/style/gecko/restyle_damage.rs +++ b/components/style/gecko/restyle_damage.rs @@ -9,7 +9,7 @@ use gecko_bindings::structs; use gecko_bindings::structs::{nsChangeHint, nsStyleContext}; use gecko_bindings::sugar::ownership::FFIArcHelpers; use properties::ComputedValues; -use std::ops::{BitOr, BitOrAssign}; +use std::ops::{BitAnd, BitOr, BitOrAssign, Not}; use std::sync::Arc; /// The representation of Gecko's restyle damage is just a wrapper over @@ -56,16 +56,35 @@ impl GeckoRestyleDamage { GeckoRestyleDamage(hint) } - /// Get a restyle damage that represents the maximum action to be taken - /// (rebuild and reflow). - pub fn rebuild_and_reflow() -> Self { + /// Returns true if this restyle damage contains all the damage of |other|. + pub fn contains(self, other: Self) -> bool { + self & other == other + } + + /// Gets restyle damage to reconstruct the entire frame, subsuming all + /// other damage. + pub fn reconstruct() -> Self { GeckoRestyleDamage(structs::nsChangeHint_nsChangeHint_ReconstructFrame) } + + /// Assuming |self| is applied to an element, returns the set of damage that + /// would be superfluous to apply for descendants. + pub fn handled_for_descendants(self) -> Self { + let hint = unsafe { + bindings::Gecko_HintsHandledForDescendants(self.0) + }; + GeckoRestyleDamage(hint) + } +} + +impl Default for GeckoRestyleDamage { + fn default() -> Self { + Self::empty() + } } impl BitOr for GeckoRestyleDamage { type Output = Self; - fn bitor(self, other: Self) -> Self { GeckoRestyleDamage(self.0 | other.0) } @@ -76,3 +95,17 @@ impl BitOrAssign for GeckoRestyleDamage { *self = *self | other; } } + +impl BitAnd for GeckoRestyleDamage { + type Output = Self; + fn bitand(self, other: Self) -> Self { + GeckoRestyleDamage(nsChangeHint((self.0).0 & (other.0).0)) + } +} + +impl Not for GeckoRestyleDamage { + type Output = Self; + fn not(self) -> Self { + GeckoRestyleDamage(nsChangeHint(!(self.0).0)) + } +} diff --git a/components/style/gecko_bindings/bindings.rs b/components/style/gecko_bindings/bindings.rs index 643deac69e4..46b7fe63bab 100644 --- a/components/style/gecko_bindings/bindings.rs +++ b/components/style/gecko_bindings/bindings.rs @@ -649,6 +649,10 @@ extern "C" { newstyle: ServoComputedValuesBorrowed) -> nsChangeHint; } +extern "C" { + pub fn Gecko_HintsHandledForDescendants(aHint: nsChangeHint) + -> nsChangeHint; +} extern "C" { pub fn Gecko_CreateElementSnapshot(element: RawGeckoElementBorrowed) -> ServoElementSnapshotOwned; diff --git a/components/style/matching.rs b/components/style/matching.rs index 2b180f47892..d710d558263 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -569,7 +569,36 @@ trait PrivateMatchMethods: TElement { } } + /// Computes and applies non-redundant damage. + #[cfg(feature = "gecko")] + fn accumulate_damage(&self, + restyle: &mut RestyleData, + old_values: &Arc, + new_values: &Arc, + pseudo: Option<&PseudoElement>) { + // If an ancestor is already getting reconstructed by Gecko's top-down + // frame constructor, no need to apply damage. + if restyle.damage_handled.contains(RestyleDamage::reconstruct()) { + restyle.damage = RestyleDamage::empty(); + return; + } + + // Add restyle damage, but only the bits that aren't redundant with respect + // to damage applied on our ancestors. + // + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1301258#c12 + // for followup work to make the optimization here more optimal by considering + // each bit individually. + if !restyle.damage.contains(RestyleDamage::reconstruct()) { + let new_damage = self.compute_restyle_damage(&old_values, &new_values, pseudo); + if !restyle.damage_handled.contains(new_damage) { + restyle.damage |= new_damage; + } + } + } + /// Computes and applies restyle damage unless we've already maxed it out. + #[cfg(feature = "servo")] fn accumulate_damage(&self, restyle: &mut RestyleData, old_values: &Arc, @@ -712,7 +741,7 @@ pub trait MatchMethods : TElement { if let Some(r) = data.get_restyle_mut() { // Any changes to the matched pseudo-elements trigger // reconstruction. - r.damage |= RestyleDamage::rebuild_and_reflow(); + r.damage |= RestyleDamage::reconstruct(); } } @@ -924,7 +953,7 @@ pub trait MatchMethods : TElement { RestyleDamage::empty() } else { // Something else. Be conservative for now. - RestyleDamage::rebuild_and_reflow() + RestyleDamage::reconstruct() } } } diff --git a/components/style/servo/restyle_damage.rs b/components/style/servo/restyle_damage.rs index f389da58b1a..68da9238afd 100644 --- a/components/style/servo/restyle_damage.rs +++ b/components/style/servo/restyle_damage.rs @@ -67,11 +67,19 @@ impl ServoRestyleDamage { /// Returns a bitmask that represents a flow that needs to be rebuilt and /// reflowed. + /// + /// FIXME(bholley): Do we ever actually need this? Shouldn't RECONSTRUCT_FLOW + /// imply everything else? pub fn rebuild_and_reflow() -> ServoRestyleDamage { REPAINT | REPOSITION | STORE_OVERFLOW | BUBBLE_ISIZES | REFLOW_OUT_OF_FLOW | REFLOW | RECONSTRUCT_FLOW } + /// Returns a bitmask indicating that the frame needs to be reconstructed. + pub fn reconstruct() -> ServoRestyleDamage { + RECONSTRUCT_FLOW + } + /// Supposing a flow has the given `position` property and this damage, /// returns the damage that we should add to the *parent* of this flow. pub fn damage_for_parent(self, child_is_absolutely_positioned: bool) -> ServoRestyleDamage { @@ -112,6 +120,17 @@ impl ServoRestyleDamage { } } } + + /// Servo doesn't implement this optimization. + pub fn handled_for_descendants(self) -> Self { + Self::empty() + } +} + +impl Default for ServoRestyleDamage { + fn default() -> Self { + Self::empty() + } } impl fmt::Display for ServoRestyleDamage { diff --git a/components/style/traversal.rs b/components/style/traversal.rs index f49a2042ec7..a5984e21a8d 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -455,7 +455,10 @@ pub fn recalc_style_at(traversal: &D, // Preprocess children, propagating restyle hints and handling sibling relationships. if traversal.should_traverse_children(&mut context.thread_local, element, &data, DontLog) && (element.has_dirty_descendants() || !propagated_hint.is_empty() || inherited_style_changed) { - preprocess_children(traversal, element, propagated_hint, inherited_style_changed); + let damage_handled = data.get_restyle().map_or(RestyleDamage::empty(), |r| { + r.damage_handled() | r.damage.handled_for_descendants() + }); + preprocess_children(traversal, element, propagated_hint, damage_handled, inherited_style_changed); } // Make sure the dirty descendants bit is not set for the root of a @@ -557,6 +560,7 @@ fn compute_style(_traversal: &D, fn preprocess_children(traversal: &D, element: E, mut propagated_hint: StoredRestyleHint, + damage_handled: RestyleDamage, parent_inherited_style_changed: bool) where E: TElement, D: DomTraversal @@ -580,8 +584,7 @@ fn preprocess_children(traversal: &D, // any reason to create one, avoid the useless allocation and move on to // the next child. if propagated_hint.is_empty() && !parent_inherited_style_changed && - !child_data.has_restyle() - { + damage_handled.is_empty() && !child_data.has_restyle() { continue; } let mut restyle_data = child_data.ensure_restyle(); @@ -598,6 +601,9 @@ fn preprocess_children(traversal: &D, propagated_hint.insert(&(RESTYLE_SELF | RESTYLE_DESCENDANTS).into()); } + // Store the damage already handled by ancestors. + restyle_data.set_damage_handled(damage_handled); + // If properties that we inherited from the parent changed, we need to recascade. // // FIXME(bholley): Need to handle explicitly-inherited reset properties somewhere.