From 490db1e2bd071388ce781a6b07af706f90421ca1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 17 Feb 2021 00:21:36 +0000 Subject: [PATCH] style: Reduce the amount of code generated by UnparsedValues::substitute_variables. 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 --- components/style/properties/cascade.rs | 1 + .../style/properties/declaration_block.rs | 3 +- components/style/properties/helpers.mako.rs | 6 +- .../helpers/animated_properties.mako.rs | 3 +- .../style/properties/properties.mako.rs | 103 +++++++++--------- 5 files changed, 57 insertions(+), 59 deletions(-) diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index 36e6ee4cf9b..acfdfc51457 100644 --- a/components/style/properties/cascade.rs +++ b/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(), diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 2cee5eb7322..fd41c1141cb 100644 --- a/components/style/properties/declaration_block.rs +++ b/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, diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs index 3b8e10b5125..caa941df1ff 100644 --- a/components/style/properties/helpers.mako.rs +++ b/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> { @@ -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> { diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 8b91b35a139..2f9e692f740 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -248,7 +248,7 @@ impl AnimationValue { decl: &PropertyDeclaration, context: &mut Context, extra_custom_properties: Option<<&Arc>, - initial: &ComputedValues + initial: &ComputedValues, ) -> Option { 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(), diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index abbd7992dd9..31da5e8a700 100644 --- a/components/style/properties/properties.mako.rs +++ b/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> { - match *self { - % for property in data.longhands: - LonghandId::${property.camel_case} => { - longhands::${property.ident}::parse_declared(context, input) - } - % endfor - } + type ParsePropertyFn = for<'i, 't> fn(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result>; + static PARSE_PROPERTY: [ParsePropertyFn; ${len(data.longhands)}] = [ + % for property in data.longhands: + longhands::${property.ident}::parse_declared, + % 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 { - % for shorthand in data.shorthands_except_all(): - ShorthandId::${shorthand.camel_case} => { - shorthands::${shorthand.ident}::parse_into(declarations, context, input) - } - % endfor - // 'all' accepts no value other than CSS-wide keywords - ShorthandId::All => Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)) + type ParseIntoFn = for<'i, 't> fn( + declarations: &mut SourcePropertyDeclaration, + context: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result<(), ParseError<'i>>; + + fn unreachable<'i, 't>( + _: &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>, quirks_mode: QuirksMode, device: &Device, @@ -1719,43 +1739,18 @@ impl UnparsedValue { Some(shorthand) => shorthand, }; - match shorthand { - ShorthandId::All => { - // No need to parse the 'all' shorthand as anything other - // than a CSS-wide keyword, after variable substitution. - 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(), - }; + let mut decls = SourcePropertyDeclaration::new(); + // parse_into takes care of doing `parse_entirely` for us. + if shorthand.parse_into(&mut decls, &context, &mut input).is_err() { + return invalid_at_computed_value_time(); + } - <% seen = set() %> - % for property in shorthand.sub_properties: - // 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 - ) - ); - % endif - <% seen.add(prop.ident) %> - % endfor - % endfor + for declaration in decls.declarations.drain(..) { + let longhand = declaration.id().as_longhand().unwrap(); + if longhand.is_logical() { + shorthand_cache.insert((shorthand, longhand.to_physical(writing_mode)), declaration.clone()); } - % 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), });