From 26b39241f9f5d6c6756b33be3cea9ac9bc0204ae Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Sat, 9 Sep 2017 04:31:25 -0400 Subject: [PATCH 1/2] Box gradients and rects in Image. r=xidorn Gradients and rects are rare, and large. Image is much smaller with them boxed. This is part of of the fix for Gecko bug 1397614 --- components/style/gecko/conversions.rs | 10 ++-- components/style/values/generics/image.rs | 54 +++++++++++++++++++--- components/style/values/specified/image.rs | 4 +- tests/unit/stylo/size_of.rs | 10 ++++ 4 files changed, 65 insertions(+), 13 deletions(-) diff --git a/components/style/gecko/conversions.rs b/components/style/gecko/conversions.rs index ee13e6fffe7..bd8c17f132e 100644 --- a/components/style/gecko/conversions.rs +++ b/components/style/gecko/conversions.rs @@ -151,8 +151,8 @@ impl nsStyleImage { /// Set a given Servo `Image` value into this `nsStyleImage`. pub fn set(&mut self, image: Image) { match image { - GenericImage::Gradient(gradient) => { - self.set_gradient(gradient) + GenericImage::Gradient(boxed_gradient) => { + self.set_gradient(*boxed_gradient) }, GenericImage::Url(ref url) => { unsafe { @@ -399,7 +399,7 @@ impl nsStyleImage { NumberOrPercentage::from_gecko_style_coord(&rect.data_at(2)), NumberOrPercentage::from_gecko_style_coord(&rect.data_at(3))) { (Some(top), Some(right), Some(bottom), Some(left)) => - Some(GenericImage::Rect(MozImageRect { url, top, right, bottom, left } )), + Some(GenericImage::Rect(Box::new(MozImageRect { url, top, right, bottom, left } ))), _ => { debug_assert!(false, "mCropRect could not convert to NumberOrPercentage"); None @@ -428,7 +428,7 @@ impl nsStyleImage { url } - unsafe fn get_gradient(self: &nsStyleImage) -> Gradient { + unsafe fn get_gradient(self: &nsStyleImage) -> Box { use gecko::values::convert_nscolor_to_rgba; use gecko_bindings::bindings::Gecko_GetGradientImageValue; use gecko_bindings::structs::{NS_STYLE_GRADIENT_SHAPE_CIRCULAR, NS_STYLE_GRADIENT_SHAPE_ELLIPTICAL}; @@ -581,7 +581,7 @@ impl nsStyleImage { CompatMode::Modern }; - Gradient { items, repeating: gecko_gradient.mRepeating, kind, compat_mode } + Box::new(Gradient { items, repeating: gecko_gradient.mRepeating, kind, compat_mode }) } } diff --git a/components/style/values/generics/image.rs b/components/style/values/generics/image.rs index 9c58569dd0b..4f42e459036 100644 --- a/components/style/values/generics/image.rs +++ b/components/style/values/generics/image.rs @@ -11,20 +11,21 @@ use cssparser::serialize_identifier; use custom_properties::SpecifiedValue; use std::fmt; use style_traits::ToCss; -use values::computed::ComputedValueAsSpecified; +use values::computed::{ComputedValueAsSpecified, Context, ToComputedValue}; /// An [image]. /// /// [image]: https://drafts.csswg.org/css-images/#image-values -#[derive(Clone, PartialEq, ToComputedValue)] +#[derive(Clone, PartialEq)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub enum Image { /// A `` image. Url(ImageUrl), - /// A `` image. - Gradient(Gradient), - /// A `-moz-image-rect` image - Rect(MozImageRect), + /// A `` image. Gradients are rather large, and not nearly as + /// common as urls, so we box them here to keep the size of this enum sane. + Gradient(Box), + /// A `-moz-image-rect` image. Also fairly large and rare. + Rect(Box), /// A `-moz-element(# )` Element(Atom), /// A paint worklet image. @@ -33,6 +34,47 @@ pub enum Image { PaintWorklet(PaintWorklet), } +// Can't just use derive(ToComputedValue) on Image, because when trying to do +// "impl ToComputedValue for Box" the Rust compiler complains that +// "impl ToComputedValue for T where T: ComputedValueAsSpecified + Clone" +// aleady implements ToComputedValue for std::boxed::Box<_> and hence we have +// conflicting implementations. +impl ToComputedValue for Image { + type ComputedValue = Image<::ComputedValue, + ::ComputedValue, + ::ComputedValue>; + + #[inline] + fn to_computed_value(&self, context: &Context) -> Self::ComputedValue { + match *self { + Image::Url(ref url) => Image::Url(url.to_computed_value(context)), + Image::Gradient(ref gradient) => + Image::Gradient(Box::new(gradient.to_computed_value(context))), + Image::Rect(ref rect) => Image::Rect(Box::new(rect.to_computed_value(context))), + Image::Element(ref atom) => Image::Element(atom.to_computed_value(context)), + #[cfg(feature = "servo")] + Image::PaintWorklet(ref worklet) => Image::PaintWorklet(worklet.to_computed_value(context)), + } + } + + #[inline] + fn from_computed_value(computed: &Self::ComputedValue) -> Self { + match *computed { + Image::Url(ref url) => Image::Url(ImageUrl::from_computed_value(url)), + Image::Gradient(ref boxed_gradient) => + Image::Gradient(Box::new(Gradient::from_computed_value(&*boxed_gradient))), + Image::Rect(ref boxed_rect) => + Image::Rect(Box::new(MozImageRect::from_computed_value(&*boxed_rect))), + Image::Element(ref atom) => Image::Element(Atom::from_computed_value(atom)), + #[cfg(feature = "servo")] + Image::PaintWorklet(ref worklet) => + Image::PaintWorklet(PaintWorklet::from_computed_value(worklet)), + } + } +} + /// A CSS gradient. /// https://drafts.csswg.org/css-images/#gradients #[cfg_attr(feature = "servo", derive(HeapSizeOf))] diff --git a/components/style/values/specified/image.rs b/components/style/values/specified/image.rs index a0d1b2260b9..851700257ba 100644 --- a/components/style/values/specified/image.rs +++ b/components/style/values/specified/image.rs @@ -138,7 +138,7 @@ impl Parse for Image { return Ok(GenericImage::Url(url)); } if let Ok(gradient) = input.try(|i| Gradient::parse(context, i)) { - return Ok(GenericImage::Gradient(gradient)); + return Ok(GenericImage::Gradient(Box::new(gradient))); } #[cfg(feature = "servo")] { @@ -151,7 +151,7 @@ impl Parse for Image { { image_rect.url.build_image_value(); } - return Ok(GenericImage::Rect(image_rect)); + return Ok(GenericImage::Rect(Box::new(image_rect))); } Ok(GenericImage::Element(Image::parse_element(input)?)) } diff --git a/tests/unit/stylo/size_of.rs b/tests/unit/stylo/size_of.rs index 3a830e1c40a..8ec8e8c0fd1 100644 --- a/tests/unit/stylo/size_of.rs +++ b/tests/unit/stylo/size_of.rs @@ -11,6 +11,8 @@ use style::data::{ElementData, ElementStyles, RestyleData}; use style::gecko::selector_parser as real; use style::properties::ComputedValues; use style::rule_tree::{RuleNode, StrongRuleNode}; +use style::values::computed; +use style::values::specified; #[test] fn size_of_selectors_dummy_types() { @@ -47,3 +49,11 @@ size_of_test!(test_size_of_rule_node, RuleNode, 80); // This is huge, but we allocate it on the stack and then never move it, // we only pass `&mut SourcePropertyDeclaration` references around. size_of_test!(test_size_of_parsed_declaration, style::properties::SourcePropertyDeclaration, 704); + +size_of_test!(test_size_of_computed_image, computed::image::Image, 40); +size_of_test!(test_size_of_specified_image, specified::image::Image, 40); + +// FIXME(bz): These can shrink if we move the None_ value inside the +// enum instead of paying an extra word for the Either discriminant. +size_of_test!(test_size_of_computed_image_layer, computed::image::ImageLayer, 48); +size_of_test!(test_size_of_specified_image_layer, specified::image::ImageLayer, 48); From 385e34acee65b3c7fdf9ff410722b3963a3827ee Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Sat, 9 Sep 2017 04:31:25 -0400 Subject: [PATCH 2/2] Avoid array overallocation when parsing background shorthand. r=xidorn This is part 2 of the fix for Gecko bug 1397614 --- .../style/properties/shorthand/background.mako.rs | 14 +++++++++++--- components/style/properties/shorthand/mask.mako.rs | 14 +++++++++++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/components/style/properties/shorthand/background.mako.rs b/components/style/properties/shorthand/background.mako.rs index 10ecddf3c1c..32cf30b6db6 100644 --- a/components/style/properties/shorthand/background.mako.rs +++ b/components/style/properties/shorthand/background.mako.rs @@ -37,7 +37,11 @@ let mut background_color = None; % for name in "image position_x position_y repeat size attachment origin clip".split(): - let mut background_${name} = background_${name}::SpecifiedValue(Vec::new()); + // Vec grows from 0 to 4 by default on first push(). So allocate + // with capacity 1, so in the common case of only one item we don't + // way overallocate. Note that we always push at least one item if + // parsing succeeds. + let mut background_${name} = background_${name}::SpecifiedValue(Vec::with_capacity(1)); % endfor input.parse_comma_separated(|input| { // background-color can only be in the last element, so if it @@ -197,8 +201,12 @@ pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { - let mut position_x = background_position_x::SpecifiedValue(Vec::new()); - let mut position_y = background_position_y::SpecifiedValue(Vec::new()); + // Vec grows from 0 to 4 by default on first push(). So allocate with + // capacity 1, so in the common case of only one item we don't way + // overallocate. Note that we always push at least one item if parsing + // succeeds. + let mut position_x = background_position_x::SpecifiedValue(Vec::with_capacity(1)); + let mut position_y = background_position_y::SpecifiedValue(Vec::with_capacity(1)); let mut any = false; input.parse_comma_separated(|input| { diff --git a/components/style/properties/shorthand/mask.mako.rs b/components/style/properties/shorthand/mask.mako.rs index 5b84052c45a..b0e298317fe 100644 --- a/components/style/properties/shorthand/mask.mako.rs +++ b/components/style/properties/shorthand/mask.mako.rs @@ -38,7 +38,11 @@ pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { % for name in "image mode position_x position_y size repeat origin clip composite".split(): - let mut mask_${name} = mask_${name}::SpecifiedValue(Vec::new()); + // Vec grows from 0 to 4 by default on first push(). So allocate + // with capacity 1, so in the common case of only one item we don't + // way overallocate. Note that we always push at least one item if + // parsing succeeds. + let mut mask_${name} = mask_${name}::SpecifiedValue(Vec::with_capacity(1)); % endfor input.parse_comma_separated(|input| { @@ -183,8 +187,12 @@ pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result> { - let mut position_x = mask_position_x::SpecifiedValue(Vec::new()); - let mut position_y = mask_position_y::SpecifiedValue(Vec::new()); + // Vec grows from 0 to 4 by default on first push(). So allocate with + // capacity 1, so in the common case of only one item we don't way + // overallocate. Note that we always push at least one item if parsing + // succeeds. + let mut position_x = mask_position_x::SpecifiedValue(Vec::with_capacity(1)); + let mut position_y = mask_position_y::SpecifiedValue(Vec::with_capacity(1)); let mut any = false; input.parse_comma_separated(|input| {