Auto merge of #18048 - bholley:more_traversal_refactoring_for_restyle_roots, r=emilio

More refactoring of the traversal in preparation for restyle roots

https://bugzilla.mozilla.org/show_bug.cgi?id=1389347

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18048)
<!-- Reviewable:end -->

components/style/dom.rs

@@ -34,7 +34,7 @@ use std::hash::Hash;
 use std::ops::Deref;
 use stylist::Stylist;
 use thread_state;
-use traversal_flags::TraversalFlags;
+use traversal_flags::{TraversalFlags, self};
 
 pub use style_traits::UnsafeNode;
 
@@ -489,6 +489,11 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
                    !data.restyle.hint.has_animation_hint_or_recascade();
         }
 
+        if traversal_flags.contains(traversal_flags::UnstyledOnly) {
+            // We don't process invalidations in UnstyledOnly mode.
+            return data.has_styles();
+        }
+
         if self.has_snapshot() && !self.handled_snapshot() {
             return false;
         }

components/style/gecko/generated/pseudo_element_definition.rs

@@ -173,6 +173,8 @@ pub enum PseudoElement {
         MozSVGText,
 }
 
+/// Important: If you change this, you should also update Gecko's
+/// nsCSSPseudoElements::IsEagerlyCascadedInServo.
 
 
 

components/style/parallel.rs

@@ -59,47 +59,26 @@ pub fn traverse_dom<E, D>(traversal: &D,
     where E: TElement,
           D: DomTraversal<E>,
 {
+    debug_assert!(traversal.is_parallel());
+    debug_assert!(token.should_traverse());
+
     let dump_stats = traversal.shared_context().options.dump_style_statistics;
     let start_time = if dump_stats { Some(time::precise_time_s()) } else { None };
 
-    // Set up the SmallVec. We need to move this, and in most cases this is just
-    // one node, so keep it small.
-    let mut nodes = SmallVec::<[SendNode<E::ConcreteNode>; 8]>::new();
-
-    debug_assert!(traversal.is_parallel());
-    // Handle Gecko's eager initial styling. We don't currently support it
-    // in conjunction with bottom-up traversal. If we did, we'd need to put
-    // it on the context to make it available to the bottom-up phase.
-    let depth = if token.traverse_unstyled_children_only() {
-        debug_assert!(!D::needs_postorder_traversal());
-        for kid in root.as_node().traversal_children() {
-            if kid.as_element().map_or(false, |el| el.get_data().is_none()) {
-                nodes.push(unsafe { SendNode::new(kid) });
-            }
-        }
-        root.depth() + 1
-    } else {
-        nodes.push(unsafe { SendNode::new(root.as_node()) });
-        root.depth()
-    };
-
-    if nodes.is_empty() {
-        return;
-    }
-
     let traversal_data = PerLevelTraversalData {
-        current_dom_depth: depth,
+        current_dom_depth: root.depth(),
     };
     let tls = ScopedTLS::<ThreadLocalStyleContext<E>>::new(pool);
-    let root = root.as_node().opaque();
+    let send_root = unsafe { SendNode::new(root.as_node()) };
 
     pool.install(|| {
         rayon::scope(|scope| {
-            let nodes = nodes;
+            let root = send_root;
-            traverse_nodes(&*nodes,
+            let root_opaque = root.opaque();
+            traverse_nodes(&[root],
                            DispatchMode::TailCall,
                            0,
-                           root,
+                           root_opaque,
                            traversal_data,
                            scope,
                            pool,

components/style/sequential.rs

@@ -35,16 +35,7 @@ pub fn traverse_dom<E, D>(traversal: &D,
     };
 
     let root_depth = root.depth();
-
+    discovered.push_back(WorkItem(root.as_node(), root_depth));
-    if token.traverse_unstyled_children_only() {
-        for kid in root.as_node().traversal_children() {
-            if kid.as_element().map_or(false, |el| el.get_data().is_none()) {
-                discovered.push_back(WorkItem(kid, root_depth + 1));
-            }
-        }
-    } else {
-        discovered.push_back(WorkItem(root.as_node(), root_depth));
-    }
 
     // Process the nodes breadth-first, just like the parallel traversal does.
     // This helps keep similar traversal characteristics for the style sharing

components/style/traversal.rs

@@ -31,23 +31,12 @@ pub struct PerLevelTraversalData {
     pub current_dom_depth: usize,
 }
 
-/// This structure exists to enforce that callers invoke pre_traverse, and also
+/// We use this structure, rather than just returning a boolean from pre_traverse,
-/// to pass information from the pre-traversal into the primary traversal.
+/// to enfore that callers process root invalidations before starting the traversal.
-pub struct PreTraverseToken {
+pub struct PreTraverseToken(bool);
-    traverse: bool,
-    unstyled_children_only: bool,
-}
-
 impl PreTraverseToken {
     /// Whether we should traverse children.
-    pub fn should_traverse(&self) -> bool {
+    pub fn should_traverse(&self) -> bool { self.0 }
-        self.traverse
-    }
-
-    /// Whether we should traverse only unstyled children.
-    pub fn traverse_unstyled_children_only(&self) -> bool {
-        self.unstyled_children_only
-    }
 }
 
 /// The kind of traversals we could perform.
@@ -157,33 +146,20 @@ pub trait DomTraversal<E: TElement> : Sync {
         }
     }
 
-    /// Must be invoked before traversing the root element to determine whether
+    /// Style invalidations happen when traversing from a parent to its children.
-    /// a traversal is needed. Returns a token that allows the caller to prove
+    /// However, this mechanism can't handle style invalidations on the root. As
-    /// that the call happened.
+    /// such, we have a pre-traversal step to handle that part and determine whether
-    ///
+    /// a full traversal is needed.
-    /// The traversal_flags is used in Gecko.
-    ///
-    /// If traversal_flag::UNSTYLED_CHILDREN_ONLY is specified, style newly-
-    /// appended children without restyling the parent.
-    ///
-    /// If traversal_flag::ANIMATION_ONLY is specified, style only elements for
-    /// animations.
     fn pre_traverse(
         root: E,
         shared_context: &SharedStyleContext,
         traversal_flags: TraversalFlags
     ) -> PreTraverseToken {
-        if traversal_flags.contains(traversal_flags::UnstyledChildrenOnly) {
+        // If this is an unstyled-only traversal, the caller has already verified
-            if root.borrow_data().map_or(true, |d| d.has_styles() && d.styles.is_display_none()) {
+        // that there's something to traverse, and we don't need to do any
-                return PreTraverseToken {
+        // invalidation since we're not doing any restyling.
-                    traverse: false,
+        if traversal_flags.contains(traversal_flags::UnstyledOnly) {
-                    unstyled_children_only: false,
+            return PreTraverseToken(true)
-                };
-            }
-            return PreTraverseToken {
-                traverse: true,
-                unstyled_children_only: true,
-            };
         }
 
         let flags = shared_context.traversal_flags;
@@ -205,10 +181,7 @@ pub trait DomTraversal<E: TElement> : Sync {
             parent_data.as_ref().map(|d| &**d)
         );
 
-        PreTraverseToken {
+        PreTraverseToken(should_traverse)
-            traverse: should_traverse,
-            unstyled_children_only: false,
-        }
     }
 
     /// Returns true if traversal should visit a text node. The style system
@@ -231,16 +204,32 @@ pub trait DomTraversal<E: TElement> : Sync {
     ) -> bool {
         debug!("element_needs_traversal({:?}, {:?}, {:?}, {:?})",
                el, traversal_flags, data, parent_data);
-        let data = match data {
+
-            Some(d) if d.has_styles() => d,
+        if traversal_flags.contains(traversal_flags::UnstyledOnly) {
-            _ => return !traversal_flags.for_animation_only(),
+            return data.map_or(true, |d| !d.has_styles()) || el.has_dirty_descendants();
-        };
+        }
+
+
+        // In case of animation-only traversal we need to traverse the element
+        // if the element has animation only dirty descendants bit,
+        // animation-only restyle hint or recascade.
+        if traversal_flags.for_animation_only() {
+            return data.map_or(false, |d| d.has_styles()) &&
+                   (el.has_animation_only_dirty_descendants() ||
+                    data.as_ref().unwrap().restyle.hint.has_animation_hint_or_recascade());
+        }
 
         // Non-incremental layout visits every node.
         if is_servo_nonincremental_layout() {
             return true;
         }
 
+        // Unwrap the data.
+        let data = match data {
+            Some(d) if d.has_styles() => d,
+            _ => return true,
+        };
+
         // If the element is native-anonymous and an ancestor frame will be
         // reconstructed, the child and all its descendants will be destroyed.
         // In that case, we wouldn't need to traverse the subtree...
@@ -283,14 +272,6 @@ pub trait DomTraversal<E: TElement> : Sync {
             }
         }
 
-        // In case of animation-only traversal we need to traverse the element
-        // if the element has animation only dirty descendants bit,
-        // animation-only restyle hint or recascade.
-        if traversal_flags.for_animation_only() {
-            return el.has_animation_only_dirty_descendants() ||
-                   data.restyle.hint.has_animation_hint_or_recascade();
-        }
-
         // If the dirty descendants bit is set, we need to traverse no matter
         // what. Skip examining the ElementData.
         if el.has_dirty_descendants() {
@@ -484,14 +465,15 @@ where
     D: DomTraversal<E>,
     F: FnMut(E::ConcreteNode),
 {
+    use traversal_flags::*;
+    let flags = context.shared.traversal_flags;
     context.thread_local.begin_element(element, data);
     context.thread_local.statistics.elements_traversed += 1;
-    debug_assert!(context.shared.traversal_flags.for_animation_only() ||
+    debug_assert!(flags.intersects(AnimationOnly | UnstyledOnly) ||
                   !element.has_snapshot() || element.handled_snapshot(),
                   "Should've handled snapshots here already");
 
-    let compute_self =
+    let compute_self = !element.has_current_styles_for_traversal(data, flags);
-        !element.has_current_styles_for_traversal(data, context.shared.traversal_flags);
     let mut hint = RestyleHint::empty();
 
     debug!("recalc_style_at: {:?} (compute_self={:?}, \
@@ -534,13 +516,17 @@ where
     }
 
     // Now that matching and cascading is done, clear the bits corresponding to
-    // those operations and compute the propagated restyle hint.
+    // those operations and compute the propagated restyle hint (unless we're
-    let mut propagated_hint = {
+    // not processing invalidations, in which case don't need to propagate it
-        debug_assert!(context.shared.traversal_flags.for_animation_only() ||
+    // and must avoid clearing it).
+    let mut propagated_hint = if flags.contains(UnstyledOnly) {
+        RestyleHint::empty()
+    } else {
+        debug_assert!(flags.for_animation_only() ||
                       !data.restyle.hint.has_animation_hint(),
                       "animation restyle hint should be handled during \
                        animation-only restyles");
-        data.restyle.hint.propagate(&context.shared.traversal_flags)
+        data.restyle.hint.propagate(&flags)
     };
 
     // FIXME(bholley): Need to handle explicitly-inherited reset properties
@@ -552,11 +538,10 @@ where
            propagated_hint,
            data.styles.is_display_none(),
            element.implemented_pseudo_element());
-    debug_assert!(element.has_current_styles_for_traversal(data, context.shared.traversal_flags),
+    debug_assert!(element.has_current_styles_for_traversal(data, flags),
                   "Should have computed style or haven't yet valid computed \
                    style in case of animation-only restyle");
 
-    let flags = context.shared.traversal_flags;
     let has_dirty_descendants_for_this_restyle =
         if flags.for_animation_only() {
             element.has_animation_only_dirty_descendants()
@@ -602,31 +587,33 @@ where
     // If we are in a forgetful traversal, drop the existing restyle
     // data here, since we won't need to perform a post-traversal to pick up
     // any change hints.
-    if flags.contains(traversal_flags::Forgetful) {
+    if flags.contains(Forgetful) {
         data.clear_restyle_flags_and_damage();
     }
 
-    // There are two cases when we want to clear the dity descendants bit here
+    // Optionally clear the descendants bit for the traversal type we're in.
-    // after styling this element. The first case is when we were explicitly
+    if flags.for_animation_only() {
-    // asked to clear the bit by the caller.
+        if flags.contains(ClearAnimationOnlyDirtyDescendants) {
-    //
+            unsafe { element.unset_animation_only_dirty_descendants(); }
-    // The second case is when this element is the root of a display:none
+        }
-    // subtree, even if the style didn't change (since, if the style did change,
+    } else {
-    // we'd have already cleared it above).
+        // There are two cases when we want to clear the dity descendants bit here
-    //
+        // after styling this element. The first case is when we were explicitly
-    // This keeps the tree in a valid state without requiring the DOM to check
+        // asked to clear the bit by the caller.
-    // display:none on the parent when inserting new children (which can be
+        //
-    // moderately expensive). Instead, DOM implementations can unconditionally
+        // The second case is when this element is the root of a display:none
-    // set the dirty descendants bit on any styled parent, and let the traversal
+        // subtree, even if the style didn't change (since, if the style did change,
-    // sort it out.
+        // we'd have already cleared it above).
-    if flags.contains(traversal_flags::ClearDirtyDescendants) ||
+        //
-       data.styles.is_display_none() {
+        // This keeps the tree in a valid state without requiring the DOM to check
-        unsafe { element.unset_dirty_descendants(); }
+        // display:none on the parent when inserting new children (which can be
-    }
+        // moderately expensive). Instead, DOM implementations can unconditionally
-
+        // set the dirty descendants bit on any styled parent, and let the traversal
-    // Similarly, check if we're supposed to clear the animation bit.
+        // sort it out.
-    if flags.contains(traversal_flags::ClearAnimationOnlyDirtyDescendants) {
+        if flags.contains(ClearDirtyDescendants) ||
-        unsafe { element.unset_animation_only_dirty_descendants(); }
+           data.styles.is_display_none() {
+            unsafe { element.unset_dirty_descendants(); }
+        }
     }
 
     context.thread_local.end_element(element);

components/style/traversal_flags.rs

@@ -16,8 +16,9 @@ bitflags! {
         /// Traverse and update all elements with CSS animations since
         /// @keyframes rules may have changed. Triggered by CSS rule changes.
         const ForCSSRuleChanges = 1 << 1,
-        /// Traverse only unstyled children of the root and their descendants.
+        /// Styles unstyled elements, but does not handle invalidations on
-        const UnstyledChildrenOnly = 1 << 2,
+        /// already-styled elements.
+        const UnstyledOnly = 1 << 2,
         /// A forgetful traversal ignores the previous state of the frame tree, and
         /// thus does not compute damage or maintain other state describing the styles
         /// pre-traversal. A forgetful traversal is usually the right thing if you
@@ -30,6 +31,10 @@ bitflags! {
         const ClearDirtyDescendants = 1 << 5,
         /// Clears the animation-only dirty descendants bit in the subtree.
         const ClearAnimationOnlyDirtyDescendants = 1 << 6,
+        /// Allows the traversal to run in parallel if there are sufficient cores on
+        /// the machine.
+        const ParallelTraversal = 1 << 7,
+
     }
 }
 
@@ -55,12 +60,13 @@ pub fn assert_traversal_flags_match() {
     check_traversal_flags! {
         ServoTraversalFlags_AnimationOnly => AnimationOnly,
         ServoTraversalFlags_ForCSSRuleChanges => ForCSSRuleChanges,
-        ServoTraversalFlags_UnstyledChildrenOnly => UnstyledChildrenOnly,
+        ServoTraversalFlags_UnstyledOnly => UnstyledOnly,
         ServoTraversalFlags_Forgetful => Forgetful,
         ServoTraversalFlags_AggressivelyForgetful => AggressivelyForgetful,
         ServoTraversalFlags_ClearDirtyDescendants => ClearDirtyDescendants,
         ServoTraversalFlags_ClearAnimationOnlyDirtyDescendants =>
             ClearAnimationOnlyDirtyDescendants,
+        ServoTraversalFlags_ParallelTraversal => ParallelTraversal,
     }
 }
 

ports/geckolib/glue.rs

@@ -229,7 +229,8 @@ fn traverse_subtree(element: GeckoElement,
     debug!("{:?}", ShowSubtreeData(element.as_node()));
 
     let style_thread_pool = &*STYLE_THREAD_POOL;
-    let traversal_driver = if style_thread_pool.style_thread_pool.is_none() || !element.is_root() {
+    let traversal_driver = if !traversal_flags.contains(traversal_flags::ParallelTraversal) ||
+                              style_thread_pool.style_thread_pool.is_none() {
         TraversalDriver::Sequential
     } else {
         TraversalDriver::Parallel
@@ -260,10 +261,9 @@ pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed,
     let element = GeckoElement(root);
 
     debug!("Servo_TraverseSubtree (flags={:?})", traversal_flags);
-
+    // It makes no sense to do an animation restyle when we're styling
-    // It makes no sense to do an animation restyle when we're restyling
     // newly-inserted content.
-    if !traversal_flags.contains(traversal_flags::UnstyledChildrenOnly) {
+    if !traversal_flags.contains(traversal_flags::UnstyledOnly) {
         let needs_animation_only_restyle =
             element.has_animation_only_dirty_descendants() ||
             element.has_animation_restyle_hints();