Auto merge of #17315 - emilio:quadratic-serialization, r=SimonSapin

style: Avoid quadratic time serialization of a declaration block.

At least when the longhands aren't custom properties.

We should also look into not serializing the style attribute eagerly when it's
not needed... But a lot of code currently rely on attribute values being
dereferenciables to &str, so that's harder to fix.

We should really look into all those vectors around too, but that's probably
less urgent.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17315)
<!-- Reviewable:end -->

components/style/properties/declaration_block.rs

@@ -14,6 +14,7 @@ use parser::{ParserContext, log_css_error};
 use properties::animated_properties::AnimationValue;
 use selectors::parser::SelectorParseError;
 use shared_lock::Locked;
+use smallvec::SmallVec;
 use std::fmt;
 use std::slice::Iter;
 use style_traits::{PARSING_MODE_DEFAULT, ToCss, ParseError, ParsingMode, StyleParseError};
@@ -590,7 +591,7 @@ impl ToCss for PropertyDeclarationBlock {
         // Step 1 -> dest = result list
 
         // Step 2
-        let mut already_serialized = Vec::new();
+        let mut already_serialized = PropertyDeclarationIdSet::new();
 
         // Step 3
         for &(ref declaration, importance) in &*self.declarations {
@@ -598,29 +599,38 @@ impl ToCss for PropertyDeclarationBlock {
             let property = declaration.id();
 
             // Step 3.2
-            if already_serialized.contains(&property) {
+            if already_serialized.contains(property) {
                 continue;
             }
 
             // Step 3.3
             let shorthands = declaration.shorthands();
             if !shorthands.is_empty() {
-                // Step 3.3.1
+                // Step 3.3.1 is done by checking already_serialized while
-                let mut longhands = self.declarations.iter()
+                // iterating below.
-                    .filter(|d| !already_serialized.contains(&d.0.id()))
-                    .collect::<Vec<_>>();
 
                 // Step 3.3.2
                 for &shorthand in shorthands {
                     let properties = shorthand.longhands();
 
                     // Substep 2 & 3
-                    let mut current_longhands = Vec::new();
+                    let mut current_longhands = SmallVec::<[_; 10]>::new();
                     let mut important_count = 0;
                     let mut found_system = None;
 
-                    if shorthand == ShorthandId::Font && longhands.iter().any(|&&(ref l, _)| l.get_system().is_some()) {
+                    let is_system_font =
-                        for &&(ref longhand, longhand_importance) in longhands.iter() {
+                        shorthand == ShorthandId::Font &&
+                        self.declarations.iter().any(|&(ref l, _)| {
+                            !already_serialized.contains(l.id()) &&
+                            l.get_system().is_some()
+                        });
+
+                    if is_system_font {
+                        for &(ref longhand, longhand_importance) in &self.declarations {
+                            if already_serialized.contains(longhand.id()) {
+                                continue;
+                            }
+
                             if longhand.get_system().is_some() || longhand.is_default_line_height() {
                                 current_longhands.push(longhand);
                                 if found_system.is_none() {
@@ -632,7 +642,11 @@ impl ToCss for PropertyDeclarationBlock {
                             }
                         }
                     } else {
-                        for &&(ref longhand, longhand_importance) in longhands.iter() {
+                        for &(ref longhand, longhand_importance) in &self.declarations {
+                            if already_serialized.contains(longhand.id()) {
+                                continue;
+                            }
+
                             if longhand.id().is_longhand_of(shorthand) {
                                 current_longhands.push(longhand);
                                 if longhand_importance.important() {
@@ -642,10 +656,10 @@ impl ToCss for PropertyDeclarationBlock {
                         }
                         // Substep 1:
                         //
-                         // Assuming that the PropertyDeclarationBlock contains no
+                        // Assuming that the PropertyDeclarationBlock contains no
-                         // duplicate entries, if the current_longhands length is
+                        // duplicate entries, if the current_longhands length is
-                         // equal to the properties length, it means that the
+                        // equal to the properties length, it means that the
-                         // properties that map to shorthand are present in longhands
+                        // properties that map to shorthand are present in longhands
                         if current_longhands.len() != properties.len() {
                             continue;
                         }
@@ -725,18 +739,13 @@ impl ToCss for PropertyDeclarationBlock {
 
                     for current_longhand in &current_longhands {
                         // Substep 9
-                        already_serialized.push(current_longhand.id());
+                        already_serialized.insert(current_longhand.id());
-                        let index_to_remove = longhands.iter().position(|l| l.0 == **current_longhand);
-                        if let Some(index) = index_to_remove {
-                            // Substep 10
-                            longhands.remove(index);
-                        }
                     }
                 }
             }
 
             // Step 3.3.4
-            if already_serialized.contains(&property) {
+            if already_serialized.contains(property) {
                 continue;
             }
 
@@ -756,7 +765,7 @@ impl ToCss for PropertyDeclarationBlock {
                 &mut is_first_serialization)?;
 
             // Step 3.3.8
-            already_serialized.push(property);
+            already_serialized.insert(property);
         }
 
         // Step 4

components/style/properties/properties.mako.rs

@@ -551,6 +551,42 @@ impl LonghandId {
         }
     }
 
+    fn shorthands(&self) -> &'static [ShorthandId] {
+        // first generate longhand to shorthands lookup map
+        //
+        // NOTE(emilio): This currently doesn't exclude the "all" shorthand. It
+        // could potentially do so, which would speed up serialization
+        // algorithms and what not, I guess.
+        <%
+            longhand_to_shorthand_map = {}
+            for shorthand in data.shorthands:
+                for sub_property in shorthand.sub_properties:
+                    if sub_property.ident not in longhand_to_shorthand_map:
+                        longhand_to_shorthand_map[sub_property.ident] = []
+
+                    longhand_to_shorthand_map[sub_property.ident].append(shorthand.camel_case)
+
+            for shorthand_list in longhand_to_shorthand_map.itervalues():
+                shorthand_list.sort()
+        %>
+
+        // based on lookup results for each longhand, create result arrays
+        % for property in data.longhands:
+            static ${property.ident.upper()}: &'static [ShorthandId] = &[
+                % for shorthand in longhand_to_shorthand_map.get(property.ident, []):
+                    ShorthandId::${shorthand},
+                % endfor
+            ];
+        % endfor
+
+        match *self {
+            % for property in data.longhands:
+                LonghandId::${property.camel_case} => ${property.ident.upper()},
+            % endfor
+        }
+    }
+
+
     /// If this is a logical property, return the corresponding physical one in the given writing mode.
     /// Otherwise, return unchanged.
     pub fn to_physical(&self, wm: WritingMode) -> Self {
@@ -885,7 +921,7 @@ impl<'a> PropertyDeclarationId<'a> {
             PropertyDeclarationId::Longhand(id) => {
                 match *other {
                     PropertyId::Longhand(other_id) => id == other_id,
-                    PropertyId::Shorthand(shorthand) => shorthand.longhands().contains(&id),
+                    PropertyId::Shorthand(shorthand) => self.is_longhand_of(shorthand),
                     PropertyId::Custom(_) => false,
                 }
             }
@@ -899,7 +935,7 @@ impl<'a> PropertyDeclarationId<'a> {
     /// shorthand.
     pub fn is_longhand_of(&self, shorthand: ShorthandId) -> bool {
         match *self {
-            PropertyDeclarationId::Longhand(ref id) => shorthand.longhands().contains(id),
+            PropertyDeclarationId::Longhand(ref id) => id.shorthands().contains(&shorthand),
             _ => false,
         }
     }
@@ -1308,40 +1344,9 @@ impl PropertyDeclaration {
 
     /// The shorthands that this longhand is part of.
     pub fn shorthands(&self) -> &'static [ShorthandId] {
-        // first generate longhand to shorthands lookup map
+        match self.id() {
-        <%
+            PropertyDeclarationId::Longhand(id) => id.shorthands(),
-            longhand_to_shorthand_map = {}
+            PropertyDeclarationId::Custom(..) => &[],
-            for shorthand in data.shorthands:
-                for sub_property in shorthand.sub_properties:
-                    if sub_property.ident not in longhand_to_shorthand_map:
-                        longhand_to_shorthand_map[sub_property.ident] = []
-
-                    longhand_to_shorthand_map[sub_property.ident].append(shorthand.camel_case)
-
-            for shorthand_list in longhand_to_shorthand_map.itervalues():
-                shorthand_list.sort()
-        %>
-
-        // based on lookup results for each longhand, create result arrays
-        % for property in data.longhands:
-            static ${property.ident.upper()}: &'static [ShorthandId] = &[
-                % for shorthand in longhand_to_shorthand_map.get(property.ident, []):
-                    ShorthandId::${shorthand},
-                % endfor
-            ];
-        % endfor
-
-        match *self {
-            % for property in data.longhands:
-                PropertyDeclaration::${property.camel_case}(_) => ${property.ident.upper()},
-            % endfor
-            PropertyDeclaration::CSSWideKeyword(id, _) |
-            PropertyDeclaration::WithVariables(id, _) => match id {
-                % for property in data.longhands:
-                    LonghandId::${property.camel_case} => ${property.ident.upper()},
-                % endfor
-            },
-            PropertyDeclaration::Custom(_, _) => &[]
         }
     }