Hoist bloom filter into scoped TLS and simplify code.

components/layout/traversal.rs

@@ -9,7 +9,6 @@ use context::{LayoutContext, ScopedThreadLocalLayoutContext, SharedLayoutContext
 use display_list_builder::DisplayListBuildState;
 use flow::{self, PreorderFlowTraversal};
 use flow::{CAN_BE_FRAGMENTED, Flow, ImmutableFlowUtils, PostorderFlowTraversal};
-use gfx::display_list::OpaqueNode;
 use script_layout_interface::wrapper_traits::{LayoutNode, ThreadSafeLayoutNode};
 use servo_config::opts;
 use style::atomic_refcell::AtomicRefCell;
@@ -18,13 +17,12 @@ use style::data::ElementData;
 use style::dom::{TElement, TNode};
 use style::selector_parser::RestyleDamage;
 use style::servo::restyle_damage::{BUBBLE_ISIZES, REFLOW, REFLOW_OUT_OF_FLOW, REPAINT};
-use style::traversal::{DomTraversal, recalc_style_at, remove_from_bloom_filter};
+use style::traversal::{DomTraversal, recalc_style_at};
 use style::traversal::PerLevelTraversalData;
 use wrapper::{GetRawData, LayoutNodeHelpers, LayoutNodeLayoutData};
 
 pub struct RecalcStyleAndConstructFlows {
     shared: SharedLayoutContext,
-    root: OpaqueNode,
 }
 
 impl RecalcStyleAndConstructFlows {
@@ -35,10 +33,9 @@ impl RecalcStyleAndConstructFlows {
 
 impl RecalcStyleAndConstructFlows {
     /// Creates a traversal context, taking ownership of the shared layout context.
-    pub fn new(shared: SharedLayoutContext, root: OpaqueNode) -> Self {
+    pub fn new(shared: SharedLayoutContext) -> Self {
         RecalcStyleAndConstructFlows {
             shared: shared,
-            root: root,
         }
     }
 
@@ -75,7 +72,7 @@ impl<N> DomTraversal<N> for RecalcStyleAndConstructFlows
 
     fn process_postorder(&self, thread_local: &mut Self::ThreadLocalContext, node: N) {
         let context = LayoutContext::new(&self.shared);
-        construct_flows_at(&context, thread_local, self.root, node);
+        construct_flows_at(&context, thread_local, node);
     }
 
     fn text_node_needs_traversal(node: N) -> bool {
@@ -115,8 +112,8 @@ pub trait PostorderNodeMutTraversal<ConcreteThreadSafeLayoutNode: ThreadSafeLayo
 #[inline]
 #[allow(unsafe_code)]
 fn construct_flows_at<'a, N>(context: &LayoutContext<'a>,
-                             _thread_local: &ScopedThreadLocalLayoutContext<N::ConcreteElement>,
-                             root: OpaqueNode, node: N)
+                             thread_local: &mut ScopedThreadLocalLayoutContext<N::ConcreteElement>,
+                             node: N)
     where N: LayoutNode,
 {
     debug!("construct_flows_at: {:?}", node);
@@ -143,7 +140,7 @@ fn construct_flows_at<'a, N>(context: &LayoutContext<'a>,
         el.mutate_data().unwrap().persist();
         unsafe { el.unset_dirty_descendants(); }
 
-        remove_from_bloom_filter(&context.shared.style_context, root, el);
+        thread_local.style_context.bloom_filter.maybe_pop(el);
     }
 }
 

components/layout_thread/lib.rs

@@ -1152,7 +1152,7 @@ impl LayoutThread {
                                                                          data.reflow_info.goal);
 
         // NB: Type inference falls apart here for some reason, so we need to be very verbose. :-(
-        let traversal = RecalcStyleAndConstructFlows::new(shared_layout_context, element.as_node().opaque());
+        let traversal = RecalcStyleAndConstructFlows::new(shared_layout_context);
         let dom_depth = Some(0); // This is always the root node.
         let token = {
             let stylist = &<RecalcStyleAndConstructFlows as

components/style/bloom.rs

@@ -19,29 +19,20 @@ pub struct StyleBloom {
     /// Note that the use we do for them is safe, since the data we access from
     /// them is completely read-only during restyling.
     elements: Vec<UnsafeNode>,
-
-    /// A monotonic counter incremented which each reflow in order to invalidate
-    /// the bloom filter if appropriate.
-    generation: u32,
 }
 
 impl StyleBloom {
-    pub fn new(generation: u32) -> Self {
+    pub fn new() -> Self {
         StyleBloom {
             filter: Box::new(BloomFilter::new()),
             elements: vec![],
-            generation: generation,
         }
     }
 
     pub fn filter(&self) -> &BloomFilter {
         &*self.filter
     }
 
-    pub fn generation(&self) -> u32 {
-        self.generation
-    }
-
     pub fn maybe_pop<E>(&mut self, element: E)
         where E: TElement + MatchMethods
     {
@@ -129,14 +120,12 @@ impl StyleBloom {
     /// Returns the new bloom filter depth.
     pub fn insert_parents_recovering<E>(&mut self,
                                         element: E,
-                                        element_depth: Option<usize>,
-                                        generation: u32)
+                                        element_depth: Option<usize>)
                                         -> usize
         where E: TElement,
     {
         // Easy case, we're in a different restyle, or we're empty.
-        if self.generation != generation || self.elements.is_empty() {
-            self.generation = generation;
+        if self.elements.is_empty() {
             return self.rebuild(element);
         }
 

components/style/context.rs

@@ -6,6 +6,7 @@
 
 use animation::Animation;
 use app_units::Au;
+use bloom::StyleBloom;
 use dom::{OpaqueNode, TElement};
 use error_reporting::ParseErrorReporter;
 use euclid::Size2D;
@@ -77,6 +78,7 @@ pub struct SharedStyleContext {
 
 pub struct ThreadLocalStyleContext<E: TElement> {
     pub style_sharing_candidate_cache: StyleSharingCandidateCache<E>,
+    pub bloom_filter: StyleBloom,
     /// A channel on which new animations that have been triggered by style
     /// recalculation can be sent.
     pub new_animations_sender: Sender<Animation>,
@@ -86,6 +88,7 @@ impl<E: TElement> ThreadLocalStyleContext<E> {
     pub fn new(shared: &SharedStyleContext) -> Self {
         ThreadLocalStyleContext {
             style_sharing_candidate_cache: StyleSharingCandidateCache::new(),
+            bloom_filter: StyleBloom::new(),
             new_animations_sender: shared.local_context_creation_data.lock().unwrap().new_animations_sender.clone(),
         }
     }

components/style/lib.rs

@@ -124,7 +124,6 @@ pub mod sink;
 pub mod str;
 pub mod stylesheets;
 pub mod thread_state;
-mod tid;
 pub mod timer;
 pub mod traversal;
 #[macro_use]

components/style/traversal.rs

@@ -5,17 +5,15 @@
 //! Traversing the DOM tree; the bloom filter.
 
 use atomic_refcell::{AtomicRefCell, AtomicRefMut};
-use bloom::StyleBloom;
 use context::{SharedStyleContext, StyleContext};
 use data::{ElementData, StoredRestyleHint};
-use dom::{OpaqueNode, TElement, TNode};
+use dom::{TElement, TNode};
 use matching::{MatchMethods, StyleSharingResult};
 use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_SELF};
 use selector_parser::RestyleDamage;
 use selectors::Element;
 use selectors::matching::StyleRelations;
 use servo_config::opts;
-use std::cell::RefCell;
 use std::mem;
 use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering};
 use stylist::Stylist;
@@ -29,69 +27,6 @@ pub type Generation = u32;
 pub static STYLE_SHARING_CACHE_HITS: AtomicUsize = ATOMIC_USIZE_INIT;
 pub static STYLE_SHARING_CACHE_MISSES: AtomicUsize = ATOMIC_USIZE_INIT;
 
-thread_local!(
-    static STYLE_BLOOM: RefCell<Option<StyleBloom>> = RefCell::new(None));
-
-/// Returns the thread local bloom filter.
-///
-/// If one does not exist, a new one will be made for you. If it is out of date,
-/// it will be cleared and reused.
-pub fn take_thread_local_bloom_filter(context: &SharedStyleContext)
-                                      -> StyleBloom
-{
-    trace!("{} taking bf", ::tid::tid());
-
-    STYLE_BLOOM.with(|style_bloom| {
-        style_bloom.borrow_mut().take()
-            .unwrap_or_else(|| StyleBloom::new(context.generation))
-    })
-}
-
-pub fn put_thread_local_bloom_filter(bf: StyleBloom) {
-    trace!("[{}] putting bloom filter back", ::tid::tid());
-
-    STYLE_BLOOM.with(move |style_bloom| {
-        debug_assert!(style_bloom.borrow().is_none(),
-                     "Putting into a never-taken thread-local bloom filter");
-        *style_bloom.borrow_mut() = Some(bf);
-    })
-}
-
-/// Remove `element` from the bloom filter if it's the last element we inserted.
-///
-/// Restores the bloom filter if this is not the root of the reflow.
-///
-/// This is mostly useful for sequential traversal, where the element will
-/// always be the last one.
-pub fn remove_from_bloom_filter<E: TElement>(context: &SharedStyleContext,
-                                             root: OpaqueNode, element: E)
-{
-    trace!("[{}] remove_from_bloom_filter", ::tid::tid());
-
-    // We may have arrived to `reconstruct_flows` without entering in style
-    // recalc at all due to our optimizations, nor that it's up to date, so we
-    // can't ensure there's a bloom filter at all.
-    let bf = STYLE_BLOOM.with(|style_bloom| {
-        style_bloom.borrow_mut().take()
-    });
-
-    if let Some(mut bf) = bf {
-        if context.generation == bf.generation() {
-            bf.maybe_pop(element);
-
-            // If we're the root of the reflow, just get rid of the bloom
-            // filter.
-            //
-            // FIXME: We might want to just leave it in TLS? You don't do 4k
-            // allocations every day. Also, this just clears one thread's bloom
-            // filter, which is... not great?
-            if element.as_node().opaque() != root {
-                put_thread_local_bloom_filter(bf);
-            }
-        }
-    }
-}
-
 // NB: Keep this as small as possible, please!
 #[derive(Clone, Debug)]
 pub struct PerLevelTraversalData {
@@ -429,19 +364,17 @@ fn compute_style<E, D>(_traversal: &D,
           D: DomTraversal<E::ConcreteNode>,
 {
     let shared_context = context.shared;
-    let mut bf = take_thread_local_bloom_filter(shared_context);
     // Ensure the bloom filter is up to date.
-    let dom_depth = bf.insert_parents_recovering(element,
-                                                 traversal_data.current_dom_depth,
-                                                 shared_context.generation);
+    let dom_depth = context.thread_local.bloom_filter
+                           .insert_parents_recovering(element, traversal_data.current_dom_depth);
 
     // Update the dom depth with the up-to-date dom depth.
     //
     // Note that this is always the same than the pre-existing depth, but it can
     // change from unknown to known at this step.
     traversal_data.current_dom_depth = Some(dom_depth);
 
-    bf.assert_complete(element);
+    context.thread_local.bloom_filter.assert_complete(element);
 
     // Check to see whether we can share a style with someone.
     let sharing_result = if element.parent_element().is_none() {
@@ -461,7 +394,8 @@ fn compute_style<E, D>(_traversal: &D,
                 }
 
                 // Perform the CSS selector matching.
-                match_results = element.match_element(context, Some(bf.filter()));
+                let filter = context.thread_local.bloom_filter.filter();
+                match_results = element.match_element(context, Some(filter));
                 if match_results.primary_is_shareable() {
                     Some(element)
                 } else {
@@ -503,13 +437,6 @@ fn compute_style<E, D>(_traversal: &D,
         clear_descendant_data(element, &|e| unsafe { D::clear_element_data(&e) });
     }
 
-    // TODO(emilio): It's pointless to insert the element in the parallel
-    // traversal, but it may be worth todo it for sequential restyling. What we
-    // do now is trying to recover it which in that case is really cheap, so
-    // we'd save a few instructions, but probably not worth given the added
-    // complexity.
-    put_thread_local_bloom_filter(bf);
-
     // FIXME(bholley): Compute this accurately from the call to CalcStyleDifference.
     let inherited_styles_changed = true;