From f569274cca74efa20a9fc6d4a47b48fd50e29b17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 26 May 2017 17:47:32 +0200 Subject: [PATCH] Bug 1357583: Add a bunch of logging, shortcuts, and look also at the rightmost selector while invalidating sheets. r=heycam MozReview-Commit-ID: 2XGcOCTa7MV --- components/style/data.rs | 5 ++ components/style/invalidation/mod.rs | 80 ++++++++++++++++++++++------ components/style/restyle_hints.rs | 2 +- components/style/stylesheet_set.rs | 6 +++ 4 files changed, 76 insertions(+), 17 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index 90ca649e461..b16f918f949 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -388,6 +388,11 @@ impl StoredRestyleHint { self.0.insert(other.0) } + /// Contains whether the whole subtree is invalid. + pub fn contains_subtree(&self) -> bool { + self.0.contains(&RestyleHint::subtree()) + } + /// Insert another restyle hint, effectively resulting in the union of both. pub fn insert_from(&mut self, other: &Self) { self.0.insert_from(&other.0) diff --git a/components/style/invalidation/mod.rs b/components/style/invalidation/mod.rs index a1fecd63d9d..b720126b405 100644 --- a/components/style/invalidation/mod.rs +++ b/components/style/invalidation/mod.rs @@ -71,6 +71,7 @@ impl StylesheetInvalidationSet { /// Mark the DOM tree styles' as fully invalid. pub fn invalidate_fully(&mut self) { + debug!("StylesheetInvalidationSet::invalidate_fully"); self.invalid_scopes.clear(); self.fully_invalid = true; } @@ -84,9 +85,15 @@ impl StylesheetInvalidationSet { stylesheet: &Stylesheet, guard: &SharedRwLockReadGuard) { - if self.fully_invalid || - stylesheet.disabled() || + debug!("StylesheetInvalidationSet::collect_invalidations_for"); + if self.fully_invalid { + debug!(" > Fully invalid already"); + return; + } + + if stylesheet.disabled() || !stylesheet.is_effective_for_device(stylist.device(), guard) { + debug!(" > Stylesheet was not effective"); return; // Nothing to do here. } @@ -94,6 +101,9 @@ impl StylesheetInvalidationSet { stylesheet.rules.read_with(guard), stylist, guard); + + debug!(" > resulting invalidations: {:?}", self.invalid_scopes); + debug!(" > fully_invalid: {}", self.fully_invalid); } /// Clears the invalidation set, invalidating elements as needed if @@ -126,13 +136,25 @@ impl StylesheetInvalidationSet { return false; } + if let Some(ref r) = data.get_restyle() { + if r.hint.contains_subtree() { + debug!("process_invalidations_in_subtree: {:?} was already invalid", + element); + return false; + } + } + if self.fully_invalid { + debug!("process_invalidations_in_subtree: fully_invalid({:?})", + element); data.ensure_restyle().hint.insert(StoredRestyleHint::subtree()); return true; } for scope in &self.invalid_scopes { if scope.matches(element) { + debug!("process_invalidations_in_subtree: {:?} matched {:?}", + element, scope); data.ensure_restyle().hint.insert(StoredRestyleHint::subtree()); return true; } @@ -151,6 +173,8 @@ impl StylesheetInvalidationSet { } if any_children_invalid { + debug!("Children of {:?} changed, setting dirty descendants", + element); unsafe { element.set_dirty_descendants() } } @@ -171,6 +195,27 @@ impl StylesheetInvalidationSet { } } + fn scan_component( + component: &Component, + scope: &mut Option) + { + match *component { + Component::Class(ref class) => { + if scope.as_ref().map_or(true, |s| !s.is_id()) { + *scope = Some(InvalidationScope::Class(class.clone())); + } + } + Component::ID(ref id) => { + if scope.is_none() { + *scope = Some(InvalidationScope::ID(id.clone())); + } + } + _ => { + // Ignore everything else, at least for now. + } + } + } + /// Collect a style scopes for a given selector. /// /// We look at the outermost class or id selector to the left of an ancestor @@ -179,29 +224,30 @@ impl StylesheetInvalidationSet { /// We prefer id scopes to class scopes, and outermost scopes to innermost /// scopes (to reduce the amount of traversal we need to do). fn collect_scopes(&mut self, selector: &Selector) { + debug!("StylesheetInvalidationSet::collect_scopes({:?})", selector); + let mut scope: Option = None; - let iter = selector.inner.complex.iter_ancestors(); - for component in iter { - match *component { - Component::Class(ref class) => { - if scope.as_ref().map_or(true, |s| !s.is_id()) { - scope = Some(InvalidationScope::Class(class.clone())); - } + let mut scan = true; + let mut iter = selector.inner.complex.iter(); + + loop { + for component in &mut iter { + if scan { + Self::scan_component(component, &mut scope); } - Component::ID(ref id) => { - if scope.is_none() { - scope = Some(InvalidationScope::ID(id.clone())); - } - } - _ => { - // Ignore everything else, at least for now. + } + match iter.next_sequence() { + None => break, + Some(combinator) => { + scan = combinator.is_ancestor(); } } } match scope { Some(s) => { + debug!(" > Found scope: {:?}", s); self.invalid_scopes.insert(s); } None => { @@ -226,6 +272,7 @@ impl StylesheetInvalidationSet { -> bool { use stylesheets::CssRule::*; + debug!("StylesheetInvalidationSet::collect_invalidations_for_rule"); debug_assert!(!self.fully_invalid, "Not worth to be here!"); match *rule { @@ -281,6 +328,7 @@ impl StylesheetInvalidationSet { Viewport(..) => { debug!(" > Found unsupported rule, marking the whole subtree \ invalid."); + // TODO(emilio): Can we do better here? // // At least in `@page`, we could check the relevant media, I diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 57c9f13c672..9e5dc9ad3e4 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -368,7 +368,7 @@ impl RestyleHint { /// Returns whether this `RestyleHint` represents at least as much restyle /// work as the specified one. #[inline] - pub fn contains(&mut self, other: &Self) -> bool { + pub fn contains(&self, other: &Self) -> bool { self.match_under_self.contains(other.match_under_self) && (self.match_later_siblings & other.match_later_siblings) == other.match_later_siblings && self.replacements.contains(other.replacements) diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs index 93b14dbe7a4..2908d81bef4 100644 --- a/components/style/stylesheet_set.rs +++ b/components/style/stylesheet_set.rs @@ -78,6 +78,7 @@ impl StylesheetSet { unique_id: u64, guard: &SharedRwLockReadGuard) { + debug!("StylesheetSet::append_stylesheet"); self.remove_stylesheet_if_present(unique_id); self.entries.push(StylesheetSetEntry { unique_id: unique_id, @@ -98,6 +99,7 @@ impl StylesheetSet { unique_id: u64, guard: &SharedRwLockReadGuard) { + debug!("StylesheetSet::prepend_stylesheet"); self.remove_stylesheet_if_present(unique_id); self.entries.insert(0, StylesheetSetEntry { unique_id: unique_id, @@ -119,6 +121,7 @@ impl StylesheetSet { before_unique_id: u64, guard: &SharedRwLockReadGuard) { + debug!("StylesheetSet::insert_stylesheet_before"); self.remove_stylesheet_if_present(unique_id); let index = self.entries.iter().position(|x| { x.unique_id == before_unique_id @@ -136,6 +139,7 @@ impl StylesheetSet { /// Remove a given stylesheet from the set. pub fn remove_stylesheet(&mut self, unique_id: u64) { + debug!("StylesheetSet::remove_stylesheet"); self.remove_stylesheet_if_present(unique_id); self.dirty = true; // FIXME(emilio): We can do better! @@ -144,6 +148,7 @@ impl StylesheetSet { /// Notes that the author style has been disabled for this document. pub fn set_author_style_disabled(&mut self, disabled: bool) { + debug!("StylesheetSet::set_author_style_disabled"); if self.author_style_disabled == disabled { return; } @@ -164,6 +169,7 @@ impl StylesheetSet { -> StylesheetIterator where E: TElement, { + debug!("StylesheetSet::flush"); debug_assert!(self.dirty); self.dirty = false;