style: Cache substituted values from shorthand properties during the cascade.

This brings the time down to 1.6ms from 4.8ms on the test-case in the
bug. This should be improvable too, but I think this is a nice
improvement for regular styling as well.

Differential Revision: https://phabricator.services.mozilla.com/D105187

components/style/properties/cascade.rs

@@ -10,11 +10,10 @@ use crate::dom::TElement;
 use crate::font_metrics::FontMetricsProvider;
 use crate::logical_geometry::WritingMode;
 use crate::media_queries::Device;
-use crate::properties::{CSSWideKeyword, LonghandId, LonghandIdSet, PropertyFlags};
-use crate::properties::{ComputedValueFlags, CASCADE_PROPERTY};
-use crate::properties::{ComputedValues, Importance, StyleBuilder};
 use crate::properties::{
-    DeclarationImportanceIterator, PropertyDeclaration, PropertyDeclarationId,
+    CSSWideKeyword, ComputedValueFlags, ComputedValues, DeclarationImportanceIterator, Importance,
+    LonghandId, LonghandIdSet, PropertyDeclaration, PropertyDeclarationId, PropertyFlags,
+    ShorthandsWithPropertyReferencesCache, StyleBuilder, CASCADE_PROPERTY,
 };
 use crate::rule_cache::{RuleCache, RuleCacheConditions};
 use crate::rule_tree::StrongRuleNode;
@@ -325,10 +324,12 @@ where
 
     let using_cached_reset_properties = {
         let mut cascade = Cascade::new(&mut context, cascade_mode);
+        let mut shorthand_cache = ShorthandsWithPropertyReferencesCache::default();
 
         cascade.apply_properties::<EarlyProperties, _>(
             ApplyResetProperties::Yes,
             declarations.iter().cloned(),
+            &mut shorthand_cache,
         );
 
         cascade.compute_visited_style_if_needed(
@@ -348,7 +349,11 @@ where
             ApplyResetProperties::Yes
         };
 
-        cascade.apply_properties::<LateProperties, _>(apply_reset, declarations.iter().cloned());
+        cascade.apply_properties::<LateProperties, _>(
+            apply_reset,
+            declarations.iter().cloned(),
+            &mut shorthand_cache,
+        );
 
         using_cached_reset_properties
     };
@@ -501,10 +506,14 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
         }
     }
 
-    fn substitute_variables_if_needed<'decl>(
+    fn substitute_variables_if_needed<'decl, 'cache>(
         &mut self,
         declaration: &'decl PropertyDeclaration,
-    ) -> Cow<'decl, PropertyDeclaration> {
+        cache: &'cache mut ShorthandsWithPropertyReferencesCache,
+    ) -> Cow<'decl, PropertyDeclaration>
+    where
+        'cache: 'decl,
+    {
         let declaration = match *declaration {
             PropertyDeclaration::WithVariables(ref declaration) => declaration,
             ref d => return Cow::Borrowed(d),
@@ -536,12 +545,13 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
             }
         }
 
-        Cow::Owned(declaration.value.substitute_variables(
+        declaration.value.substitute_variables(
             declaration.id,
             self.context.builder.custom_properties.as_ref(),
             self.context.quirks_mode,
             self.context.device(),
-        ))
+            cache,
+        )
     }
 
     #[inline(always)]
@@ -559,6 +569,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
         &mut self,
         apply_reset: ApplyResetProperties,
         declarations: I,
+        mut shorthand_cache: &mut ShorthandsWithPropertyReferencesCache,
     ) where
         Phase: CascadePhase,
         I: Iterator<Item = (&'decls PropertyDeclaration, Origin)>,
@@ -619,7 +630,8 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
                 continue;
             }
 
-            let mut declaration = self.substitute_variables_if_needed(declaration);
+            let mut declaration =
+                self.substitute_variables_if_needed(declaration, &mut shorthand_cache);
 
             // When document colors are disabled, do special handling of
             // properties that are marked as ignored in that mode.

components/style/properties/declaration_block.rs

@@ -837,6 +837,7 @@ impl PropertyDeclarationBlock {
                         custom_properties.as_ref(),
                         QuirksMode::NoQuirks,
                         device,
+                        &mut Default::default()
                     )
                     .to_css(dest)
             },

components/style/properties/helpers/animated_properties.mako.rs

@@ -367,6 +367,7 @@ impl AnimationValue {
                 }
             },
             PropertyDeclaration::WithVariables(ref declaration) => {
+                let mut cache = Default::default();
                 let substituted = {
                     let custom_properties =
                         extra_custom_properties.or_else(|| context.style().custom_properties());
@@ -376,6 +377,7 @@ impl AnimationValue {
                         custom_properties,
                         context.quirks_mode,
                         context.device(),
+                        &mut cache,
                     )
                 };
                 return AnimationValue::from_declaration(

components/style/properties/properties.mako.rs

@@ -29,11 +29,11 @@ use crate::context::QuirksMode;
 use crate::logical_geometry::WritingMode;
 use malloc_size_of::{MallocSizeOf, MallocSizeOfOps};
 use crate::computed_value_flags::*;
+use crate::hash::FxHashMap;
 use crate::media_queries::Device;
 use crate::parser::ParserContext;
 use crate::properties::longhands::system_font::SystemFont;
 use crate::selector_parser::PseudoElement;
-use selectors::parser::SelectorParseErrorKind;
 #[cfg(feature = "servo")] use servo_config::prefs;
 use style_traits::{CssWriter, KeywordsCollectFn, ParseError, ParsingMode};
 use style_traits::{SpecifiedValueInfo, StyleParseErrorKind, ToCss};
@@ -1634,23 +1634,44 @@ impl ToCss for UnparsedValue {
     }
 }
 
+/// A simple cache for properties that come from a shorthand and have variable
+/// references.
+///
+/// This cache works because of the fact that you can't have competing values
+/// for a given longhand coming from the same shorthand (but note that this is
+/// why the shorthand needs to be part of the cache key).
+pub type ShorthandsWithPropertyReferencesCache =
+    FxHashMap<(ShorthandId, LonghandId), PropertyDeclaration>;
+
 impl UnparsedValue {
-    fn substitute_variables(
+    fn substitute_variables<'cache>(
         &self,
         longhand_id: LonghandId,
         custom_properties: Option<<&Arc<crate::custom_properties::CustomPropertiesMap>>,
         quirks_mode: QuirksMode,
         device: &Device,
-    ) -> PropertyDeclaration {
+        shorthand_cache: &'cache mut ShorthandsWithPropertyReferencesCache,
+    ) -> Cow<'cache, PropertyDeclaration> {
         let invalid_at_computed_value_time = || {
             let keyword = if longhand_id.inherited() {
                 CSSWideKeyword::Inherit
             } else {
                 CSSWideKeyword::Initial
             };
-            PropertyDeclaration::css_wide_keyword(longhand_id, keyword)
+            Cow::Owned(PropertyDeclaration::css_wide_keyword(longhand_id, keyword))
         };
 
+        if let Some(shorthand_id) = self.from_shorthand {
+            let key = (shorthand_id, longhand_id);
+            if shorthand_cache.contains_key(&key) {
+                // FIXME: This double lookup should be avoidable, but rustc
+                // doesn't like that, see:
+                //
+                // https://github.com/rust-lang/rust/issues/82146
+                return Cow::Borrowed(&shorthand_cache[&key]);
+            }
+        }
+
         let css = match crate::custom_properties::substitute(
             &self.css,
             self.first_token_type,
@@ -1685,53 +1706,59 @@ impl UnparsedValue {
         let mut input = Parser::new(&mut input);
         input.skip_whitespace();  // Unnecessary for correctness, but may help try() rewind less.
         if let Ok(keyword) = input.try_parse(CSSWideKeyword::parse) {
-            return PropertyDeclaration::css_wide_keyword(longhand_id, keyword);
+            return Cow::Owned(PropertyDeclaration::css_wide_keyword(longhand_id, keyword));
         }
 
-        let declaration = input.parse_entirely(|input| {
+        let shorthand = match self.from_shorthand {
-            match self.from_shorthand {
+            None => {
-                None => longhand_id.parse_value(&context, input),
+                return match input.parse_entirely(|input| longhand_id.parse_value(&context, input)) {
-                Some(ShorthandId::All) => {
+                    Ok(decl) => Cow::Owned(decl),
-                    // No need to parse the 'all' shorthand as anything other
+                    Err(..) => invalid_at_computed_value_time(),
-                    // than a CSS-wide keyword, after variable substitution.
-                    Err(input.new_custom_error(SelectorParseErrorKind::UnexpectedIdent("all".into())))
                 }
-                % for shorthand in data.shorthands_except_all():
+            },
-                Some(ShorthandId::${shorthand.camel_case}) => {
+            Some(shorthand) => shorthand,
-                    shorthands::${shorthand.ident}::parse_value(&context, input)
+        };
-                    .map(|longhands| {
+
-                        match longhand_id {
+        match shorthand {
-                            <% seen = set() %>
+            ShorthandId::All => {
-                            % for property in shorthand.sub_properties:
+                // No need to parse the 'all' shorthand as anything other
-                            // When animating logical properties, we end up
+                // than a CSS-wide keyword, after variable substitution.
-                            // physicalizing the value during the animation, but
+                return invalid_at_computed_value_time();
-                            // the value still comes from the logical shorthand.
+            }
-                            //
+            % for shorthand in data.shorthands_except_all():
-                            // So we need to handle the physical properties too.
+            ShorthandId::${shorthand.camel_case} => {
-                            % for prop in [property] + property.all_physical_mapped_properties(data):
+                let longhands = match input.parse_entirely(|input| shorthands::${shorthand.ident}::parse_value(&context, input)) {
-                            % if prop.camel_case not in seen:
+                    Ok(l) => l,
-                            LonghandId::${prop.camel_case} => {
+                    _ => return invalid_at_computed_value_time(),
-                                PropertyDeclaration::${prop.camel_case}(
+                };
-                                    longhands.${property.ident}
+
-                                )
+                <% seen = set() %>
-                            }
+                % for property in shorthand.sub_properties:
-                            <% seen.add(prop.camel_case) %>
+                // When animating logical properties, we end up
+                // physicalizing the value during the animation, but the
+                // value still comes from the logical shorthand.
+                //
+                // So we need to handle the physical properties too.
+                % for prop in property.all_physical_mapped_properties(data) + [property]:
+                % if prop.ident not in seen:
+                    shorthand_cache.insert(
+                        (shorthand, LonghandId::${prop.camel_case}),
+                        PropertyDeclaration::${prop.camel_case}(
+                            longhands.${property.ident}
+                            % if prop.ident != property.ident:
+                            .clone()
                             % endif
-                            % endfor
+                        )
-                            % endfor
+                    );
-                            <% del seen %>
+                % endif
-                            _ => unreachable!()
+                <% seen.add(prop.ident) %>
-                        }
+                % endfor
-                    })
-                }
                 % endfor
             }
-        });
+            % endfor
-
-        match declaration {
-            Ok(decl) => decl,
-            Err(..) => invalid_at_computed_value_time(),
         }
+
+        Cow::Borrowed(&shorthand_cache[&(shorthand, longhand_id)])
     }
 }