auto merge of #1589 : pcwalton/servo/stop-removing-flows, r=larsbergstrom

to be leaves.

60% improvement in flow tree construction time on the rainbow page.

r? @larsbergstrom

3 files changed, 100 insertions, 52 deletions

diff --git a/src/components/main/layout/construct.rs b/src/components/main/layout/construct.rs
index e2605e6bd6c..dbb0f9d9b5b 100644
--- a/src/components/main/layout/construct.rs
+++ b/src/components/main/layout/construct.rs
@@ -26,7 +26,7 @@ use layout::box_::{Box, GenericBox, IframeBox, IframeBoxInfo, ImageBox, ImageBox
 use layout::box_::{UnscannedTextBox, UnscannedTextBoxInfo, InlineInfo, InlineParentInfo};
 use layout::context::LayoutContext;
 use layout::float_context::FloatType;
-use layout::flow::{BaseFlow, Flow, LeafSet, MutableOwnedFlowUtils};
+use layout::flow::{BaseFlow, Flow, ImmutableFlowUtils, LeafSet, MutableOwnedFlowUtils};
 use layout::inline::InlineFlow;
 use layout::text::TextRunScanner;
 use layout::util::{LayoutDataAccess, OpaqueNode};
@@ -274,13 +274,10 @@ impl<'fc> FlowConstructor<'fc> {
 
         let inline_base = BaseFlow::new(self.next_flow_id(), node);
         let mut inline_flow = ~InlineFlow::from_boxes(inline_base, boxes) as ~Flow;
-        self.layout_context.leaf_set.access(|leaf_set| leaf_set.insert(&inline_flow));
+        self.layout_context.leaf_set.access(|leaf_set| inline_flow.mark_as_leaf(leaf_set));
         TextRunScanner::new().scan_for_runs(self.font_context, inline_flow);
 
-        let mut inline_flow = Some(inline_flow);
-        self.layout_context.leaf_set.access(|leaf_set| {
-            flow.add_new_child(inline_flow.take_unwrap(), leaf_set)
-        })
+        flow.add_new_child(inline_flow)
     }
 
     /// Creates an inline flow from a set of inline boxes, if present, and adds it as a child of
@@ -323,10 +320,7 @@ impl<'fc> FlowConstructor<'fc> {
                     self.flush_inline_boxes_to_flow_if_necessary(&mut opt_boxes_for_inline_flow,
                                                                  flow,
                                                                  node);
-                    let mut kid_flow = Some(kid_flow);
-                    self.layout_context.leaf_set.access(|leaf_set| {
-                        flow.add_new_child(kid_flow.take_unwrap(), leaf_set)
-                    })
+                    flow.add_new_child(kid_flow)
                 }
                 ConstructionItemConstructionResult(InlineBoxesConstructionItem(
                         InlineBoxesConstructionResult {
@@ -366,10 +360,7 @@ impl<'fc> FlowConstructor<'fc> {
 
                                 // Push the flow generated by the {ib} split onto our list of
                                 // flows.
-                                let mut kid_flow = Some(kid_flow);
-                                self.layout_context.leaf_set.access(|leaf_set| {
-                                    flow.add_new_child(kid_flow.take_unwrap(), leaf_set)
-                                })
+                                flow.add_new_child(kid_flow)
                             }
                         }
                     }
@@ -386,6 +377,13 @@ impl<'fc> FlowConstructor<'fc> {
         self.flush_inline_boxes_to_flow_if_necessary(&mut opt_boxes_for_inline_flow,
                                                      flow,
                                                      node);
+
+        // The flow is done. If it ended up with no kids, add the flow to the leaf set.
+        if flow.child_count() == 0 {
+            self.layout_context.leaf_set.access(|leaf_set| flow.mark_as_leaf(leaf_set))
+        } else {
+            flow.mark_as_nonleaf()
+        }
     }
 
     /// Builds a flow for a node with `display: block`. This yields a `BlockFlow` with possibly
@@ -395,9 +393,6 @@ impl<'fc> FlowConstructor<'fc> {
         let base = BaseFlow::new(self.next_flow_id(), node);
         let box_ = self.build_box_for_node(node);
         let mut flow = ~BlockFlow::from_box(base, box_, is_fixed) as ~Flow;
-
-        self.layout_context.leaf_set.access(|leaf_set| leaf_set.insert(&flow));
-
         self.build_children_of_block_flow(&mut flow, node);
         flow
     }
@@ -410,9 +405,6 @@ impl<'fc> FlowConstructor<'fc> {
         let box_ = self.build_box_for_node(node);
 
         let mut flow = ~BlockFlow::float_from_box(base, float_type, box_) as ~Flow;
-
-        self.layout_context.leaf_set.access(|leaf_set| leaf_set.insert(&flow));
-
         self.build_children_of_block_flow(&mut flow, node);
         flow
     }
diff --git a/src/components/main/layout/flow.rs b/src/components/main/layout/flow.rs
index 175baeddea6..56d81fc78ea 100644
--- a/src/components/main/layout/flow.rs
+++ b/src/components/main/layout/flow.rs
@@ -197,12 +197,6 @@ pub trait MutableFlowUtils {
     /// Invokes a closure with the last child of this flow.
     fn with_last_child<R>(self, f: |Option<&mut ~Flow>| -> R) -> R;
 
-    /// Removes the first child of this flow and destroys it.
-    fn remove_first(self);
-
-    /// Removes the last child of this flow and destroys it.
-    fn remove_last(self);
-
     /// Computes the overflow region for this flow.
     fn store_overflow(self, _: &mut LayoutContext);
 
@@ -218,7 +212,14 @@ pub trait MutableFlowUtils {
 pub trait MutableOwnedFlowUtils {
     /// Adds a new flow as a child of this flow. Removes the flow from the given leaf set if
     /// it's present.
-    fn add_new_child(&mut self, new_child: ~Flow, leaf_set: &mut LeafSet);
+    fn add_new_child(&mut self, new_child: ~Flow);
+
+    /// Marks the flow as a leaf. The flow must not have children and must not be marked as a
+    /// nonleaf.
+    fn mark_as_leaf(&mut self, leaf_set: &mut LeafSet);
+
+    /// Marks the flow as a nonleaf. The flow must not be marked as a leaf.
+    fn mark_as_nonleaf(&mut self);
 
     /// Destroys the flow.
     fn destroy(&mut self, leaf_set: &mut LeafSet);
@@ -263,7 +264,7 @@ pub trait PostorderFlowTraversal {
 }
 
 #[deriving(Clone)]
-pub struct FlowFlagsInfo{
+pub struct FlowFlagsInfo {
     flags: FlowFlags,
 
     /// text-decoration colors
@@ -284,12 +285,12 @@ pub struct FlowFlags(u8);
 /// The bitmask of flags that represent text decoration fields that get propagated downward.
 ///
 /// NB: If you update this field, you must update the bitfields below.
-static TEXT_DECORATION_OVERRIDE_BITMASK: u8 = 0b00001110;
+static TEXT_DECORATION_OVERRIDE_BITMASK: u8 = 0b0000_1110;
 
 /// The bitmask of flags that represent the text alignment field.
 ///
 /// NB: If you update this field, you must update the bitfields below.
-static TEXT_ALIGN_BITMASK: u8 = 0b00110000;
+static TEXT_ALIGN_BITMASK: u8 = 0b0011_0000;
 
 /// The number of bits we must shift off to handle the text alignment field.
 ///
@@ -428,22 +429,29 @@ impl FlowFlagsInfo {
 }
 
 // Whether we need an in-order traversal.
-bitfield!(FlowFlags, inorder, set_inorder, 0x01)
+bitfield!(FlowFlags, inorder, set_inorder, 0b0000_0001)
 
 // Whether this flow forces `text-decoration: underline` on.
 //
 // NB: If you update this, you need to update TEXT_DECORATION_OVERRIDE_BITMASK.
-bitfield!(FlowFlags, override_underline, set_override_underline, 0x02)
+bitfield!(FlowFlags, override_underline, set_override_underline, 0b0000_0010)
 
 // Whether this flow forces `text-decoration: overline` on.
 //
 // NB: If you update this, you need to update TEXT_DECORATION_OVERRIDE_BITMASK.
-bitfield!(FlowFlags, override_overline, set_override_overline, 0x04)
+bitfield!(FlowFlags, override_overline, set_override_overline, 0b0000_0100)
 
 // Whether this flow forces `text-decoration: line-through` on.
 //
 // NB: If you update this, you need to update TEXT_DECORATION_OVERRIDE_BITMASK.
-bitfield!(FlowFlags, override_line_through, set_override_line_through, 0x08)
+bitfield!(FlowFlags, override_line_through, set_override_line_through, 0b0000_1000)
+
+// Whether this flow is marked as a leaf. Flows marked as leaves must not have any more kids added
+// to them.
+bitfield!(FlowFlags, is_leaf, set_is_leaf, 0b0100_0000)
+
+// Whether this flow is marked as a nonleaf. Flows marked as nonleaves must have children.
+bitfield!(FlowFlags, is_nonleaf, set_is_nonleaf, 0b1000_0000)
 
 // The text alignment for this flow.
 impl FlowFlags {
@@ -678,16 +686,6 @@ impl<'a> MutableFlowUtils for &'a mut Flow {
         f(mut_base(self).children.back_mut())
     }
 
-    /// Removes the first child of this flow and destroys it.
-    fn remove_first(self) {
-        let _ = mut_base(self).children.pop_front();
-    }
-
-    /// Removes the last child of this flow and destroys it.
-    fn remove_last(self) {
-        let _ = mut_base(self).children.pop_back();
-    }
-
     fn store_overflow(self, _: &mut LayoutContext) {
         let my_position = mut_base(self).position;
         let mut overflow = my_position;
@@ -743,23 +741,45 @@ impl<'a> MutableFlowUtils for &'a mut Flow {
 }
 
 impl MutableOwnedFlowUtils for ~Flow {
-    /// Adds a new flow as a child of this flow. Removes the flow from the given leaf set if
-    /// it's present.
-    fn add_new_child(&mut self, mut new_child: ~Flow, leaf_set: &mut LeafSet) {
-        if self.child_count() == 0 {
-            leaf_set.remove(self)
-        }
-
+    /// Adds a new flow as a child of this flow. Fails if this flow is marked as a leaf.
+    fn add_new_child(&mut self, mut new_child: ~Flow) {
         {
             let kid_base = mut_base(new_child);
             kid_base.parallel.parent = parallel::mut_owned_flow_to_unsafe_flow(self);
         }
 
         let base = mut_base(*self);
+        assert!(!base.flags_info.flags.is_leaf());
         base.children.push_back(new_child);
         let _ = base.parallel.children_count.fetch_add(1, Relaxed);
     }
 
+    /// Marks the flow as a leaf. The flow must not have children and must not be marked as a
+    /// nonleaf.
+    fn mark_as_leaf(&mut self, leaf_set: &mut LeafSet) {
+        {
+            let base = mut_base(*self);
+            if base.flags_info.flags.is_nonleaf() {
+                fail!("attempted to mark a nonleaf flow as a leaf!")
+            }
+            if base.children.len() != 0 {
+                fail!("attempted to mark a flow with children as a leaf!")
+            }
+            base.flags_info.flags.set_is_leaf(true)
+        }
+        leaf_set.insert(self)
+    }
+
+    /// Marks the flow as a nonleaf. The flow must not be marked as a leaf.
+    fn mark_as_nonleaf(&mut self) {
+        let base = mut_base(*self);
+        if base.flags_info.flags.is_leaf() {
+            fail!("attempted to mark a leaf flow as a nonleaf!")
+        }
+        base.flags_info.flags.set_is_nonleaf(true)
+        // We don't check to make sure there are no children as they might be added later.
+    }
+
     /// Destroys the flow.
     fn destroy(&mut self, leaf_set: &mut LeafSet) {
         let is_leaf = {
@@ -796,7 +816,7 @@ impl LeafSet {
     }
 
     /// Inserts a newly-created flow into the leaf set.
-    pub fn insert(&mut self, flow: &~Flow) {
+    fn insert(&mut self, flow: &~Flow) {
         self.set.insert(parallel::owned_flow_to_unsafe_flow(flow));
     }
 
diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs
index 61af5250de1..12b2c9af7ed 100644
--- a/src/components/main/layout/layout_task.rs
+++ b/src/components/main/layout/layout_task.rs
@@ -143,6 +143,24 @@ impl PreorderFlowTraversal for PropagateDamageTraversal {
     }
 }
 
+/// The flow tree verification traversal. This is only on in debug builds.
+#[cfg(debug)]
+struct FlowTreeVerificationTraversal;
+
+#[cfg(debug)]
+impl PreorderFlowTraversal for FlowTreeVerificationTraversal {
+    #[inline]
+    fn process(&mut self, flow: &mut Flow) -> bool {
+        let base = flow::base(flow);
+        if !base.flags_info.flags.is_leaf() && !base.flags_info.flags.is_nonleaf() {
+            println("flow tree verification failed: flow wasn't a leaf or a nonleaf!");
+            flow.dump();
+            fail!("flow tree verification failed")
+        }
+        true
+    }
+}
+
 /// The bubble-widths traversal, the first part of layout computation. This computes preferred
 /// and intrinsic widths and bubbles them up the tree.
 pub struct BubbleWidthsTraversal<'a> {
@@ -472,6 +490,19 @@ impl LayoutTask {
         }
     }
 
+    /// Verifies that every node was either marked as a leaf or as a nonleaf in the flow tree.
+    /// This is only on in debug builds.
+    #[inline(never)]
+    #[cfg(debug)]
+    fn verify_flow_tree(&mut self, layout_root: &mut ~Flow) {
+        let mut traversal = FlowTreeVerificationTraversal;
+        layout_root.traverse_preorder(&mut traversal);
+    }
+
+    #[cfg(not(debug))]
+    fn verify_flow_tree(&mut self, _: &mut ~Flow) {
+    }
+
     /// The high-level routine that performs layout tasks.
     fn handle_reflow(&mut self, data: &Reflow) {
         // FIXME: Isolate this transmutation into a "bridge" module.
@@ -548,6 +579,11 @@ impl LayoutTask {
                     || self.construct_flow_tree(&mut layout_ctx, *node))
         });
 
+        // Verification of the flow tree, which ensures that all nodes were either marked as leaves
+        // or as non-leaves. This becomes a no-op in release builds. (It is inconsequential to
+        // memory safety but is a useful debugging tool.)
+        self.verify_flow_tree(&mut layout_root);
+
         // Propagate damage.
         profile(time::LayoutDamagePropagateCategory, self.profiler_chan.clone(), || {
             layout_root.traverse_preorder(&mut PropagateDamageTraversal {