Reduce allocator churn when parsing property declaration blocks (fixes #15060)

6 files changed, 81 insertions, 49 deletions

diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs
index 7595fe4bfd5..c231957d61d 100644
--- a/components/script/dom/cssstyledeclaration.rs
+++ b/components/script/dom/cssstyledeclaration.rs
@@ -193,7 +193,7 @@ impl CSSStyleDeclaration {
                                       ParserContextExtraData::default());
 
             // Step 7
-            let declarations = match declarations {
+            let mut declarations = match declarations {
                 Ok(declarations) => declarations,
                 Err(_) => return Ok(())
             };
@@ -204,7 +204,7 @@ impl CSSStyleDeclaration {
                 Some(ref lock) => {
                     let mut style_attribute = lock.write();
                     for declaration in declarations {
-                        style_attribute.set_parsed_declaration(declaration, importance);
+                        style_attribute.set_parsed_declaration(declaration.0, importance);
                     }
                     self.owner.flush_style(&style_attribute);
                 }
@@ -214,8 +214,11 @@ impl CSSStyleDeclaration {
                     } else {
                         0
                     };
+                    for decl in &mut declarations {
+                        decl.1 = importance
+                    }
                     let block = PropertyDeclarationBlock {
-                        declarations: declarations.into_iter().map(|d| (d, importance)).collect(),
+                        declarations: declarations,
                         important_count: important_count,
                     };
                     if let CSSStyleOwner::Element(ref el) = self.owner {
diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs
index dff585a1fee..001a994dbfe 100644
--- a/components/style/keyframes.rs
+++ b/components/style/keyframes.rs
@@ -325,14 +325,14 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
 
     fn parse_block(&mut self, prelude: Self::Prelude, input: &mut Parser)
                    -> Result<Self::QualifiedRule, ()> {
-        let mut declarations = Vec::new();
         let parser = KeyframeDeclarationParser {
             context: self.context,
+            declarations: vec![],
         };
         let mut iter = DeclarationListParser::new(input, parser);
         while let Some(declaration) = iter.next() {
             match declaration {
-                Ok(d) => declarations.extend(d.into_iter().map(|d| (d, Importance::Normal))),
+                Ok(_) => (),
                 Err(range) => {
                     let pos = range.start;
                     let message = format!("Unsupported keyframe property declaration: '{}'",
@@ -345,7 +345,7 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
         Ok(Arc::new(RwLock::new(Keyframe {
             selector: prelude,
             block: Arc::new(RwLock::new(PropertyDeclarationBlock {
-                declarations: declarations,
+                declarations: iter.parser.declarations,
                 important_count: 0,
             })),
         })))
@@ -354,24 +354,35 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
 
 struct KeyframeDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
+    declarations: Vec<(PropertyDeclaration, Importance)>
 }
 
 /// Default methods reject all at rules.
 impl<'a, 'b> AtRuleParser for KeyframeDeclarationParser<'a, 'b> {
     type Prelude = ();
-    type AtRule = Vec<PropertyDeclaration>;
+    type AtRule = ();
 }
 
 impl<'a, 'b> DeclarationParser for KeyframeDeclarationParser<'a, 'b> {
-    type Declaration = Vec<PropertyDeclaration>;
+    /// We parse rules directly into the declarations object
+    type Declaration = ();
 
-    fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result<Vec<PropertyDeclaration>, ()> {
+    fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result<(), ()> {
         let id = try!(PropertyId::parse(name.into()));
-        let mut results = Vec::new();
-        match PropertyDeclaration::parse(id, self.context, input, &mut results, true) {
+        let old_len = self.declarations.len();
+        match PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, true) {
             PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => {}
-            _ => return Err(())
+            _ => {
+                self.declarations.truncate(old_len);
+                return Err(());
+            }
+        }
+        // In case there is still unparsed text in the declaration, we should roll back.
+        if !input.is_exhausted() {
+            self.declarations.truncate(old_len);
+            Err(())
+        } else {
+            Ok(())
         }
-        Ok(results)
     }
 }
diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs
index 822a0924eed..e819efa40a4 100644
--- a/components/style/properties/declaration_block.rs
+++ b/components/style/properties/declaration_block.rs
@@ -495,12 +495,15 @@ pub fn parse_style_attribute(input: &str,
 
 /// Parse a given property declaration. Can result in multiple
 /// `PropertyDeclaration`s when expanding a shorthand, for example.
+///
+/// The vector returned will not have the importance set;
+/// this does not attempt to parse !important at all
 pub fn parse_one_declaration(id: PropertyId,
                              input: &str,
                              base_url: &ServoUrl,
                              error_reporter: StdBox<ParseErrorReporter + Send>,
                              extra_data: ParserContextExtraData)
-                             -> Result<Vec<PropertyDeclaration>, ()> {
+                             -> Result<Vec<(PropertyDeclaration, Importance)>, ()> {
     let context = ParserContext::new_with_extra_data(Origin::Author, base_url, error_reporter, extra_data);
     let mut results = vec![];
     match PropertyDeclaration::parse(id, &context, &mut Parser::new(input), &mut results, false) {
@@ -512,39 +515,51 @@ pub fn parse_one_declaration(id: PropertyId,
 /// A struct to parse property declarations.
 struct PropertyDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
+    declarations: Vec<(PropertyDeclaration, Importance)>
 }
 
 
 /// Default methods reject all at rules.
 impl<'a, 'b> AtRuleParser for PropertyDeclarationParser<'a, 'b> {
     type Prelude = ();
-    type AtRule = (Vec<PropertyDeclaration>, Importance);
+    type AtRule = (u32, Importance);
 }
 
 
 impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> {
-    /// A single declaration may be expanded into multiple ones if it's a
-    /// shorthand for example, so that's why this is a vector.
-    ///
-    /// TODO(emilio): Seems like there's potentially a bunch of performance work
-    /// we could do here.
-    type Declaration = (Vec<PropertyDeclaration>, Importance);
+    /// Declarations are pushed to the internal vector. This will only
+    /// let you know if the decl was !important and how many longhands
+    /// it expanded to
+    type Declaration = (u32, Importance);
 
     fn parse_value(&mut self, name: &str, input: &mut Parser)
-                   -> Result<(Vec<PropertyDeclaration>, Importance), ()> {
+                   -> Result<(u32, Importance), ()> {
         let id = try!(PropertyId::parse(name.into()));
-        let mut results = vec![];
-        try!(input.parse_until_before(Delimiter::Bang, |input| {
-            match PropertyDeclaration::parse(id, self.context, input, &mut results, false) {
+        let old_len = self.declarations.len();
+        let parse_result = input.parse_until_before(Delimiter::Bang, |input| {
+            match PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, false) {
                 PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => Ok(()),
                 _ => Err(())
             }
-        }));
+        });
+        if let Err(_) = parse_result {
+            // rollback
+            self.declarations.truncate(old_len);
+            return Err(())
+        }
         let importance = match input.try(parse_important) {
             Ok(()) => Importance::Important,
             Err(()) => Importance::Normal,
         };
-        Ok((results, importance))
+        // In case there is still unparsed text in the declaration, we should roll back.
+        if !input.is_exhausted() {
+            self.declarations.truncate(old_len);
+            return Err(())
+        }
+        for decl in &mut self.declarations[old_len..] {
+            decl.1 = importance
+        }
+        Ok(((self.declarations.len() - old_len) as u32, importance))
     }
 }
 
@@ -554,19 +569,18 @@ impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> {
 pub fn parse_property_declaration_list(context: &ParserContext,
                                        input: &mut Parser)
                                        -> PropertyDeclarationBlock {
-    let mut declarations = Vec::new();
     let mut important_count = 0;
     let parser = PropertyDeclarationParser {
         context: context,
+        declarations: vec![],
     };
     let mut iter = DeclarationListParser::new(input, parser);
     while let Some(declaration) = iter.next() {
         match declaration {
-            Ok((results, importance)) => {
+            Ok((count, importance)) => {
                 if importance.important() {
-                    important_count += results.len() as u32;
+                    important_count += count;
                 }
-                declarations.extend(results.into_iter().map(|d| (d, importance)))
             }
             Err(range) => {
                 let pos = range.start;
@@ -577,7 +591,7 @@ pub fn parse_property_declaration_list(context: &ParserContext,
         }
     }
     let mut block = PropertyDeclarationBlock {
-        declarations: declarations,
+        declarations: iter.parser.declarations,
         important_count: important_count,
     };
     super::deduplicate_property_declarations(&mut block);
diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs
index 095dd4a6439..e268c89f593 100644
--- a/components/style/properties/helpers.mako.rs
+++ b/components/style/properties/helpers.mako.rs
@@ -406,6 +406,7 @@
         use cssparser::Parser;
         use parser::ParserContext;
         use properties::{longhands, PropertyDeclaration, DeclaredValue, ShorthandId};
+        use properties::declaration_block::Importance;
         use std::fmt;
         use style_traits::ToCss;
         use super::{SerializeFlags, ALL_INHERIT, ALL_INITIAL, ALL_UNSET};
@@ -508,7 +509,7 @@
         /// `declarations` vector.
         pub fn parse(context: &ParserContext,
                      input: &mut Parser,
-                     declarations: &mut Vec<PropertyDeclaration>)
+                     declarations: &mut Vec<(PropertyDeclaration, Importance)>)
                      -> Result<(), ()> {
             input.look_for_var_functions();
             let start = input.position();
@@ -519,12 +520,12 @@
             let var = input.seen_var_functions();
             if let Ok(value) = value {
                 % for sub_property in shorthand.sub_properties:
-                    declarations.push(PropertyDeclaration::${sub_property.camel_case}(
+                    declarations.push((PropertyDeclaration::${sub_property.camel_case}(
                         match value.${sub_property.ident} {
                             Some(value) => DeclaredValue::Value(value),
                             None => DeclaredValue::Initial,
                         }
-                    ));
+                    ), Importance::Normal));
                 % endfor
                 Ok(())
             } else if var {
@@ -532,14 +533,14 @@
                 let (first_token_type, css) = try!(
                     ::custom_properties::parse_non_custom_with_var(input));
                 % for sub_property in shorthand.sub_properties:
-                    declarations.push(PropertyDeclaration::${sub_property.camel_case}(
+                    declarations.push((PropertyDeclaration::${sub_property.camel_case}(
                         DeclaredValue::WithVariables {
                             css: css.clone().into_owned(),
                             first_token_type: first_token_type,
                             base_url: context.base_url.clone(),
                             from_shorthand: Some(ShorthandId::${shorthand.camel_case}),
                         }
-                    ));
+                    ), Importance::Normal));
                 % endfor
                 Ok(())
             } else {
diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs
index 3ef6e64a427..ec6873f436a 100644
--- a/components/style/properties/properties.mako.rs
+++ b/components/style/properties/properties.mako.rs
@@ -998,8 +998,12 @@ impl PropertyDeclaration {
     /// > The <declaration-list> inside of <keyframe-block> accepts any CSS property
     /// > except those defined in this specification,
     /// > but does accept the `animation-play-state` property and interprets it specially.
+    ///
+    /// This will not actually parse Importance values, and will always set things
+    /// to Importance::Normal. Parsing Importance values is the job of PropertyDeclarationParser,
+    /// we only set them here so that we don't have to reallocate
     pub fn parse(id: PropertyId, context: &ParserContext, input: &mut Parser,
-                 result_list: &mut Vec<PropertyDeclaration>,
+                 result_list: &mut Vec<(PropertyDeclaration, Importance)>,
                  in_keyframe_block: bool)
                  -> PropertyDeclarationParseResult {
         match id {
@@ -1013,7 +1017,7 @@ impl PropertyDeclaration {
                         Err(()) => return PropertyDeclarationParseResult::InvalidValue,
                     }
                 };
-                result_list.push(PropertyDeclaration::Custom(name, value));
+                result_list.push((PropertyDeclaration::Custom(name, value), Importance::Normal));
                 return PropertyDeclarationParseResult::ValidOrIgnoredDeclaration;
             }
             PropertyId::Longhand(id) => match id {
@@ -1035,7 +1039,8 @@ impl PropertyDeclaration {
 
                         match longhands::${property.ident}::parse_declared(context, input) {
                             Ok(value) => {
-                                result_list.push(PropertyDeclaration::${property.camel_case}(value));
+                                result_list.push((PropertyDeclaration::${property.camel_case}(value),
+                                                  Importance::Normal));
                                 PropertyDeclarationParseResult::ValidOrIgnoredDeclaration
                             },
                             Err(()) => PropertyDeclarationParseResult::InvalidValue,
@@ -1065,24 +1070,24 @@ impl PropertyDeclaration {
                     match input.try(|i| CSSWideKeyword::parse(context, i)) {
                         Ok(CSSWideKeyword::InheritKeyword) => {
                             % for sub_property in shorthand.sub_properties:
-                                result_list.push(
+                                result_list.push((
                                     PropertyDeclaration::${sub_property.camel_case}(
-                                        DeclaredValue::Inherit));
+                                        DeclaredValue::Inherit), Importance::Normal));
                             % endfor
                             PropertyDeclarationParseResult::ValidOrIgnoredDeclaration
                         },
                         Ok(CSSWideKeyword::InitialKeyword) => {
                             % for sub_property in shorthand.sub_properties:
-                                result_list.push(
+                                result_list.push((
                                     PropertyDeclaration::${sub_property.camel_case}(
-                                        DeclaredValue::Initial));
+                                        DeclaredValue::Initial), Importance::Normal));
                             % endfor
                             PropertyDeclarationParseResult::ValidOrIgnoredDeclaration
                         },
                         Ok(CSSWideKeyword::UnsetKeyword) => {
                             % for sub_property in shorthand.sub_properties:
-                                result_list.push(PropertyDeclaration::${sub_property.camel_case}(
-                                        DeclaredValue::Unset));
+                                result_list.push((PropertyDeclaration::${sub_property.camel_case}(
+                                        DeclaredValue::Unset), Importance::Normal));
                             % endfor
                             PropertyDeclarationParseResult::ValidOrIgnoredDeclaration
                         },
diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs
index 70ec377cce7..39a7aa96176 100644
--- a/ports/geckolib/glue.rs
+++ b/ports/geckolib/glue.rs
@@ -724,8 +724,6 @@ pub extern "C" fn Servo_ParseProperty(property: *const nsACString, value: *const
         _ => return RawServoDeclarationBlockStrong::null(),
     }
 
-    let results = results.into_iter().map(|r| (r, Importance::Normal)).collect();
-
     Arc::new(RwLock::new(PropertyDeclarationBlock {
         declarations: results,
         important_count: 0,
@@ -863,7 +861,7 @@ fn set_property(declarations: RawServoDeclarationBlockBorrowed, property_id: Pro
         let mut declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations).write();
         let importance = if is_important { Importance::Important } else { Importance::Normal };
         for decl in decls.into_iter() {
-            declarations.set_parsed_declaration(decl, importance);
+            declarations.set_parsed_declaration(decl.0, importance);
         }
         true
     } else {