Auto merge of #14010 - bholley:element_data_management, r=emilio

incremental restyle: Centralize pre-styling setup, eliminate RestyleResult, and drop data for display:none subtrees

<!-- 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/14010)
<!-- Reviewable:end -->

components/layout/traversal.rs

@@ -19,16 +19,16 @@ use style::data::ElementData;
 use style::dom::{StylingMode, TElement, TNode};
 use style::traversal::{DomTraversalContext, put_thread_local_bloom_filter};
 use style::traversal::{recalc_style_at, remove_from_bloom_filter};
-use style::traversal::RestyleResult;
 use style::traversal::take_thread_local_bloom_filter;
 use util::opts;
-use wrapper::{LayoutNodeHelpers, LayoutNodeLayoutData};
+use wrapper::{GetRawData, LayoutNodeHelpers, LayoutNodeLayoutData};
 
 pub struct RecalcStyleAndConstructFlows<'lc> {
     context: LayoutContext<'lc>,
     root: OpaqueNode,
 }
 
+#[allow(unsafe_code)]
 impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
     where N: LayoutNode + TNode,
           N::ConcreteElement: LayoutElement
@@ -73,7 +73,7 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
         }
     }
 
-    fn process_preorder(&self, node: N) -> RestyleResult {
+    fn process_preorder(&self, node: N) {
         // FIXME(pcwalton): Stop allocating here. Ideally this should just be
         // done by the HTML parser.
         node.initialize_data();
@@ -101,11 +101,9 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
             let parent = node.parent_node().unwrap().as_element();
             let bf = take_thread_local_bloom_filter(parent, self.root, self.context.shared_context());
             put_thread_local_bloom_filter(bf, &node.to_unsafe(), self.context.shared_context());
-
-            RestyleResult::Stop
         } else {
             let el = node.as_element().unwrap();
-            recalc_style_at::<_, _, Self>(&self.context, self.root, el)
+            recalc_style_at::<_, _, Self>(&self.context, self.root, el);
         }
     }
 
@@ -114,8 +112,13 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
     }
 
     fn should_traverse_child(parent: N::ConcreteElement, child: N) -> bool {
+        // If the parent is display:none, we don't need to do anything.
+        if parent.is_display_none() {
+            return false;
+        }
+
         // If this node has been marked as damaged in some way, we need to
-        // traverse it unconditionally for layout.
+        // traverse it for layout.
         if child.has_changed() {
             return true;
         }
@@ -131,9 +134,13 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
         }
     }
 
-    fn ensure_element_data(element: &N::ConcreteElement) -> &AtomicRefCell<ElementData> {
+    unsafe fn ensure_element_data(element: &N::ConcreteElement) -> &AtomicRefCell<ElementData> {
         element.as_node().initialize_data();
-        element.get_style_data().unwrap()
+        element.get_data().unwrap()
+    }
+
+    unsafe fn clear_element_data(element: &N::ConcreteElement) {
+        element.as_node().clear_data();
     }
 
     fn local_context(&self) -> &LocalStyleContext {

components/layout/wrapper.rs

@@ -33,13 +33,21 @@
 use core::nonzero::NonZero;
 use data::{LayoutDataFlags, PersistentLayoutData};
 use script_layout_interface::{OpaqueStyleAndLayoutData, PartialPersistentLayoutData};
-use script_layout_interface::wrapper_traits::{ThreadSafeLayoutElement, ThreadSafeLayoutNode};
+use script_layout_interface::wrapper_traits::{LayoutNode, ThreadSafeLayoutElement, ThreadSafeLayoutNode};
 use script_layout_interface::wrapper_traits::GetLayoutData;
 use style::atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
 use style::computed_values::content::{self, ContentItem};
+use style::dom::TElement;
+use style::traversal::prepare_for_styling;
 
 pub type NonOpaqueStyleAndLayoutData = AtomicRefCell<PersistentLayoutData>;
 
+pub unsafe fn drop_style_and_layout_data(data: OpaqueStyleAndLayoutData) {
+    let ptr: *mut AtomicRefCell<PartialPersistentLayoutData> = *data.ptr;
+    let non_opaque: *mut NonOpaqueStyleAndLayoutData = ptr as *mut _;
+    let _ = Box::from_raw(non_opaque);
+}
+
 pub trait LayoutNodeLayoutData {
     /// Similar to borrow_data*, but returns the full PersistentLayoutData rather
     /// than only the style::data::ElementData.
@@ -62,28 +70,43 @@ impl<T: GetLayoutData> LayoutNodeLayoutData for T {
     }
 }
 
+pub trait GetRawData {
+    fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData>;
+}
+
+impl<T: GetLayoutData> GetRawData for T {
+    fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData> {
+        self.get_style_and_layout_data().map(|opaque| {
+            let container = *opaque.ptr as *mut NonOpaqueStyleAndLayoutData;
+            unsafe { &*container }
+        })
+    }
+}
+
 pub trait LayoutNodeHelpers {
     fn initialize_data(&self);
-    fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData>;
+    fn clear_data(&self);
 }
 
-impl<T: GetLayoutData> LayoutNodeHelpers for T {
+impl<T: LayoutNode> LayoutNodeHelpers for T {
     fn initialize_data(&self) {
         if self.get_raw_data().is_none() {
             let ptr: *mut NonOpaqueStyleAndLayoutData =
                 Box::into_raw(box AtomicRefCell::new(PersistentLayoutData::new()));
             let opaque = OpaqueStyleAndLayoutData {
                 ptr: unsafe { NonZero::new(ptr as *mut AtomicRefCell<PartialPersistentLayoutData>) }
             };
-            self.init_style_and_layout_data(opaque);
+            unsafe { self.init_style_and_layout_data(opaque) };
+            if let Some(el) = self.as_element() {
+                let _ = prepare_for_styling(el, el.get_data().unwrap());
+            }
         };
     }
 
-    fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData> {
-        self.get_style_and_layout_data().map(|opaque| {
-            let container = *opaque.ptr as *mut NonOpaqueStyleAndLayoutData;
-            unsafe { &*container }
-        })
+    fn clear_data(&self) {
+        if self.get_raw_data().is_some() {
+            unsafe { drop_style_and_layout_data(self.take_style_and_layout_data()) };
+        }
     }
 }
 

components/layout_thread/lib.rs

@@ -77,7 +77,8 @@ use layout::query::process_offset_parent_query;
 use layout::sequential;
 use layout::traversal::{ComputeAbsolutePositions, RecalcStyleAndConstructFlows};
 use layout::webrender_helpers::{WebRenderDisplayListConverter, WebRenderFrameBuilder};
-use layout::wrapper::{LayoutNodeLayoutData, NonOpaqueStyleAndLayoutData};
+use layout::wrapper::LayoutNodeLayoutData;
+use layout::wrapper::drop_style_and_layout_data;
 use layout_traits::LayoutThreadFactory;
 use msg::constellation_msg::PipelineId;
 use net_traits::image_cache_thread::{ImageCacheChan, ImageCacheResult, ImageCacheThread};
@@ -86,7 +87,6 @@ use profile_traits::mem::{self, Report, ReportKind, ReportsChan};
 use profile_traits::time::{self, TimerMetadata, profile};
 use profile_traits::time::{TimerMetadataFrameType, TimerMetadataReflowType};
 use script::layout_wrapper::{ServoLayoutDocument, ServoLayoutNode};
-use script_layout_interface::{OpaqueStyleAndLayoutData, PartialPersistentLayoutData};
 use script_layout_interface::message::{Msg, NewLayoutThreadInfo, Reflow, ReflowQueryType, ScriptReflow};
 use script_layout_interface::reporter::CSSErrorReporter;
 use script_layout_interface::restyle_damage::{REFLOW, REFLOW_OUT_OF_FLOW, REPAINT, REPOSITION};
@@ -105,7 +105,6 @@ use std::sync::{Arc, Mutex, MutexGuard, RwLock};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::mpsc::{Receiver, Sender, channel};
 use style::animation::Animation;
-use style::atomic_refcell::AtomicRefCell;
 use style::computed_values::{filter, mix_blend_mode};
 use style::context::{LocalStyleContextCreationInfo, ReflowGoal, SharedStyleContext};
 use style::dom::{TDocument, TElement, TNode};
@@ -648,7 +647,7 @@ impl LayoutThread {
             }
             Msg::ReapStyleAndLayoutData(dead_data) => {
                 unsafe {
-                    self.handle_reap_style_and_layout_data(dead_data)
+                    drop_style_and_layout_data(dead_data)
                 }
             }
             Msg::CollectReports(reports_chan) => {
@@ -756,7 +755,7 @@ impl LayoutThread {
             match self.port.recv().unwrap() {
                 Msg::ReapStyleAndLayoutData(dead_data) => {
                     unsafe {
-                        self.handle_reap_style_and_layout_data(dead_data)
+                        drop_style_and_layout_data(dead_data)
                     }
                 }
                 Msg::ExitNow => {
@@ -1483,14 +1482,6 @@ impl LayoutThread {
         }
     }
 
-    /// Handles a message to destroy layout data. Layout data must be destroyed on *this* thread
-    /// because the struct type is transmuted to a different type on the script side.
-    unsafe fn handle_reap_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        let ptr: *mut AtomicRefCell<PartialPersistentLayoutData> = *data.ptr;
-        let non_opaque: *mut NonOpaqueStyleAndLayoutData = ptr as *mut _;
-        let _ = Box::from_raw(non_opaque);
-    }
-
     /// Returns profiling information which is passed to the time profiler.
     fn profiler_metadata(&self) -> Option<TimerMetadata> {
         Some(TimerMetadata {

components/script/dom/node.rs

@@ -952,6 +952,7 @@ pub trait LayoutNodeHelpers {
 
     unsafe fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData>;
     unsafe fn init_style_and_layout_data(&self, OpaqueStyleAndLayoutData);
+    unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData;
 
     fn text_content(&self) -> String;
     fn selection(&self) -> Option<Range<usize>>;
@@ -1051,6 +1052,14 @@ impl LayoutNodeHelpers for LayoutJS<Node> {
         (*self.unsafe_get()).style_and_layout_data.set(Some(val));
     }
 
+    #[inline]
+    #[allow(unsafe_code)]
+    unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData {
+        let val = (*self.unsafe_get()).style_and_layout_data.get().unwrap();
+        (*self.unsafe_get()).style_and_layout_data.set(None);
+        val
+    }
+
     #[allow(unsafe_code)]
     fn text_content(&self) -> String {
         if let Some(text) = self.downcast::<Text>() {

components/script/layout_wrapper.rs

@@ -56,7 +56,7 @@ use std::mem::transmute;
 use std::sync::Arc;
 use std::sync::atomic::Ordering;
 use string_cache::{Atom, Namespace};
-use style::atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
+use style::atomic_refcell::{AtomicRef, AtomicRefCell};
 use style::attr::AttrValue;
 use style::computed_values::display;
 use style::context::SharedStyleContext;
@@ -252,6 +252,14 @@ impl<'ln> LayoutNode for ServoLayoutNode<'ln> {
         self.script_type_id().into()
     }
 
+    unsafe fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
+        self.get_jsmanaged().init_style_and_layout_data(data);
+    }
+
+    unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData {
+        self.get_jsmanaged().take_style_and_layout_data()
+    }
+
     fn has_changed(&self) -> bool {
         unsafe { self.node.get_flag(HAS_CHANGED) }
     }
@@ -269,42 +277,24 @@ impl<'ln> GetLayoutData for ServoLayoutNode<'ln> {
             self.get_jsmanaged().get_style_and_layout_data()
         }
     }
-
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        unsafe {
-            self.get_jsmanaged().init_style_and_layout_data(data);
-        }
-    }
 }
 
 impl<'le> GetLayoutData for ServoLayoutElement<'le> {
     fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData> {
         self.as_node().get_style_and_layout_data()
     }
-
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        self.as_node().init_style_and_layout_data(data)
-    }
 }
 
 impl<'ln> GetLayoutData for ServoThreadSafeLayoutNode<'ln> {
     fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData> {
         self.node.get_style_and_layout_data()
     }
-
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        self.node.init_style_and_layout_data(data)
-    }
 }
 
 impl<'le> GetLayoutData for ServoThreadSafeLayoutElement<'le> {
     fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData> {
         self.element.as_node().get_style_and_layout_data()
     }
-
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        self.element.as_node().init_style_and_layout_data(data)
-    }
 }
 
 impl<'ln> ServoLayoutNode<'ln> {
@@ -506,20 +496,11 @@ impl<'le> TElement for ServoLayoutElement<'le> {
         old_value - 1
     }
 
-    fn begin_styling(&self) -> AtomicRefMut<ElementData> {
-        let mut data = self.mutate_data().unwrap();
-        data.gather_previous_styles(|| None);
-        data
-    }
-
     fn borrow_data(&self) -> Option<AtomicRef<ElementData>> {
-        self.get_style_data().map(|d| d.borrow())
+        self.get_data().map(|d| d.borrow())
     }
 
-}
-
-impl<'le> LayoutElement for ServoLayoutElement<'le> {
-    fn get_style_data(&self) -> Option<&AtomicRefCell<ElementData>> {
+    fn get_data(&self) -> Option<&AtomicRefCell<ElementData>> {
         unsafe {
             self.get_style_and_layout_data().map(|d| {
                 let ppld: &AtomicRefCell<PartialPersistentLayoutData> = &**d.ptr;
@@ -551,10 +532,6 @@ impl<'le> ServoLayoutElement<'le> {
         }
     }
 
-    fn mutate_data(&self) -> Option<AtomicRefMut<ElementData>> {
-        self.get_style_data().map(|d| d.borrow_mut())
-    }
-
     fn get_partial_layout_data(&self) -> Option<&AtomicRefCell<PartialPersistentLayoutData>> {
         unsafe {
             self.get_style_and_layout_data().map(|d| &**d.ptr)
@@ -836,7 +813,7 @@ impl<'ln> ThreadSafeLayoutNode for ServoThreadSafeLayoutNode<'ln> {
         // also not visible to script.)
         debug_assert!(self.is_text_node());
         let parent = self.node.parent_node().unwrap().as_element().unwrap();
-        let parent_data = parent.get_style_data().unwrap().borrow();
+        let parent_data = parent.get_data().unwrap().borrow();
         parent_data.current_styles().primary.clone()
     }
 
@@ -1089,10 +1066,12 @@ impl<'le> ThreadSafeLayoutElement for ServoThreadSafeLayoutElement<'le> {
     }
 
     fn get_style_data(&self) -> Option<&AtomicRefCell<ElementData>> {
-        self.element.get_style_data()
+        self.element.get_data()
     }
 }
 
+impl<'le> LayoutElement for ServoLayoutElement<'le> {}
+
 /// This implementation of `::selectors::Element` is used for implementing lazy
 /// pseudo-elements.
 ///

components/script_layout_interface/wrapper_traits.rs

@@ -73,7 +73,6 @@ impl<T> PseudoElementType<T> {
 /// Trait to abstract access to layout data across various data structures.
 pub trait GetLayoutData {
     fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData>;
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData);
 }
 
 /// A wrapper so that layout can access only the methods that it should have access to. Layout must
@@ -85,6 +84,9 @@ pub trait LayoutNode: GetLayoutData + TNode {
     /// Returns the type ID of this node.
     fn type_id(&self) -> LayoutNodeType;
 
+    unsafe fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData);
+    unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData;
+
     fn has_changed(&self) -> bool;
 
     unsafe fn clear_dirty_bits(&self);
@@ -274,7 +276,6 @@ pub trait DangerousThreadSafeLayoutNode: ThreadSafeLayoutNode {
 }
 
 pub trait LayoutElement: Clone + Copy + Sized + Debug + GetLayoutData + TElement {
-    fn get_style_data(&self) -> Option<&AtomicRefCell<ElementData>>;
 }
 
 pub trait ThreadSafeLayoutElement: Clone + Copy + Sized + Debug +

components/style/atomic_refcell.rs

@@ -27,10 +27,10 @@ impl<T> AtomicRefCell<T> {
         AtomicRefCell(RwLock::new(value))
     }
     pub fn borrow(&self) -> AtomicRef<T> {
-        self.0.try_read().unwrap()
+        self.0.try_read().expect("already mutably borrowed")
     }
     pub fn borrow_mut(&self) -> AtomicRefMut<T> {
-        self.0.try_write().unwrap()
+        self.0.try_write().expect("already borrowed")
     }
 }