Bug 1357583: Add a bunch of logging, shortcuts, and look also at the rightmost selector while invalidating sheets. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 26 May 2017 17:47:32 +0200
changeset 585142 8fee90bc61560020ebb487c401b0d1cfd31e5863
parent 585141 de3e030f71e98a75f72e399fd4f771013f280ab2
child 585143 9fa7f1b2a1dca8f9a5a44dbd5d8849559ca97840
push id61023
push userbmo:emilio+bugs@crisal.io
push dateFri, 26 May 2017 16:09:20 +0000
reviewersheycam
bugs1357583
milestone55.0a1
Bug 1357583: Add a bunch of logging, shortcuts, and look also at the rightmost selector while invalidating sheets. r?heycam MozReview-Commit-ID: 2XGcOCTa7MV
servo/components/style/data.rs
servo/components/style/invalidation/mod.rs
servo/components/style/restyle_hints.rs
servo/components/style/stylesheet_set.rs
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -383,16 +383,21 @@ impl StoredRestyleHint {
         self.0.is_empty()
     }
 
     /// Insert another restyle hint, effectively resulting in the union of both.
     pub fn insert(&mut self, other: Self) {
         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)
     }
 
     /// Returns true if the hint has animation-only restyle.
     pub fn has_animation_hint(&self) -> bool {
         self.0.has_animation_hint()
--- a/servo/components/style/invalidation/mod.rs
+++ b/servo/components/style/invalidation/mod.rs
@@ -12,17 +12,17 @@ use dom::{TElement, TNode};
 use fnv::FnvHashSet;
 use selector_parser::SelectorImpl;
 use selectors::parser::{Component, Selector};
 use shared_lock::SharedRwLockReadGuard;
 use stylesheets::{CssRule, CssRules, Stylesheet};
 use stylist::Stylist;
 
 /// A style scope represents a kind of subtree that may need to be restyled.
-#[derive(Hash, Eq, PartialEq)]
+#[derive(Debug, Hash, Eq, PartialEq)]
 enum InvalidationScope {
     /// All the descendants of an element with a given id.
     ID(Atom),
     /// All the descendants of an element with a given class name.
     Class(Atom),
 }
 
 impl InvalidationScope {
@@ -64,39 +64,49 @@ impl StylesheetInvalidationSet {
         Self {
             invalid_scopes: FnvHashSet::default(),
             fully_invalid: false,
         }
     }
 
     /// 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;
     }
 
     /// Analyze the given stylesheet, and collect invalidations from their
     /// rules, in order to avoid doing a full restyle when we style the document
     /// next time.
     pub fn collect_invalidations_for(
         &mut self,
         stylist: &Stylist,
         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.
         }
 
         self.collect_invalidations_for_rule_list(
             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
     /// `document_element` is provided.
     pub fn flush<E>(&mut self, document_element: Option<E>)
         where E: TElement,
     {
         if let Some(e) = document_element {
@@ -119,23 +129,35 @@ impl StylesheetInvalidationSet {
             Some(data) => data,
             None => return false,
         };
 
         if !data.has_styles() {
             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;
             }
         }
 
 
         let mut any_children_invalid = false;
 
@@ -144,16 +166,18 @@ impl StylesheetInvalidationSet {
                 Some(e) => e,
                 None => continue,
             };
 
             any_children_invalid |= self.process_invalidations_in_subtree(child);
         }
 
         if any_children_invalid {
+            debug!("Children of {:?} changed, setting dirty descendants",
+                   element);
             unsafe { element.set_dirty_descendants() }
         }
 
         return any_children_invalid
     }
 
     /// Collects invalidations for a given list of CSS rules.
     fn collect_invalidations_for_rule_list(
@@ -164,50 +188,73 @@ impl StylesheetInvalidationSet {
     {
         for rule in &rules.0 {
             if !self.collect_invalidations_for_rule(rule, stylist, guard) {
                 return;
             }
         }
     }
 
+    fn scan_component(
+        component: &Component<SelectorImpl>,
+        scope: &mut Option<InvalidationScope>)
+    {
+        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
     /// combinator, in order to restyle only a given subtree.
     ///
     /// 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<SelectorImpl>) {
+        debug!("StylesheetInvalidationSet::collect_scopes({:?})", selector);
+
         let mut scope: Option<InvalidationScope> = 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 => {
+                debug!(" > Scope not found");
                 // If we didn't found a scope, any selector could match this, so
                 // let's just bail out.
                 self.fully_invalid = true;
             }
         }
     }
 
     /// Collects invalidations for a given CSS rule.
@@ -217,16 +264,17 @@ impl StylesheetInvalidationSet {
     fn collect_invalidations_for_rule(
         &mut self,
         rule: &CssRule,
         stylist: &Stylist,
         guard: &SharedRwLockReadGuard)
         -> bool
     {
         use stylesheets::CssRule::*;
+        debug!("StylesheetInvalidationSet::collect_invalidations_for_rule");
         debug_assert!(!self.fully_invalid, "Not worth to be here!");
 
         match *rule {
             Document(ref lock) => {
                 let doc_rule = lock.read_with(guard);
                 if !doc_rule.condition.evaluate(stylist.device()) {
                     return false; // Won't apply anything else after this.
                 }
@@ -270,16 +318,17 @@ impl StylesheetInvalidationSet {
                     stylist,
                     guard);
             }
             FontFace(..) |
             CounterStyle(..) |
             Keyframes(..) |
             Page(..) |
             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
                 // guess.
                 self.fully_invalid = true;
             }
         }
 
--- a/servo/components/style/restyle_hints.rs
+++ b/servo/components/style/restyle_hints.rs
@@ -363,17 +363,17 @@ impl RestyleHint {
         // A later patch should make it worthwhile to have an insert() function
         // that consumes its argument.
         self.insert_from(&other)
     }
 
     /// 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)
     }
 }
 
 impl RestyleReplacements {
     /// The replacements for the animation cascade levels.
--- a/servo/components/style/stylesheet_set.rs
+++ b/servo/components/style/stylesheet_set.rs
@@ -73,16 +73,17 @@ impl StylesheetSet {
     /// Appends a new stylesheet to the current set.
     pub fn append_stylesheet(
         &mut self,
         stylist: &Stylist,
         sheet: &Arc<Stylesheet>,
         unique_id: u64,
         guard: &SharedRwLockReadGuard)
     {
+        debug!("StylesheetSet::append_stylesheet");
         self.remove_stylesheet_if_present(unique_id);
         self.entries.push(StylesheetSetEntry {
             unique_id: unique_id,
             sheet: sheet.clone(),
         });
         self.dirty = true;
         self.invalidations.collect_invalidations_for(
             stylist,
@@ -93,16 +94,17 @@ impl StylesheetSet {
     /// Prepend a new stylesheet to the current set.
     pub fn prepend_stylesheet(
         &mut self,
         stylist: &Stylist,
         sheet: &Arc<Stylesheet>,
         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,
             sheet: sheet.clone(),
         });
         self.dirty = true;
         self.invalidations.collect_invalidations_for(
             stylist,
@@ -114,16 +116,17 @@ impl StylesheetSet {
     pub fn insert_stylesheet_before(
         &mut self,
         stylist: &Stylist,
         sheet: &Arc<Stylesheet>,
         unique_id: u64,
         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
         }).expect("`before_unique_id` stylesheet not found");
         self.entries.insert(index, StylesheetSetEntry {
             unique_id: unique_id,
             sheet: sheet.clone(),
         });
@@ -131,24 +134,26 @@ impl StylesheetSet {
         self.invalidations.collect_invalidations_for(
             stylist,
             sheet,
             guard)
     }
 
     /// 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!
         self.invalidations.invalidate_fully();
     }
 
     /// 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;
         }
         self.author_style_disabled = disabled;
         self.dirty = true;
         self.invalidations.invalidate_fully();
     }
 
@@ -159,16 +164,17 @@ impl StylesheetSet {
 
     /// Flush the current set, unmarking it as dirty, and returns an iterator
     /// over the new stylesheet list.
     pub fn flush<E>(&mut self,
                     document_element: Option<E>)
                     -> StylesheetIterator
         where E: TElement,
     {
+        debug!("StylesheetSet::flush");
         debug_assert!(self.dirty);
 
         self.dirty = false;
         self.invalidations.flush(document_element);
 
         StylesheetIterator(self.entries.iter())
     }