From 4c8383c38b60aad5c8bfb0c094518364e692428e Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 11 Dec 2013 19:58:27 -0800 Subject: [PATCH] layout: Reference count `ComputedValues` structures like Gecko does. This has no difference in CSS selector matching performance and results in a 31% speedup in constraint solving on the rainbow page. --- src/components/main/css/matching.rs | 15 +++--- src/components/main/css/node_style.rs | 10 ++-- src/components/main/css/node_util.rs | 20 ++------ src/components/main/layout/box.rs | 56 +++++++++++++++-------- src/components/main/layout/construct.rs | 5 +- src/components/main/layout/inline.rs | 7 +-- src/components/main/layout/layout_task.rs | 4 +- src/components/main/layout/util.rs | 2 +- 8 files changed, 66 insertions(+), 53 deletions(-) diff --git a/src/components/main/css/matching.rs b/src/components/main/css/matching.rs index 6de2d33cf80..cbe11e00010 100644 --- a/src/components/main/css/matching.rs +++ b/src/components/main/css/matching.rs @@ -8,7 +8,7 @@ use std::cell::Cell; use std::comm; use std::task; use std::vec; -use extra::arc::RWArc; +use extra::arc::{Arc, RWArc}; use css::node_style::StyledNode; use layout::incremental; @@ -85,13 +85,16 @@ impl MatchMethods for AbstractNode { fn cascade_node(&self, parent: Option>) { let parent_style = match parent { - Some(parent) => Some(parent.style()), + Some(ref parent) => Some(parent.style()), None => None }; let computed_values = unsafe { - cascade(self.borrow_layout_data_unchecked().as_ref().unwrap().applicable_declarations, - parent_style) + Arc::new(cascade(self.borrow_layout_data_unchecked() + .as_ref() + .unwrap() + .applicable_declarations, + parent_style.map(|parent_style| parent_style.get()))) }; match *self.mutate_layout_data().ptr { @@ -102,8 +105,8 @@ impl MatchMethods for AbstractNode { None => (), Some(ref previous_style) => { layout_data.restyle_damage = - Some(incremental::compute_damage(previous_style, - &computed_values).to_int()) + Some(incremental::compute_damage(previous_style.get(), + computed_values.get()).to_int()) } } *style = Some(computed_values) diff --git a/src/components/main/css/node_style.rs b/src/components/main/css/node_style.rs index 33fdb8d43b2..af9b927e171 100644 --- a/src/components/main/css/node_style.rs +++ b/src/components/main/css/node_style.rs @@ -7,22 +7,20 @@ use css::node_util::NodeUtil; use layout::incremental::RestyleDamage; +use extra::arc::Arc; use style::ComputedValues; use script::dom::node::{AbstractNode, LayoutView}; /// Node mixin providing `style` method that returns a `NodeStyle` pub trait StyledNode { - fn style(&self) -> &ComputedValues; + fn style<'a>(&'a self) -> &'a Arc; fn restyle_damage(&self) -> RestyleDamage; } impl StyledNode for AbstractNode { #[inline] - fn style(&self) -> &ComputedValues { - // FIXME(pcwalton): Is this assertion needed for memory safety? It's slow. - //assert!(self.is_element()); // Only elements can have styles - let results = self.get_css_select_results(); - results + fn style<'a>(&'a self) -> &'a Arc { + self.get_css_select_results() } fn restyle_damage(&self) -> RestyleDamage { diff --git a/src/components/main/css/node_util.rs b/src/components/main/css/node_util.rs index 5ca43c69243..84b7c6098d1 100644 --- a/src/components/main/css/node_util.rs +++ b/src/components/main/css/node_util.rs @@ -5,30 +5,28 @@ use layout::incremental::RestyleDamage; use layout::util::LayoutDataAccess; +use extra::arc::Arc; use std::cast; use style::ComputedValues; use script::dom::node::{AbstractNode, LayoutView}; use servo_util::tree::TreeNodeRef; -pub trait NodeUtil<'self> { - fn get_css_select_results(self) -> &'self ComputedValues; - fn set_css_select_results(self, decl: ComputedValues); +pub trait NodeUtil { + fn get_css_select_results<'a>(&'a self) -> &'a Arc; fn have_css_select_results(self) -> bool; fn get_restyle_damage(self) -> RestyleDamage; fn set_restyle_damage(self, damage: RestyleDamage); } -impl<'self> NodeUtil<'self> for AbstractNode { +impl NodeUtil for AbstractNode { /** * Provides the computed style for the given node. If CSS selector * Returns the style results for the given node. If CSS selector * matching has not yet been performed, fails. - * FIXME: This isn't completely memory safe since the style is - * stored in a box that can be overwritten */ #[inline] - fn get_css_select_results(self) -> &'self ComputedValues { + fn get_css_select_results<'a>(&'a self) -> &'a Arc { unsafe { cast::transmute_region(self.borrow_layout_data_unchecked() .as_ref() @@ -44,14 +42,6 @@ impl<'self> NodeUtil<'self> for AbstractNode { self.borrow_layout_data().ptr.as_ref().unwrap().style.is_some() } - /// Update the computed style of an HTML element with a style specified by CSS. - fn set_css_select_results(self, decl: ComputedValues) { - match *self.mutate_layout_data().ptr { - Some(ref mut data) => data.style = Some(decl), - _ => fail!("no layout data for this node"), - } - } - /// Get the description of how to account for recent style changes. /// This is a simple bitfield and fine to copy by value. fn get_restyle_damage(self) -> RestyleDamage { diff --git a/src/components/main/layout/box.rs b/src/components/main/layout/box.rs index 0280b646e58..2f41511009f 100644 --- a/src/components/main/layout/box.rs +++ b/src/components/main/layout/box.rs @@ -60,6 +60,9 @@ pub struct Box { /// The DOM node that this `Box` originates from. node: AbstractNode, + /// The CSS style of this box. + style: Arc, + /// The position of this box relative to its owning flow. position: Slot>, @@ -212,8 +215,27 @@ pub enum SplitBoxResult { impl Box { /// Constructs a new `Box` instance. pub fn new(node: AbstractNode, specific: SpecificBoxInfo) -> Box { + // Find the nearest ancestor element and take its style. (It should be either that node or + // its immediate parent.) + // + // FIXME(pcwalton): This is incorrect for non-inherited properties on anonymous boxes. For + // example: + // + //
+ //

Foo

+ // Bar + //

Baz

+ //
+ // + // An anonymous block box is generated around `Bar`, but it shouldn't inherit the border. + let mut nearest_ancestor_element = node; + while !nearest_ancestor_element.is_element() { + nearest_ancestor_element = node.parent_node().expect("no nearest element?!"); + } + Box { node: node, + style: (*nearest_ancestor_element.style()).clone(), position: Slot::init(Au::zero_rect()), border: Slot::init(Zero::zero()), padding: Slot::init(Zero::zero()), @@ -236,6 +258,7 @@ impl Box { pub fn transform(&self, size: Size2D, specific: SpecificBoxInfo) -> Box { Box { node: self.node, + style: self.style.clone(), position: Slot::init(Rect(self.position.get().origin, size)), border: Slot::init(self.border.get()), padding: Slot::init(self.padding.get()), @@ -376,7 +399,7 @@ impl Box { /// FIXME(pcwalton): Just replace with the clear type from the style module for speed? #[inline(always)] pub fn clear(&self) -> Option { - let style = self.node.style(); + let style = self.style(); match style.Box.clear { clear::none => None, clear::left => Some(ClearLeft), @@ -390,7 +413,7 @@ impl Box { /// FIXME(pcwalton): This should not be necessary; just make the font part of style sharable /// with the display list somehow. (Perhaps we should use an ARC.) pub fn font_style(&self) -> FontStyle { - let my_style = self.nearest_ancestor_element().style(); + let my_style = self.style(); debug!("(font style) start: {:?}", self.nearest_ancestor_element().type_id()); @@ -421,24 +444,23 @@ impl Box { } } - // FIXME(pcwalton): Why &'static??? Isn't that wildly unsafe? #[inline(always)] - pub fn style(&self) -> &'static ComputedValues { - self.node.style() + pub fn style<'a>(&'a self) -> &'a ComputedValues { + self.style.get() } /// Returns the text alignment of the computed style of the nearest ancestor-or-self `Element` /// node. pub fn text_align(&self) -> text_align::T { - self.nearest_ancestor_element().style().Text.text_align + self.style().Text.text_align } pub fn line_height(&self) -> line_height::T { - self.nearest_ancestor_element().style().Box.line_height + self.style().Box.line_height } pub fn vertical_align(&self) -> vertical_align::T { - self.nearest_ancestor_element().style().Box.vertical_align + self.style().Box.vertical_align } /// Returns the text decoration of the computed style of the nearest `Element` node @@ -458,13 +480,13 @@ impl Box { // FIXME: Implement correctly. let display_in_flow = true; - let position = element.style().Box.position; - let float = element.style().Box.float; + let position = element.style().get().Box.position; + let float = element.style().get().Box.float; let in_flow = (position == position::static_) && (float == float::none) && display_in_flow; - let text_decoration = element.style().Text.text_decoration; + let text_decoration = element.style().get().Text.text_decoration; if text_decoration == text_decoration::none && in_flow { match element.parent_node() { @@ -524,9 +546,7 @@ impl Box { // needed. We could use display list optimization to clean this up, but it still seems // inefficient. What we really want is something like "nearest ancestor element that // doesn't have a box". - let nearest_ancestor_element = self.nearest_ancestor_element(); - - let style = nearest_ancestor_element.style(); + let style = self.style(); let background_color = style.resolve_color(style.Background.background_color); if !background_color.alpha.approx_eq(&0.0) { list.with_mut_ref(|list| { @@ -613,7 +633,7 @@ impl Box { box_bounds, absolute_box_bounds, self.debug_str()); debug!("Box::build_display_list: dirty={}, offset={}", *dirty, *offset); - if self.nearest_ancestor_element().style().Box.visibility != visibility::visible { + if self.style().Box.visibility != visibility::visible { return; } @@ -642,9 +662,7 @@ impl Box { list.append_item(ClipDisplayItemClass(item)); } - - let nearest_ancestor_element = self.nearest_ancestor_element(); - let color = nearest_ancestor_element.style().Color.color.to_gfx_color(); + let color = self.style().Color.color.to_gfx_color(); // Create the text box. do list.with_mut_ref |list| { @@ -991,7 +1009,7 @@ impl Box { /// Returns true if the contents should be clipped (i.e. if `overflow` is `hidden`). pub fn needs_clip(&self) -> bool { - self.node.style().Box.overflow == overflow::hidden + self.style().Box.overflow == overflow::hidden } /// Returns a debugging string describing this box. diff --git a/src/components/main/layout/construct.rs b/src/components/main/layout/construct.rs index d447d5775d3..25a642bdace 100644 --- a/src/components/main/layout/construct.rs +++ b/src/components/main/layout/construct.rs @@ -472,7 +472,10 @@ impl<'self> PostorderNodeMutTraversal for FlowConstructor<'self> { fn process(&mut self, node: AbstractNode) -> bool { // Get the `display` property for this node, and determine whether this node is floated. let (display, float) = match node.type_id() { - ElementNodeTypeId(_) => (node.style().Box.display, node.style().Box.float), + ElementNodeTypeId(_) => { + let style = node.style().get(); + (style.Box.display, style.Box.float) + } TextNodeTypeId => (display::inline, float::none), CommentNodeTypeId | DoctypeNodeTypeId | diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index 12414571256..a69c1ec1d33 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -589,7 +589,7 @@ impl InlineFlow { // not from the style of the first box child. let linebox_align = if line.range.begin() < boxes.len() { let first_box = &boxes[line.range.begin()]; - first_box.nearest_ancestor_element().style().Text.text_align + first_box.style().Text.text_align } else { // Nothing to lay out, so assume left alignment. text_align::left @@ -804,8 +804,9 @@ impl Flow for InlineFlow { // content area. But for now we assume it's zero. let parent_text_bottom = Au::new(0); - let parent = cur_box.node.parent_node(); - let font_size = parent.unwrap().style().Font.font_size; + let parent = cur_box.node.parent_node().unwrap(); + let parent_style = parent.style(); + let font_size = parent_style.get().Font.font_size; parent_text_top = font_size; // Calculate a relative offset from the baseline. diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs index 0ccdcfd5db0..b888f5b1935 100644 --- a/src/components/main/layout/layout_task.rs +++ b/src/components/main/layout/layout_task.rs @@ -477,8 +477,8 @@ impl LayoutTask { for child in node.traverse_preorder() { if child.type_id() == ElementNodeTypeId(HTMLHtmlElementTypeId) || child.type_id() == ElementNodeTypeId(HTMLBodyElementTypeId) { - let element_bg_color = child.style().resolve_color( - child.style().Background.background_color + let element_bg_color = child.style().get().resolve_color( + child.style().get().Background.background_color ).to_gfx_color(); match element_bg_color { color::rgba(0., 0., 0., 0.) => {} diff --git a/src/components/main/layout/util.rs b/src/components/main/layout/util.rs index 63594026e04..9fb3257de24 100644 --- a/src/components/main/layout/util.rs +++ b/src/components/main/layout/util.rs @@ -127,7 +127,7 @@ pub struct LayoutData { applicable_declarations: ~[Arc<~[PropertyDeclaration]>], /// The results of CSS styling for this node. - style: Option, + style: Option>, /// Description of how to account for recent style changes. restyle_damage: Option,