Bug 1690836 - Reduce the amount of code generated by UnparsedValues::substitute_variables. r=boris

This reduces the amount of assembly instructions generated by this
function from 18k+ to ~800.

This should make reasoning about its stack space usage sane, and should
fix the ASAN stack overflows, but also we should take this regardless,
because it's saner and makes reading it simpler.

I also think that the writing_mode shenanigans is fixing a bug (I think
before this, we'd pick the first physical value which mapped to any of
the properties, which is wrong), but I haven't bothered looking for a
test-case that fails before my patch. The relevant WPTs
(css/css-logical/animation*) still pass.

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

servo/components/style/properties/cascade.rs

@@ -547,6 +547,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
 
         declaration.value.substitute_variables(
             declaration.id,
+            self.context.builder.writing_mode,
             self.context.builder.custom_properties.as_ref(),
             self.context.quirks_mode,
             self.context.device(),

servo/components/style/properties/declaration_block.rs

@@ -829,11 +829,12 @@ impl PropertyDeclarationBlock {
             // getKeyframes() implementation for CSS animations, if
             // |computed_values| is supplied, we use it to expand such variable
             // declarations. This will be fixed properly in Gecko bug 1391537.
-            (&PropertyDeclaration::WithVariables(ref declaration), Some(ref _computed_values)) => {
+            (&PropertyDeclaration::WithVariables(ref declaration), Some(ref computed_values)) => {
                 declaration
                     .value
                     .substitute_variables(
                         declaration.id,
+                        computed_values.writing_mode,
                         custom_properties.as_ref(),
                         QuirksMode::NoQuirks,
                         device,

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

@@ -948,7 +948,7 @@
             input.parse_entirely(|input| parse_value(context, input)).map(|longhands| {
                 % for sub_property in shorthand.sub_properties:
                 % if sub_property.may_be_disabled_in(shorthand, engine):
-                if NonCustomPropertyId::from(LonghandId::${sub_property.camel_case}).allowed_in(context) {
+                if NonCustomPropertyId::from(LonghandId::${sub_property.camel_case}).allowed_in_ignoring_rule_type(context) {
                 % endif
                     declarations.push(PropertyDeclaration::${sub_property.camel_case}(
                         longhands.${sub_property.ident}
@@ -980,7 +980,7 @@
     #[allow(unused_imports)]
     use crate::values::specified;
 
-    pub fn parse_value<'i, 't>(
+    fn parse_value<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Longhands, ParseError<'i>> {
@@ -1024,7 +1024,7 @@
         #[allow(unused_imports)]
         use crate::values::specified;
 
-        pub fn parse_value<'i, 't>(
+        fn parse_value<'i, 't>(
             context: &ParserContext,
             input: &mut Parser<'i, 't>,
         ) -> Result<Longhands, ParseError<'i>> {

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

@@ -248,7 +248,7 @@ impl AnimationValue {
         decl: &PropertyDeclaration,
         context: &mut Context,
         extra_custom_properties: Option<<&Arc<crate::custom_properties::CustomPropertiesMap>>,
-        initial: &ComputedValues
+        initial: &ComputedValues,
     ) -> Option<Self> {
         use super::PropertyDeclarationVariantRepr;
 
@@ -374,6 +374,7 @@ impl AnimationValue {
 
                     declaration.value.substitute_variables(
                         declaration.id,
+                        context.builder.writing_mode,
                         custom_properties,
                         context.quirks_mode,
                         context.device(),

servo/components/style/properties/properties.mako.rs

@@ -1194,20 +1194,18 @@ impl LonghandId {
         }
     }
 
-    // TODO(emilio): Should we use a function table like CASCADE_PROPERTY does
-    // to avoid blowing up code-size here?
     fn parse_value<'i, 't>(
         &self,
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<PropertyDeclaration, ParseError<'i>> {
-        match *self {
+        type ParsePropertyFn = for<'i, 't> fn(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<PropertyDeclaration, ParseError<'i>>;
-            % for property in data.longhands:
+        static PARSE_PROPERTY: [ParsePropertyFn; ${len(data.longhands)}] = [
-                LonghandId::${property.camel_case} => {
+        % for property in data.longhands:
-                    longhands::${property.ident}::parse_declared(context, input)
+            longhands::${property.ident}::parse_declared,
-                }
+        % endfor
-            % endfor
+        ];
-        }
+        (PARSE_PROPERTY[*self as usize])(context, input)
     }
 
     /// Returns whether this property is animatable.
@@ -1596,15 +1594,36 @@ impl ShorthandId {
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<(), ParseError<'i>> {
-        match *self {
+        type ParseIntoFn = for<'i, 't> fn(
-            % for shorthand in data.shorthands_except_all():
+            declarations: &mut SourcePropertyDeclaration,
-                ShorthandId::${shorthand.camel_case} => {
+            context: &ParserContext,
-                    shorthands::${shorthand.ident}::parse_into(declarations, context, input)
+            input: &mut Parser<'i, 't>,
-                }
+        ) -> Result<(), ParseError<'i>>;
-            % endfor
+
-            // 'all' accepts no value other than CSS-wide keywords
+        fn unreachable<'i, 't>(
-            ShorthandId::All => Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
+            _: &mut SourcePropertyDeclaration,
+            _: &ParserContext,
+            _: &mut Parser<'i, 't>
+        ) -> Result<(), ParseError<'i>> {
+            unreachable!()
         }
+
+        // 'all' accepts no value other than CSS-wide keywords
+        if *self == ShorthandId::All {
+            return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
+        }
+
+        static PARSE_INTO: [ParseIntoFn; ${len(data.shorthands)}] = [
+            % for shorthand in data.shorthands:
+            % if shorthand.ident == "all":
+            unreachable,
+            % else:
+            shorthands::${shorthand.ident}::parse_into,
+            % endif
+            % endfor
+        ];
+
+        (PARSE_INTO[*self as usize])(declarations, context, input)
     }
 }
 
@@ -1647,6 +1666,7 @@ impl UnparsedValue {
     fn substitute_variables<'cache>(
         &self,
         longhand_id: LonghandId,
+        writing_mode: WritingMode,
         custom_properties: Option<<&Arc<crate::custom_properties::CustomPropertiesMap>>,
         quirks_mode: QuirksMode,
         device: &Device,
@@ -1719,43 +1739,18 @@ impl UnparsedValue {
             Some(shorthand) => shorthand,
         };
 
-        match shorthand {
+        let mut decls = SourcePropertyDeclaration::new();
-            ShorthandId::All => {
+        // parse_into takes care of doing `parse_entirely` for us.
-                // No need to parse the 'all' shorthand as anything other
+        if shorthand.parse_into(&mut decls, &context, &mut input).is_err() {
-                // than a CSS-wide keyword, after variable substitution.
+            return invalid_at_computed_value_time();
-                return invalid_at_computed_value_time();
+        }
-            }
-            % for shorthand in data.shorthands_except_all():
-            ShorthandId::${shorthand.camel_case} => {
-                let longhands = match input.parse_entirely(|input| shorthands::${shorthand.ident}::parse_value(&context, input)) {
-                    Ok(l) => l,
-                    _ => return invalid_at_computed_value_time(),
-                };
 
-                <% seen = set() %>
+        for declaration in decls.declarations.drain(..) {
-                % for property in shorthand.sub_properties:
+            let longhand = declaration.id().as_longhand().unwrap();
-                // When animating logical properties, we end up
+            if longhand.is_logical() {
-                // physicalizing the value during the animation, but the
+                shorthand_cache.insert((shorthand, longhand.to_physical(writing_mode)), declaration.clone());
-                // 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
-                        )
-                    );
-                % endif
-                <% seen.add(prop.ident) %>
-                % endfor
-                % endfor
             }
-            % endfor
+            shorthand_cache.insert((shorthand, longhand), declaration);
         }
 
         Cow::Borrowed(&shorthand_cache[&(shorthand, longhand_id)])
@@ -2469,7 +2464,7 @@ impl PropertyDeclaration {
                             id,
                             value: Arc::new(UnparsedValue {
                                 css: css.into_owned(),
-                                first_token_type: first_token_type,
+                                first_token_type,
                                 url_data: context.url_data.clone(),
                                 from_shorthand: None,
                             }),
@@ -2506,7 +2501,7 @@ impl PropertyDeclaration {
                             crate::custom_properties::parse_non_custom_with_var(input)?;
                         let unparsed = Arc::new(UnparsedValue {
                             css: css.into_owned(),
-                            first_token_type: first_token_type,
+                            first_token_type,
                             url_data: context.url_data.clone(),
                             from_shorthand: Some(id),
                         });