From 1090abae877bb6624e49b34540682a389e7daaac Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 25 Oct 2016 19:50:31 -0700 Subject: [PATCH] Don't traverse text nodes during styling. MozReview-Commit-ID: 6CtQMxbcLnF --- components/layout/traversal.rs | 35 +++++++++++++++++-- components/script/layout_wrapper.rs | 14 ++++++-- .../script_layout_interface/wrapper_traits.rs | 2 ++ components/style/dom.rs | 3 +- components/style/gecko/traversal.rs | 11 ++++-- .../style/properties/properties.mako.rs | 12 ------- components/style/traversal.rs | 26 +++++++------- 7 files changed, 72 insertions(+), 31 deletions(-) diff --git a/components/layout/traversal.rs b/components/layout/traversal.rs index b44ef4e17f8..36130aba4c4 100644 --- a/components/layout/traversal.rs +++ b/components/layout/traversal.rs @@ -18,8 +18,10 @@ use style::context::{LocalStyleContext, SharedStyleContext, StyleContext}; use style::data::NodeData; use style::dom::TNode; use style::selector_impl::ServoSelectorImpl; -use style::traversal::{DomTraversalContext, recalc_style_at, remove_from_bloom_filter}; +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::{LayoutNodeLayoutData, ThreadSafeLayoutNodeHelpers}; @@ -77,7 +79,36 @@ impl<'lc, N> DomTraversalContext for RecalcStyleAndConstructFlows<'lc> // done by the HTML parser. node.initialize_data(); - recalc_style_at::<_, _, Self>(&self.context, self.root, node) + if node.is_text_node() { + // FIXME(bholley): Stop doing this silly work to maintain broken bloom filter + // invariants. + // + // Longer version: The bloom filter is entirely busted for parallel traversal. Because + // parallel traversal is breadth-first, each sibling rejects the bloom filter set up + // by the previous sibling (which is valid for children, not siblings) and recreates + // it. Similarly, the fixup performed in the bottom-up traversal is useless, because + // threads perform flow construction up the parent chain until they find a parent with + // other unprocessed children, at which point they bail to the work queue and find a + // different node. + // + // Nevertheless, the remove_from_bloom_filter call at the end of flow construction + // asserts that the bloom filter is valid for the current node. This breaks when we + // stop calling recalc_style_at for text nodes, because the recursive chain of + // construct_flows_at calls is no longer necessarily rooted in a call that sets up the + // thread-local bloom filter for the leaf node. + // + // The bloom filter stuff is all going to be rewritten, so we just hackily duplicate + // the bloom filter manipulation from recalc_style_at to maintain invariants. + let parent = node.parent_node(); + debug_assert!(parent.unwrap().is_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) + } } fn process_postorder(&self, node: N) { diff --git a/components/script/layout_wrapper.rs b/components/script/layout_wrapper.rs index 0cc2e7f3c78..b5e1c5a4f60 100644 --- a/components/script/layout_wrapper.rs +++ b/components/script/layout_wrapper.rs @@ -763,6 +763,9 @@ impl<'ln> NodeInfo for ServoThreadSafeLayoutNode<'ln> { } fn is_text_node(&self) -> bool { + // It's unlikely that text nodes will ever be used to implement a + // pseudo-element, but the type system doesn't really enforce that, + // so we check to be safe. self.pseudo == PseudoElementType::Normal && self.node.is_text_node() } @@ -802,11 +805,18 @@ impl<'ln> ThreadSafeLayoutNode for ServoThreadSafeLayoutNode<'ln> { } fn style_for_text_node(&self) -> Arc { + // Text nodes get a copy of the parent style. Inheriting all non- + // inherited properties into the text node is odd from a CSS + // perspective, but makes fragment construction easier (by making + // properties like vertical-align on fragments have values that + // match the parent element). This is an implementation detail of + // Servo layout that is not central to how fragment construction + // works, but would be difficult to change. (Text node style is + // also not visible to script.) debug_assert!(self.is_text_node()); let parent = self.node.parent_node().unwrap(); let parent_data = parent.get_style_data().unwrap().borrow(); - let parent_style = &parent_data.current_styles().primary; - ComputedValues::style_for_child_text_node(parent_style) + parent_data.current_styles().primary.clone() } fn debug_id(self) -> usize { diff --git a/components/script_layout_interface/wrapper_traits.rs b/components/script_layout_interface/wrapper_traits.rs index 40fdf33847d..200aeaa03af 100644 --- a/components/script_layout_interface/wrapper_traits.rs +++ b/components/script_layout_interface/wrapper_traits.rs @@ -323,6 +323,7 @@ pub trait ThreadSafeLayoutNode: Clone + Copy + NodeInfo + PartialEq + Sized { /// element style is precomputed, not from general layout itself. #[inline] fn resolved_style(&self) -> Arc { + // FIXME(bholley): This should move to Element and lose the text node check. if self.is_text_node() { return self.style_for_text_node(); } @@ -338,6 +339,7 @@ pub trait ThreadSafeLayoutNode: Clone + Copy + NodeInfo + PartialEq + Sized { #[inline] fn selected_style(&self, _context: &SharedStyleContext) -> Arc { + // FIXME(bholley): This should move to Element and lose the text node check. if self.is_text_node() { return self.style_for_text_node(); } diff --git a/components/style/dom.rs b/components/style/dom.rs index c824457dbcb..aa5d0829001 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -268,7 +268,8 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre fn existing_style_for_restyle_damage<'a>(&'a self, current_computed_values: Option<&'a Arc>, pseudo: Option<&PseudoElement>) - -> Option<&'a <::ConcreteRestyleDamage as TRestyleDamage>::PreExistingComputedValues>; + -> Option<&'a <::ConcreteRestyleDamage as TRestyleDamage> + ::PreExistingComputedValues>; /// Properly marks nodes as dirty in response to restyle hints. fn note_restyle_hint>(&self, hint: RestyleHint) { diff --git a/components/style/gecko/traversal.rs b/components/style/gecko/traversal.rs index 61a9bd32de8..c9ddca69d73 100644 --- a/components/style/gecko/traversal.rs +++ b/components/style/gecko/traversal.rs @@ -5,7 +5,7 @@ use atomic_refcell::AtomicRefCell; use context::{LocalStyleContext, SharedStyleContext, StyleContext}; use data::NodeData; -use dom::OpaqueNode; +use dom::{NodeInfo, OpaqueNode, TNode}; use gecko::context::StandaloneStyleContext; use gecko::wrapper::GeckoNode; use std::mem; @@ -31,7 +31,14 @@ impl<'lc, 'ln> DomTraversalContext> for RecalcStyleOnly<'lc> { } fn process_preorder(&self, node: GeckoNode<'ln>) -> RestyleResult { - recalc_style_at::<_, _, Self>(&self.context, self.root, node) + if node.is_text_node() { + // Text nodes don't have children, so save the traversal algorithm + // the trouble of iterating the children. + RestyleResult::Stop + } else { + let el = node.as_element().unwrap(); + recalc_style_at::<_, _, Self>(&self.context, self.root, el) + } } fn process_postorder(&self, _: GeckoNode<'ln>) { diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 63e7b2fe5a4..40871299659 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1084,18 +1084,6 @@ impl ComputedValues { } } - pub fn style_for_child_text_node(parent: &Arc) -> Arc { - // Text nodes get a copy of the parent style. Inheriting all non- - // inherited properties into the text node is odd from a CSS - // perspective, but makes fragment construction easier (by making - // properties like vertical-align on fragments have values that - // match the parent element). This is an implementation detail of - // Servo layout that is not central to how fragment construction - // works, but would be difficult to change. (Text node style is - // also not visible to script.) - parent.clone() - } - pub fn initial_values() -> &'static Self { &*INITIAL_SERVO_VALUES } #[inline] diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 085ce8888c6..03405e59360 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -7,7 +7,7 @@ use atomic_refcell::AtomicRefCell; use context::{LocalStyleContext, SharedStyleContext, StyleContext}; use data::NodeData; -use dom::{OpaqueNode, StylingMode, TElement, TNode, UnsafeNode}; +use dom::{NodeInfo, OpaqueNode, StylingMode, TElement, TNode, UnsafeNode}; use matching::{ApplicableDeclarations, ElementMatchMethods, MatchMethods, StyleSharingResult}; use selectors::bloom::BloomFilter; use selectors::matching::StyleRelations; @@ -62,11 +62,11 @@ thread_local!( /// /// 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. -fn take_thread_local_bloom_filter(parent_node: Option, - root: OpaqueNode, - context: &SharedStyleContext) - -> Box - where N: TNode { +pub fn take_thread_local_bloom_filter(parent_node: Option, + root: OpaqueNode, + context: &SharedStyleContext) + -> Box + where N: TNode { STYLE_BLOOM.with(|style_bloom| { match (parent_node, style_bloom.borrow_mut().take()) { // Root node. Needs new bloom filter. @@ -98,8 +98,8 @@ fn take_thread_local_bloom_filter(parent_node: Option, }) } -fn put_thread_local_bloom_filter(bf: Box, unsafe_node: &UnsafeNode, - context: &SharedStyleContext) { +pub fn put_thread_local_bloom_filter(bf: Box, unsafe_node: &UnsafeNode, + context: &SharedStyleContext) { STYLE_BLOOM.with(move |style_bloom| { assert!(style_bloom.borrow().is_none(), "Putting into a never-taken thread-local bloom filter"); @@ -284,13 +284,15 @@ fn ensure_node_styled_internal<'a, N, C>(node: N, /// Calculates the style for a single node. #[inline] #[allow(unsafe_code)] -pub fn recalc_style_at<'a, N, C, D>(context: &'a C, +pub fn recalc_style_at<'a, E, C, D>(context: &'a C, root: OpaqueNode, - node: N) -> RestyleResult - where N: TNode, + element: E) -> RestyleResult + where E: TElement, C: StyleContext<'a>, - D: DomTraversalContext + D: DomTraversalContext { + let node = element.as_node(); + // Get the parent node. let parent_opt = match node.parent_node() { Some(parent) if parent.is_element() => Some(parent),