style: Share custom property declarations if they don't contain variable references.

Cargo.lock

@@ -3234,6 +3234,7 @@ dependencies = [
  "malloc_size_of_derive 0.0.1",
  "selectors 0.19.0",
  "serde 1.0.14 (registry+https://github.com/rust-lang/crates.io-index)",
+ "servo_arc 0.0.1",
  "servo_atoms 0.0.1",
  "webrender_api 0.52.0 (git+https://github.com/servo/webrender)",
 ]

components/style/custom_properties.rs

@@ -10,7 +10,7 @@ use Atom;
 use cssparser::{Delimiter, Parser, ParserInput, SourcePosition, Token, TokenSerializationType};
 use precomputed_hash::PrecomputedHash;
 use properties::{CSSWideKeyword, DeclaredValue};
-use selector_map::{PrecomputedHashSet, PrecomputedDiagnosticHashMap};
+use selector_map::{PrecomputedHashSet, PrecomputedHashMap, PrecomputedDiagnosticHashMap};
 use selectors::parser::SelectorParseError;
 use servo_arc::Arc;
 use std::ascii::AsciiExt;
@@ -35,14 +35,14 @@ pub fn parse_name(s: &str) -> Result<&str, ()> {
     }
 }
 
-/// A specified value for a custom property is just a set of tokens.
+/// A value for a custom property is just a set of tokens.
 ///
 /// We preserve the original CSS for serialization, and also the variable
 /// references to other custom property names.
 #[derive(Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
-pub struct SpecifiedValue {
+pub struct VariableValue {
     css: String,
 
     first_token_type: TokenSerializationType,
@@ -52,24 +52,6 @@ pub struct SpecifiedValue {
     references: PrecomputedHashSet<Name>,
 }
 
-/// This struct is a cheap borrowed version of a `SpecifiedValue`.
-pub struct BorrowedSpecifiedValue<'a> {
-    css: &'a str,
-    first_token_type: TokenSerializationType,
-    last_token_type: TokenSerializationType,
-    references: &'a PrecomputedHashSet<Name>,
-}
-
-/// A computed value is just a set of tokens as well, until we resolve variables
-/// properly.
-#[derive(Clone, Debug, Eq, PartialEq)]
-#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
-pub struct ComputedValue {
-    css: String,
-    first_token_type: TokenSerializationType,
-    last_token_type: TokenSerializationType,
-}
-
 impl ToCss for SpecifiedValue {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result
         where W: fmt::Write,
@@ -78,14 +60,6 @@ impl ToCss for SpecifiedValue {
     }
 }
 
-impl ToCss for ComputedValue {
-    fn to_css<W>(&self, dest: &mut W) -> fmt::Result
-        where W: fmt::Write,
-    {
-        dest.write_str(&self.css)
-    }
-}
-
 /// A map from CSS variable names to CSS variable computed values, used for
 /// resolving.
 ///
@@ -93,7 +67,17 @@ impl ToCss for ComputedValue {
 /// DOM. CSSDeclarations expose property names as indexed properties, which
 /// need to be stable. So we keep an array of property names which order is
 /// determined on the order that they are added to the name-value map.
-pub type CustomPropertiesMap = OrderedMap<Name, ComputedValue>;
+///
+/// The variable values are guaranteed to not have references to other
+/// properties.
+pub type CustomPropertiesMap = OrderedMap<Name, Arc<VariableValue>>;
+
+/// Both specified and computed values are VariableValues, the difference is
+/// whether var() functions are expanded.
+pub type SpecifiedValue = VariableValue;
+/// Both specified and computed values are VariableValues, the difference is
+/// whether var() functions are expanded.
+pub type ComputedValue = VariableValue;
 
 /// A map that preserves order for the keys, and that is easily indexable.
 #[derive(Clone, Debug, Eq, PartialEq)]
@@ -136,6 +120,11 @@ where
         value
     }
 
+    /// Get whether there's a value on the map for `key`.
+    pub fn contains_key(&self, key: &K) -> bool {
+        self.values.contains_key(key)
+    }
+
     /// Get the key located at the given index.
     pub fn get_key_at(&self, index: u32) -> Option<&K> {
         self.index.get(index as usize)
@@ -211,22 +200,30 @@ where
     }
 }
 
-impl ComputedValue {
+impl VariableValue {
-    fn empty() -> ComputedValue {
+    fn empty() -> Self {
-        ComputedValue {
+        Self {
             css: String::new(),
             last_token_type: TokenSerializationType::nothing(),
             first_token_type: TokenSerializationType::nothing(),
+            references: PrecomputedHashSet::default(),
         }
     }
 
-    fn push(&mut self, css: &str, css_first_token_type: TokenSerializationType,
+    fn push(
-            css_last_token_type: TokenSerializationType) {
+        &mut self,
-        // This happens e.g. between to subsequent var() functions: `var(--a)var(--b)`.
+        css: &str,
-        // In that case, css_*_token_type is non-sensical.
+        css_first_token_type: TokenSerializationType,
+        css_last_token_type: TokenSerializationType
+    ) {
+        // This happens e.g. between two subsequent var() functions:
+        // `var(--a)var(--b)`.
+        //
+        // In that case, css_*_token_type is nonsensical.
         if css.is_empty() {
             return
         }
+
         self.first_token_type.set_if_nothing(css_first_token_type);
         // If self.first_token_type was nothing,
         // self.last_token_type is also nothing and this will be false:
@@ -237,27 +234,32 @@ impl ComputedValue {
         self.last_token_type = css_last_token_type
     }
 
-    fn push_from(&mut self, position: (SourcePosition, TokenSerializationType),
+    fn push_from(
-                 input: &Parser, last_token_type: TokenSerializationType) {
+        &mut self,
+        position: (SourcePosition, TokenSerializationType),
+        input: &Parser,
+        last_token_type: TokenSerializationType
+    ) {
         self.push(input.slice_from(position.0), position.1, last_token_type)
     }
 
     fn push_variable(&mut self, variable: &ComputedValue) {
+        debug_assert!(variable.references.is_empty());
         self.push(&variable.css, variable.first_token_type, variable.last_token_type)
     }
 }
 
-impl SpecifiedValue {
+impl VariableValue {
-    /// Parse a custom property SpecifiedValue.
+    /// Parse a custom property value.
     pub fn parse<'i, 't>(
         input: &mut Parser<'i, 't>,
-    ) -> Result<Box<Self>, ParseError<'i>> {
+    ) -> Result<Arc<Self>, ParseError<'i>> {
         let mut references = PrecomputedHashSet::default();
 
         let (first_token_type, css, last_token_type) =
             parse_self_contained_declaration_value(input, Some(&mut references))?;
 
-        Ok(Box::new(SpecifiedValue {
+        Ok(Arc::new(VariableValue {
             css: css.into_owned(),
             first_token_type,
             last_token_type,
@@ -467,7 +469,7 @@ fn parse_var_function<'i, 't>(
 pub struct CustomPropertiesBuilder<'a> {
     seen: PrecomputedHashSet<&'a Name>,
     may_have_cycles: bool,
-    specified_custom_properties: OrderedMap<&'a Name, Option<BorrowedSpecifiedValue<'a>>>,
+    custom_properties: Option<CustomPropertiesMap>,
     inherited: Option<&'a Arc<CustomPropertiesMap>>,
 }
 
@@ -477,7 +479,7 @@ impl<'a> CustomPropertiesBuilder<'a> {
         Self {
             seen: PrecomputedHashSet::default(),
             may_have_cycles: false,
-            specified_custom_properties: OrderedMap::new(),
+            custom_properties: None,
             inherited,
         }
     }
@@ -486,27 +488,30 @@ impl<'a> CustomPropertiesBuilder<'a> {
     pub fn cascade(
         &mut self,
         name: &'a Name,
-        specified_value: DeclaredValue<'a, Box<SpecifiedValue>>,
+        specified_value: DeclaredValue<'a, Arc<SpecifiedValue>>,
     ) {
         let was_already_present = !self.seen.insert(name);
         if was_already_present {
             return;
         }
 
+        if self.custom_properties.is_none() {
+            self.custom_properties = Some(match self.inherited {
+                Some(inherited) => (**inherited).clone(),
+                None => CustomPropertiesMap::new(),
+            })
+        }
+
+        let map = self.custom_properties.as_mut().unwrap();
         match specified_value {
             DeclaredValue::Value(ref specified_value) => {
                 self.may_have_cycles |= !specified_value.references.is_empty();
-                self.specified_custom_properties.insert(name, Some(BorrowedSpecifiedValue {
+                map.insert(name.clone(), (*specified_value).clone());
-                    css: &specified_value.css,
-                    first_token_type: specified_value.first_token_type,
-                    last_token_type: specified_value.last_token_type,
-                    references: &specified_value.references,
-                }));
             },
             DeclaredValue::WithVariables(_) => unreachable!(),
             DeclaredValue::CSSWideKeyword(keyword) => match keyword {
                 CSSWideKeyword::Initial => {
-                    self.specified_custom_properties.insert(name, None);
+                    map.remove(name);
                 }
                 CSSWideKeyword::Unset | // Custom properties are inherited by default.
                 CSSWideKeyword::Inherit => {} // The inherited value is what we already have.
@@ -521,14 +526,16 @@ impl<'a> CustomPropertiesBuilder<'a> {
     ///
     /// Otherwise, just use the inherited custom properties map.
     pub fn build(mut self) -> Option<Arc<CustomPropertiesMap>> {
-        if self.specified_custom_properties.is_empty() {
+        let mut map = match self.custom_properties.take() {
-            return self.inherited.cloned();
+            Some(m) => m,
-        }
+            None => return self.inherited.cloned(),
+        };
 
         if self.may_have_cycles {
-            remove_cycles(&mut self.specified_custom_properties);
+            remove_cycles(&mut map);
+            substitute_all(&mut map);
         }
-        Some(Arc::new(substitute_all(self.specified_custom_properties, self.inherited)))
+        Some(Arc::new(map))
     }
 }
 
@@ -536,7 +543,7 @@ impl<'a> CustomPropertiesBuilder<'a> {
 ///
 /// The initial value of a custom property is represented by this property not
 /// being in the map.
-fn remove_cycles(map: &mut OrderedMap<&Name, Option<BorrowedSpecifiedValue>>) {
+fn remove_cycles(map: &mut CustomPropertiesMap) {
     let mut to_remove = PrecomputedHashSet::default();
     {
         let mut visited = PrecomputedHashSet::default();
@@ -545,7 +552,7 @@ fn remove_cycles(map: &mut OrderedMap<&Name, Option<BorrowedSpecifiedValue>>) {
             walk(map, name, &mut stack, &mut visited, &mut to_remove);
 
             fn walk<'a>(
-                map: &OrderedMap<&'a Name, Option<BorrowedSpecifiedValue<'a>>>,
+                map: &'a CustomPropertiesMap,
                 name: &'a Name,
                 stack: &mut Vec<&'a Name>,
                 visited: &mut PrecomputedHashSet<&'a Name>,
@@ -555,11 +562,11 @@ fn remove_cycles(map: &mut OrderedMap<&Name, Option<BorrowedSpecifiedValue>>) {
                 if already_visited_before {
                     return
                 }
-                if let Some(&Some(ref value)) = map.get(&name) {
+                if let Some(ref value) = map.get(name) {
                     if !value.references.is_empty() {
                         stack.push(name);
-                        for next in value.references {
+                        for next in value.references.iter() {
-                            if let Some(position) = stack.iter().position(|&x| x == next) {
+                            if let Some(position) = stack.iter().position(|x| *x == next) {
                                 // Found a cycle
                                 for &in_cycle in &stack[position..] {
                                     to_remove.insert(in_cycle.clone());
@@ -580,97 +587,116 @@ fn remove_cycles(map: &mut OrderedMap<&Name, Option<BorrowedSpecifiedValue>>) {
 }
 
 /// Replace `var()` functions for all custom properties.
-fn substitute_all(
+fn substitute_all(custom_properties_map: &mut CustomPropertiesMap) {
-    specified_values_map: OrderedMap<&Name, Option<BorrowedSpecifiedValue>>,
+    // FIXME(emilio): This stash is needed because we can't prove statically to
-    inherited: Option<&Arc<CustomPropertiesMap>>,
+    // rustc that we don't try to mutate the same variable from two recursive
-) -> CustomPropertiesMap {
+    // `substitute_one` calls.
-    let mut custom_properties_map = match inherited {
+    //
-        None => CustomPropertiesMap::new(),
+    // If this is really really hot, we may be able to cheat using `unsafe`, I
-        Some(inherited) => (**inherited).clone(),
+    // guess...
-    };
+    let mut stash = PrecomputedHashMap::default();
-
     let mut invalid = PrecomputedHashSet::default();
 
-    for (name, value) in specified_values_map.iter() {
+    for (name, value) in custom_properties_map.iter() {
-        let value = match *value {
+        if !value.references.is_empty() && !stash.contains_key(name) {
-            Some(ref v) => v,
+            let _ = substitute_one(
-            None => {
+                name,
-                custom_properties_map.remove(*name);
+                value,
-                continue;
+                custom_properties_map,
-            }
+                None,
-        };
+                &mut stash,
-
+                &mut invalid,
-        let _ = substitute_one(
+            );
-            name,
+        }
-            value,
-            &specified_values_map,
-            None,
-            &mut custom_properties_map,
-            &mut invalid,
-        );
     }
 
-    custom_properties_map
+    for (name, value) in stash.drain() {
+        custom_properties_map.insert(name, value);
+    }
+
+    for name in invalid.drain() {
+        custom_properties_map.remove(&name);
+    }
+
+    debug_assert!(custom_properties_map.iter().all(|(_, v)| v.references.is_empty()));
 }
 
-/// Replace `var()` functions for one custom property.
+/// Replace `var()` functions for one custom property, leaving the result in
-/// Also recursively record results for other custom properties referenced by `var()` functions.
+/// `stash`.
-/// Return `Err(())` for invalid at computed time.
+///
-/// or `Ok(last_token_type that was pushed to partial_computed_value)` otherwise.
+/// Also recursively record results for other custom properties referenced by
+/// `var()` functions.
+///
+/// Return `Err(())` for invalid at computed time.  or `Ok(last_token_type that
+/// was pushed to partial_computed_value)` otherwise.
 fn substitute_one(
     name: &Name,
-    specified_value: &BorrowedSpecifiedValue,
+    specified_value: &Arc<VariableValue>,
-    specified_values_map: &OrderedMap<&Name, Option<BorrowedSpecifiedValue>>,
+    custom_properties: &CustomPropertiesMap,
-    partial_computed_value: Option<&mut ComputedValue>,
+    partial_computed_value: Option<&mut VariableValue>,
-    custom_properties_map: &mut CustomPropertiesMap,
+    stash: &mut PrecomputedHashMap<Name, Arc<VariableValue>>,
     invalid: &mut PrecomputedHashSet<Name>,
 ) -> Result<TokenSerializationType, ()> {
+    debug_assert!(!specified_value.references.is_empty());
+    debug_assert!(!stash.contains_key(name));
+
     if invalid.contains(name) {
         return Err(());
     }
 
-    let computed_value = if specified_value.references.is_empty() {
+    let mut computed_value = ComputedValue::empty();
-        // The specified value contains no var() reference
+    let mut input = ParserInput::new(&specified_value.css);
-        ComputedValue {
+    let mut input = Parser::new(&mut input);
-            css: specified_value.css.to_owned(),
+    let mut position = (input.position(), specified_value.first_token_type);
-            first_token_type: specified_value.first_token_type,
+
-            last_token_type: specified_value.last_token_type,
+    let result = substitute_block(
-        }
+        &mut input,
-    } else {
+        &mut position,
-        let mut partial_computed_value = ComputedValue::empty();
+        &mut computed_value,
-        let mut input = ParserInput::new(&specified_value.css);
+        &mut |name, partial_computed_value| {
-        let mut input = Parser::new(&mut input);
+            if let Some(already_computed) = stash.get(name) {
-        let mut position = (input.position(), specified_value.first_token_type);
+                partial_computed_value.push_variable(already_computed);
-        let result = substitute_block(
+                return Ok(already_computed.last_token_type);
-            &mut input, &mut position, &mut partial_computed_value,
-            &mut |name, partial_computed_value| {
-                if let Some(other_specified_value) = specified_values_map.get(&name).and_then(|v| v.as_ref()) {
-                    substitute_one(
-                        name,
-                        other_specified_value,
-                        specified_values_map,
-                        Some(partial_computed_value),
-                        custom_properties_map,
-                        invalid,
-                    )
-                } else {
-                    Err(())
-                }
             }
-        );
+
-        if let Ok(last_token_type) = result {
+            let other_specified_value = match custom_properties.get(name) {
-            partial_computed_value.push_from(position, &input, last_token_type);
+                Some(v) => v,
-            partial_computed_value
+                None => return Err(()),
-        } else {
+            };
+
+            if other_specified_value.references.is_empty() {
+                partial_computed_value.push_variable(other_specified_value);
+                return Ok(other_specified_value.last_token_type);
+            }
+
+            substitute_one(
+                name,
+                other_specified_value,
+                custom_properties,
+                Some(partial_computed_value),
+                stash,
+                invalid
+            )
+        }
+    );
+
+    match result {
+        Ok(last_token_type) => {
+            computed_value.push_from(position, &input, last_token_type);
+        }
+        Err(..) => {
             invalid.insert(name.clone());
             return Err(())
         }
-    };
+    }
+
     if let Some(partial_computed_value) = partial_computed_value {
         partial_computed_value.push_variable(&computed_value)
     }
+
     let last_token_type = computed_value.last_token_type;
-    custom_properties_map.insert(name.clone(), computed_value);
+    stash.insert(name.clone(), Arc::new(computed_value));
+
     Ok(last_token_type)
 }
 

components/style/properties/properties.mako.rs

@@ -1398,7 +1398,12 @@ pub enum PropertyDeclaration {
     ),
     /// A custom property declaration, with the property name and the declared
     /// value.
-    Custom(::custom_properties::Name, DeclaredValueOwned<Box<::custom_properties::SpecifiedValue>>),
+    #[cfg_attr(feature = "gecko", ignore_malloc_size_of = "XXX: how to handle this?")]
+    Custom(
+        ::custom_properties::Name,
+        #[cfg_attr(feature = "gecko", ignore_malloc_size_of = "XXX: how to handle this?")]
+        DeclaredValueOwned<Arc<::custom_properties::SpecifiedValue>>
+    ),
 }
 
 impl fmt::Debug for PropertyDeclaration {

components/style/values/generics/image.rs

@@ -9,6 +9,7 @@
 use Atom;
 use cssparser::serialize_identifier;
 use custom_properties;
+use servo_arc::Arc;
 use std::fmt;
 use style_traits::ToCss;
 
@@ -153,7 +154,8 @@ pub struct PaintWorklet {
     pub name: Atom,
     /// The arguments for the worklet.
     /// TODO: store a parsed representation of the arguments.
-    pub arguments: Vec<custom_properties::SpecifiedValue>,
+    #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")]
+    pub arguments: Vec<Arc<custom_properties::SpecifiedValue>>,
 }
 
 trivial_to_computed_value!(PaintWorklet);

components/style/values/specified/image.rs

@@ -917,7 +917,7 @@ impl Parse for PaintWorklet {
             let name = Atom::from(&**input.expect_ident()?);
             let arguments = input.try(|input| {
                 input.expect_comma()?;
-                input.parse_comma_separated(|input| Ok(*SpecifiedValue::parse(input)?))
+                input.parse_comma_separated(|input| SpecifiedValue::parse(input))
             }).unwrap_or(vec![]);
             Ok(PaintWorklet { name, arguments })
         })

components/style_traits/Cargo.toml

@@ -26,3 +26,4 @@ selectors = { path = "../selectors" }
 serde = {version = "1.0", optional = true}
 webrender_api = {git = "https://github.com/servo/webrender", optional = true}
 servo_atoms = {path = "../atoms", optional = true}
+servo_arc = {path = "../servo_arc" }

components/style_traits/lib.rs

@@ -24,6 +24,7 @@ extern crate euclid;
 extern crate selectors;
 #[cfg(feature = "servo")] #[macro_use] extern crate serde;
 #[cfg(feature = "servo")] extern crate webrender_api;
+extern crate servo_arc;
 #[cfg(feature = "servo")] extern crate servo_atoms;
 
 #[cfg(feature = "servo")] pub use webrender_api::DevicePixel;

components/style_traits/values.rs

@@ -7,6 +7,7 @@
 use app_units::Au;
 use cssparser::{BasicParseError, ParseError, Parser, Token, UnicodeRange, serialize_string};
 use cssparser::ToCss as CssparserToCss;
+use servo_arc::Arc;
 use std::fmt::{self, Write};
 
 /// Serialises a value according to its CSS representation.
@@ -343,6 +344,14 @@ impl<T> ToCss for Box<T> where T: ?Sized + ToCss {
     }
 }
 
+impl<T> ToCss for Arc<T> where T: ?Sized + ToCss {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result
+        where W: Write,
+    {
+        (**self).to_css(dest)
+    }
+}
+
 impl ToCss for Au {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: Write {
         self.to_f64_px().to_css(dest)?;