From b6e948dc5d5c8e50177b647ab641e39db51b739f Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 21 Dec 2016 11:41:58 -0800 Subject: [PATCH 1/5] Hoist bloom filter into scoped TLS and simplify code. --- components/layout/traversal.rs | 15 +++--- components/layout_thread/lib.rs | 2 +- components/style/bloom.rs | 17 ++----- components/style/context.rs | 3 ++ components/style/lib.rs | 1 - components/style/tid.rs | 26 ---------- components/style/traversal.rs | 85 +++------------------------------ 7 files changed, 19 insertions(+), 130 deletions(-) delete mode 100644 components/style/tid.rs diff --git a/components/layout/traversal.rs b/components/layout/traversal.rs index be4525cd17a..811074bb083 100644 --- a/components/layout/traversal.rs +++ b/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 DomTraversal 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(context: &LayoutContext<'a>, - _thread_local: &ScopedThreadLocalLayoutContext, - root: OpaqueNode, node: N) + thread_local: &mut ScopedThreadLocalLayoutContext, + 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); } } diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 524b325dcec..b67e34b6e3c 100644 --- a/components/layout_thread/lib.rs +++ b/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 = &, - - /// 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, } } @@ -38,10 +33,6 @@ impl StyleBloom { &*self.filter } - pub fn generation(&self) -> u32 { - self.generation - } - pub fn maybe_pop(&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(&mut self, element: E, - element_depth: Option, - generation: u32) + element_depth: Option) -> 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); } diff --git a/components/style/context.rs b/components/style/context.rs index a1695bb7e8e..42f03098c65 100644 --- a/components/style/context.rs +++ b/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 { pub style_sharing_candidate_cache: StyleSharingCandidateCache, + 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, @@ -86,6 +88,7 @@ impl ThreadLocalStyleContext { 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(), } } diff --git a/components/style/lib.rs b/components/style/lib.rs index 3481a03c4fe..d425e1e5842 100644 --- a/components/style/lib.rs +++ b/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] diff --git a/components/style/tid.rs b/components/style/tid.rs deleted file mode 100644 index a60c321a984..00000000000 --- a/components/style/tid.rs +++ /dev/null @@ -1,26 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -use std::cell::RefCell; -use std::rc::Rc; -use std::sync::atomic::{ATOMIC_USIZE_INIT, AtomicUsize, Ordering}; - -static NEXT_TID: AtomicUsize = ATOMIC_USIZE_INIT; - -thread_local!(static TASK_LOCAL_TID: Rc>> = Rc::new(RefCell::new(None))); - -/// Every thread gets one, that's unique. -pub fn tid() -> usize { - TASK_LOCAL_TID.with(|ref k| { - let ret = - match *k.borrow() { - None => NEXT_TID.fetch_add(1, Ordering::SeqCst), - Some(x) => x, - }; - - *k.borrow_mut() = Some(ret); - - ret - }) -} diff --git a/components/style/traversal.rs b/components/style/traversal.rs index f6fd16429a9..9534a087695 100644 --- a/components/style/traversal.rs +++ b/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> = 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(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,11 +364,9 @@ fn compute_style(_traversal: &D, D: DomTraversal, { 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. // @@ -441,7 +374,7 @@ fn compute_style(_traversal: &D, // 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(_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(_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; From 96dc0d1eacc54a23635c9cf9c8a982bdbed7dac7 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 21 Dec 2016 12:11:27 -0800 Subject: [PATCH 2/5] Hoist SendElement into dom.rs, label construction as unsafe, and add corresponding SendNode. --- components/style/dom.rs | 36 ++++++++++++++++++++++++++++++++++++ components/style/matching.rs | 19 ++++++------------- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/components/style/dom.rs b/components/style/dom.rs index 3ce4a716e68..465590d03f5 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -16,6 +16,7 @@ use selector_parser::{ElementExt, PreExistingComputedValues, PseudoElement}; use sink::Push; use std::fmt; use std::fmt::Debug; +use std::ops::Deref; use std::sync::Arc; use stylist::ApplicableDeclarationBlock; @@ -262,3 +263,38 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre /// native anonymous content can opt out of this style fixup.) fn skip_root_and_item_based_display_fixup(&self) -> bool; } + +/// TNode and TElement aren't Send because we want to be careful and explicit +/// about our parallel traversal. However, there are certain situations +/// (including but not limited to the traversal) where we need to send DOM +/// objects to other threads. + +#[derive(Debug, PartialEq)] +pub struct SendNode(N); +unsafe impl Send for SendNode {} +impl SendNode { + pub unsafe fn new(node: N) -> Self { + SendNode(node) + } +} +impl Deref for SendNode { + type Target = N; + fn deref(&self) -> &N { + &self.0 + } +} + +#[derive(Debug, PartialEq)] +pub struct SendElement(E); +unsafe impl Send for SendElement {} +impl SendElement { + pub unsafe fn new(el: E) -> Self { + SendElement(el) + } +} +impl Deref for SendElement { + type Target = E; + fn deref(&self) -> &E { + &self.0 + } +} diff --git a/components/style/matching.rs b/components/style/matching.rs index a1580e404b4..ff89dd07003 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -13,7 +13,7 @@ use cache::LRUCache; use cascade_info::CascadeInfo; use context::{SharedStyleContext, StyleContext}; use data::{ComputedStyle, ElementData, ElementStyles, PseudoStyles}; -use dom::{TElement, TNode}; +use dom::{SendElement, TElement, TNode}; use properties::{CascadeFlags, ComputedValues, SHAREABLE, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, cascade}; use properties::longhands::display::computed_value as display; use rule_tree::StrongRuleNode; @@ -65,19 +65,13 @@ impl MatchResults { } } -// TElement isn't Send because we want to be careful and explicit about our -// parallel traversal, but we need the candidates to be Send so that we can stick -// them in ScopedTLS. -#[derive(Debug, PartialEq)] -struct SendElement(pub E); -unsafe impl Send for SendElement {} - /// Information regarding a candidate. /// /// TODO: We can stick a lot more info here. #[derive(Debug)] struct StyleSharingCandidate { - /// The element. + /// The element. We use SendElement here so that the cache may live in + /// ScopedTLS. element: SendElement, /// The cached common style affecting attribute info. common_style_affecting_attributes: Option, @@ -353,7 +347,7 @@ impl StyleSharingCandidateCache { element, parent); self.cache.insert(StyleSharingCandidate { - element: SendElement(*element), + element: unsafe { SendElement::new(*element) }, common_style_affecting_attributes: None, class_attributes: None, }, ()); @@ -501,9 +495,8 @@ trait PrivateMatchMethods: TElement { shared_context: &SharedStyleContext, candidate: &mut StyleSharingCandidate) -> Result { - let candidate_element = candidate.element.0; - element_matches_candidate(self, candidate, &candidate_element, - shared_context) + let candidate_element = *candidate.element; + element_matches_candidate(self, candidate, &candidate_element, shared_context) } } From b3cb786c8108e4c2277cf452820f7ac6cd03501e Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 21 Dec 2016 12:36:55 -0800 Subject: [PATCH 3/5] Stop using UnsafeNode in the bloom filter. --- components/style/bloom.rs | 71 +++++++++++++------------------------ components/style/context.rs | 2 +- 2 files changed, 25 insertions(+), 48 deletions(-) diff --git a/components/style/bloom.rs b/components/style/bloom.rs index 143e8875b91..e2f45bb6233 100644 --- a/components/style/bloom.rs +++ b/components/style/bloom.rs @@ -5,23 +5,19 @@ //! The style bloom filter is used as an optimization when matching deep //! descendant selectors. -use dom::{TNode, TElement, UnsafeNode}; +use dom::{SendElement, TElement}; use matching::MatchMethods; use selectors::bloom::BloomFilter; -pub struct StyleBloom { +pub struct StyleBloom { /// The bloom filter per se. filter: Box, - /// The stack of elements that this bloom filter contains. These unsafe - /// nodes are guaranteed to be elements. - /// - /// 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, + /// The stack of elements that this bloom filter contains. + elements: Vec>, } -impl StyleBloom { +impl StyleBloom { pub fn new() -> Self { StyleBloom { filter: Box::new(BloomFilter::new()), @@ -33,39 +29,27 @@ impl StyleBloom { &*self.filter } - pub fn maybe_pop(&mut self, element: E) - where E: TElement + MatchMethods - { - if self.elements.last() == Some(&element.as_node().to_unsafe()) { - self.pop::().unwrap(); + pub fn maybe_pop(&mut self, element: E) { + if self.elements.last().map(|el| **el) == Some(element) { + self.pop().unwrap(); } } /// Push an element to the bloom filter, knowing that it's a child of the /// last element parent. - pub fn push(&mut self, element: E) - where E: TElement + MatchMethods, - { + pub fn push(&mut self, element: E) { if cfg!(debug_assertions) { if self.elements.is_empty() { assert!(element.parent_element().is_none()); } } element.insert_into_bloom_filter(&mut *self.filter); - self.elements.push(element.as_node().to_unsafe()); + self.elements.push(unsafe { SendElement::new(element) }); } /// Pop the last element in the bloom filter and return it. - fn pop(&mut self) -> Option - where E: TElement + MatchMethods, - { - let popped = - self.elements.pop().map(|unsafe_node| { - let parent = unsafe { - E::ConcreteNode::from_unsafe(&unsafe_node) - }; - parent.as_element().unwrap() - }); + fn pop(&mut self) -> Option { + let popped = self.elements.pop().map(|el| *el); if let Some(popped) = popped { popped.remove_from_bloom_filter(&mut self.filter); } @@ -78,14 +62,12 @@ impl StyleBloom { self.elements.clear(); } - fn rebuild(&mut self, mut element: E) -> usize - where E: TElement + MatchMethods, - { + fn rebuild(&mut self, mut element: E) -> usize { self.clear(); while let Some(parent) = element.parent_element() { parent.insert_into_bloom_filter(&mut *self.filter); - self.elements.push(parent.as_node().to_unsafe()); + self.elements.push(unsafe { SendElement::new(parent) }); element = parent; } @@ -96,14 +78,11 @@ impl StyleBloom { /// In debug builds, asserts that all the parents of `element` are in the /// bloom filter. - pub fn assert_complete(&self, mut element: E) - where E: TElement, - { + pub fn assert_complete(&self, mut element: E) { if cfg!(debug_assertions) { let mut checked = 0; while let Some(parent) = element.parent_element() { - assert_eq!(parent.as_node().to_unsafe(), - self.elements[self.elements.len() - 1 - checked]); + assert_eq!(parent, *self.elements[self.elements.len() - 1 - checked]); element = parent; checked += 1; } @@ -118,11 +97,10 @@ impl StyleBloom { /// provided always rebuilds the filter from scratch. /// /// Returns the new bloom filter depth. - pub fn insert_parents_recovering(&mut self, - element: E, - element_depth: Option) - -> usize - where E: TElement, + pub fn insert_parents_recovering(&mut self, + element: E, + element_depth: Option) + -> usize { // Easy case, we're in a different restyle, or we're empty. if self.elements.is_empty() { @@ -138,8 +116,7 @@ impl StyleBloom { } }; - let unsafe_parent = parent_element.as_node().to_unsafe(); - if self.elements.last() == Some(&unsafe_parent) { + if self.elements.last().map(|el| **el) == Some(parent_element) { // Ta da, cache hit, we're all done. return self.elements.len(); } @@ -171,7 +148,7 @@ impl StyleBloom { // If the filter represents an element too deep in the dom, we need to // pop ancestors. while current_depth > element_depth - 1 { - self.pop::().expect("Emilio is bad at math"); + self.pop().expect("Emilio is bad at math"); current_depth -= 1; } @@ -208,9 +185,9 @@ impl StyleBloom { // off the document (such as scrollbars) as a separate subtree from the // document root. Thus it's possible with Gecko that we do not find any // common ancestor. - while *self.elements.last().unwrap() != common_parent.as_node().to_unsafe() { + while **self.elements.last().unwrap() != common_parent { parents_to_insert.push(common_parent); - self.pop::().unwrap(); + self.pop().unwrap(); common_parent = match common_parent.parent_element() { Some(parent) => parent, None => { diff --git a/components/style/context.rs b/components/style/context.rs index 42f03098c65..09a4c28e1a2 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -78,7 +78,7 @@ pub struct SharedStyleContext { pub struct ThreadLocalStyleContext { pub style_sharing_candidate_cache: StyleSharingCandidateCache, - pub bloom_filter: StyleBloom, + 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, From ea6a61af594c8795ff009d00e77363fe0ae855cc Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 21 Dec 2016 12:50:45 -0800 Subject: [PATCH 4/5] Stop using UnsafeNode in the parallel traversal. \o/ \o/ \o/ --- components/style/dom.rs | 2 +- components/style/parallel.rs | 46 ++++++++++++++++++++++-------------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/components/style/dom.rs b/components/style/dom.rs index 465590d03f5..e9095ec0c9f 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -269,7 +269,7 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre /// (including but not limited to the traversal) where we need to send DOM /// objects to other threads. -#[derive(Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub struct SendNode(N); unsafe impl Send for SendNode {} impl SendNode { diff --git a/components/style/parallel.rs b/components/style/parallel.rs index 80652bfa6b8..badd0b9f40a 100644 --- a/components/style/parallel.rs +++ b/components/style/parallel.rs @@ -4,9 +4,23 @@ //! Implements parallel traversal over the DOM tree. //! -//! This code is highly unsafe. Keep this file small and easy to audit. +//! This traversal is based on Rayon, and therefore its safety is largely +//! verified by the type system. +//! +//! The primary trickiness and fine print for the above relates to the +//! thread safety of the DOM nodes themselves. Accessing a DOM element +//! concurrently on multiple threads is actually mostly "safe", since all +//! the mutable state is protected by an AtomicRefCell, and so we'll +//! generally panic if something goes wrong. Still, we try to to enforce our +//! thread invariants at compile time whenever possible. As such, TNode and +//! TElement are not Send, so ordinary style system code cannot accidentally +//! share them with other threads. In the parallel traversal, we explicitly +//! invoke |unsafe { SendNode::new(n) }| to put nodes in containers that may +//! be sent to other threads. This occurs in only a handful of places and is +//! easy to grep for. At the time of this writing, there is no other unsafe +//! code in the parallel traversal. -use dom::{OpaqueNode, TElement, TNode, UnsafeNode}; +use dom::{OpaqueNode, SendNode, TElement, TNode}; use rayon; use scoped_tls::ScopedTLS; use servo_config::opts; @@ -16,6 +30,7 @@ use traversal::{STYLE_SHARING_CACHE_HITS, STYLE_SHARING_CACHE_MISSES}; pub const CHUNK_SIZE: usize = 64; +#[allow(unsafe_code)] pub fn traverse_dom(traversal: &D, root: N::ConcreteElement, known_root_dom_depth: Option, @@ -37,12 +52,12 @@ pub fn traverse_dom(traversal: &D, let mut children = vec![]; for kid in root.as_node().children() { if kid.as_element().map_or(false, |el| el.get_data().is_none()) { - children.push(kid.to_unsafe()); + children.push(unsafe { SendNode::new(kid) }); } } (children, known_root_dom_depth.map(|x| x + 1)) } else { - (vec![root.as_node().to_unsafe()], known_root_dom_depth) + (vec![unsafe { SendNode::new(root.as_node()) }], known_root_dom_depth) }; let traversal_data = PerLevelTraversalData { @@ -70,13 +85,13 @@ pub fn traverse_dom(traversal: &D, /// A parallel top-down DOM traversal. #[inline(always)] #[allow(unsafe_code)] -fn top_down_dom<'a, 'scope, N, D>(unsafe_nodes: &'a [UnsafeNode], +fn top_down_dom<'a, 'scope, N, D>(nodes: &'a [SendNode], root: OpaqueNode, mut traversal_data: PerLevelTraversalData, scope: &'a rayon::Scope<'scope>, traversal: &'scope D, tls: &'scope ScopedTLS<'scope, D::ThreadLocalContext>) - where N: TNode, + where N: TNode + 'scope, D: DomTraversal, { let mut discovered_child_nodes = vec![]; @@ -85,17 +100,15 @@ fn top_down_dom<'a, 'scope, N, D>(unsafe_nodes: &'a [UnsafeNode], // potentially traversing a child on this thread. let mut tlc = tls.ensure(|| traversal.create_thread_local_context()); - for unsafe_node in unsafe_nodes { - // Get a real layout node. - let node = unsafe { N::from_unsafe(&unsafe_node) }; - + for n in nodes { // Perform the appropriate traversal. + let node = **n; let mut children_to_process = 0isize; traversal.process_preorder(&mut traversal_data, &mut *tlc, node); if let Some(el) = node.as_element() { D::traverse_children(el, |kid| { children_to_process += 1; - discovered_child_nodes.push(kid.to_unsafe()) + discovered_child_nodes.push(unsafe { SendNode::new(kid) }) }); } @@ -104,7 +117,7 @@ fn top_down_dom<'a, 'scope, N, D>(unsafe_nodes: &'a [UnsafeNode], if D::needs_postorder_traversal() { if children_to_process == 0 { // If there were no more children, start walking back up. - bottom_up_dom(traversal, &mut *tlc, root, *unsafe_node) + bottom_up_dom(traversal, &mut *tlc, root, node) } else { // Otherwise record the number of children to process when the // time comes. @@ -121,12 +134,12 @@ fn top_down_dom<'a, 'scope, N, D>(unsafe_nodes: &'a [UnsafeNode], traverse_nodes(discovered_child_nodes, root, traversal_data, scope, traversal, tls); } -fn traverse_nodes<'a, 'scope, N, D>(nodes: Vec, root: OpaqueNode, +fn traverse_nodes<'a, 'scope, N, D>(nodes: Vec>, root: OpaqueNode, traversal_data: PerLevelTraversalData, scope: &'a rayon::Scope<'scope>, traversal: &'scope D, tls: &'scope ScopedTLS<'scope, D::ThreadLocalContext>) - where N: TNode, + where N: TNode + 'scope, D: DomTraversal, { if nodes.is_empty() { @@ -163,16 +176,13 @@ fn traverse_nodes<'a, 'scope, N, D>(nodes: Vec, root: OpaqueNode, /// /// The only communication between siblings is that they both /// fetch-and-subtract the parent's children count. -#[allow(unsafe_code)] fn bottom_up_dom(traversal: &D, thread_local: &mut D::ThreadLocalContext, root: OpaqueNode, - unsafe_node: UnsafeNode) + mut node: N) where N: TNode, D: DomTraversal { - // Get a real layout node. - let mut node = unsafe { N::from_unsafe(&unsafe_node) }; loop { // Perform the appropriate operation. traversal.process_postorder(thread_local, node); From 940cda1d15203063af00a5763e5ac090ddbd9c5a Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 22 Dec 2016 10:40:27 -0800 Subject: [PATCH 5/5] Remove generation, remove filter pop, and add size tests. --- components/layout/traversal.rs | 4 +--- components/layout_thread/lib.rs | 1 - components/script/test.rs | 10 +++++++++- components/style/context.rs | 4 ---- components/style/gecko/data.rs | 10 ---------- components/style/traversal.rs | 4 ---- ports/geckolib/glue.rs | 1 - tests/unit/script/size_of.rs | 4 ++++ 8 files changed, 14 insertions(+), 24 deletions(-) diff --git a/components/layout/traversal.rs b/components/layout/traversal.rs index 811074bb083..b3672ecf32c 100644 --- a/components/layout/traversal.rs +++ b/components/layout/traversal.rs @@ -112,7 +112,7 @@ pub trait PostorderNodeMutTraversal(context: &LayoutContext<'a>, - thread_local: &mut ScopedThreadLocalLayoutContext, + _thread_local: &mut ScopedThreadLocalLayoutContext, node: N) where N: LayoutNode, { @@ -139,8 +139,6 @@ fn construct_flows_at<'a, N>(context: &LayoutContext<'a>, if let Some(el) = node.as_element() { el.mutate_data().unwrap().persist(); unsafe { el.unset_dirty_descendants(); } - - thread_local.style_context.bloom_filter.maybe_pop(el); } } diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index b67e34b6e3c..f9f4c371a94 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -520,7 +520,6 @@ impl LayoutThread { viewport_size: self.viewport_size.clone(), screen_size_changed: screen_size_changed, stylist: rw_data.stylist.clone(), - generation: self.generation, goal: goal, running_animations: self.running_animations.clone(), expired_animations: self.expired_animations.clone(), diff --git a/components/script/test.rs b/components/script/test.rs index ca00b200788..31353b5ffa3 100644 --- a/components/script/test.rs +++ b/components/script/test.rs @@ -19,7 +19,7 @@ pub mod size_of { use dom::htmlspanelement::HTMLSpanElement; use dom::node::Node; use dom::text::Text; - use layout_wrapper::ServoThreadSafeLayoutNode; + use layout_wrapper::{ServoLayoutElement, ServoLayoutNode, ServoThreadSafeLayoutNode}; use std::mem::size_of; pub fn CharacterData() -> usize { @@ -50,6 +50,14 @@ pub mod size_of { size_of::() } + pub fn SendElement() -> usize { + size_of::<::style::dom::SendElement>() + } + + pub fn SendNode() -> usize { + size_of::<::style::dom::SendNode>() + } + pub fn ServoThreadSafeLayoutNode() -> usize { size_of::() } diff --git a/components/style/context.rs b/components/style/context.rs index 09a4c28e1a2..f649f64afbf 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -49,10 +49,6 @@ pub struct SharedStyleContext { /// The CSS selector stylist. pub stylist: Arc, - /// Starts at zero, and increased by one every time a layout completes. - /// This can be used to easily check for invalid stale data. - pub generation: u32, - /// Why is this reflow occurring pub goal: ReflowGoal, diff --git a/components/style/gecko/data.rs b/components/style/gecko/data.rs index 2b7cfc54a35..da334bfa4bf 100644 --- a/components/style/gecko/data.rs +++ b/components/style/gecko/data.rs @@ -25,9 +25,6 @@ pub struct PerDocumentStyleDataImpl { /// Rule processor. pub stylist: Arc, - /// Last restyle generation. - pub last_restyle_generation: u32, - /// List of stylesheets, mirrored from Gecko. pub stylesheets: Vec>, @@ -67,7 +64,6 @@ impl PerDocumentStyleData { PerDocumentStyleData(AtomicRefCell::new(PerDocumentStyleDataImpl { stylist: Arc::new(Stylist::new(device)), - last_restyle_generation: 0, stylesheets: vec![], stylesheets_changed: true, new_animations_sender: new_anims_sender, @@ -105,12 +101,6 @@ impl PerDocumentStyleDataImpl { self.stylesheets_changed = false; } } - - pub fn next_generation(&mut self) -> u32 { - self.last_restyle_generation = - self.last_restyle_generation.wrapping_add(1); - self.last_restyle_generation - } } unsafe impl HasFFI for PerDocumentStyleData { diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 9534a087695..a9fd8c57a4d 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -18,10 +18,6 @@ use std::mem; use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering}; use stylist::Stylist; -/// Every time we do another layout, the old bloom filters are invalid. This is -/// detected by ticking a generation number every layout. -pub type Generation = u32; - /// Style sharing candidate cache stats. These are only used when /// `-Z style-sharing-stats` is given. pub static STYLE_SHARING_CACHE_HITS: AtomicUsize = ATOMIC_USIZE_INIT; diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index bab3a36f470..b028065f48a 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -101,7 +101,6 @@ fn create_shared_context(mut per_doc_data: &mut AtomicRefMut