From 8b20d7a98232fd8007fce4296fbd48503e57efa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Jun 2017 11:40:49 +0200 Subject: [PATCH 1/3] 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. --- .../style/properties/declaration_block.rs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 0cc0aeca2fc3..c332b4b07da6 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -590,7 +590,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,7 +598,7 @@ impl ToCss for PropertyDeclarationBlock { let property = declaration.id(); // Step 3.2 - if already_serialized.contains(&property) { + if already_serialized.contains(property) { continue; } @@ -606,8 +606,10 @@ impl ToCss for PropertyDeclarationBlock { let shorthands = declaration.shorthands(); if !shorthands.is_empty() { // Step 3.3.1 + // + // FIXME(emilio): All this looks terribly inefficient... let mut longhands = self.declarations.iter() - .filter(|d| !already_serialized.contains(&d.0.id())) + .filter(|d| !already_serialized.contains(d.0.id())) .collect::>(); // Step 3.3.2 @@ -642,10 +644,10 @@ impl ToCss for PropertyDeclarationBlock { } // Substep 1: // - // Assuming that the PropertyDeclarationBlock contains no - // duplicate entries, if the current_longhands length is - // equal to the properties length, it means that the - // properties that map to shorthand are present in longhands + // Assuming that the PropertyDeclarationBlock contains no + // duplicate entries, if the current_longhands length is + // equal to the properties length, it means that the + // properties that map to shorthand are present in longhands if current_longhands.len() != properties.len() { continue; } @@ -725,7 +727,7 @@ impl ToCss for PropertyDeclarationBlock { for current_longhand in ¤t_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 @@ -736,7 +738,7 @@ impl ToCss for PropertyDeclarationBlock { } // Step 3.3.4 - if already_serialized.contains(&property) { + if already_serialized.contains(property) { continue; } @@ -756,7 +758,7 @@ impl ToCss for PropertyDeclarationBlock { &mut is_first_serialization)?; // Step 3.3.8 - already_serialized.push(property); + already_serialized.insert(property); } // Step 4 From 6e8560185863c08cce720c73cfab1bb2409fbf4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Jun 2017 12:53:49 +0200 Subject: [PATCH 2/3] style: Avoid extra allocations in serialization of a property declaration block. Concretely we avoid allocating and scanning a temporary vector of longhands not yet serialized for each shorthand. This doesn't save a lot of time in Linux, but I bet it's somewhat important on OSX. --- .../style/properties/declaration_block.rs | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index c332b4b07da6..efbccd9f9917 100644 --- a/components/style/properties/declaration_block.rs +++ b/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}; @@ -605,24 +606,31 @@ impl ToCss for PropertyDeclarationBlock { // Step 3.3 let shorthands = declaration.shorthands(); if !shorthands.is_empty() { - // Step 3.3.1 - // - // FIXME(emilio): All this looks terribly inefficient... - let mut longhands = self.declarations.iter() - .filter(|d| !already_serialized.contains(d.0.id())) - .collect::>(); + // Step 3.3.1 is done by checking already_serialized while + // iterating below. // 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()) { - for &&(ref longhand, longhand_importance) in longhands.iter() { + let is_system_font = + 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() { @@ -634,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() { @@ -728,11 +740,6 @@ impl ToCss for PropertyDeclarationBlock { for current_longhand in ¤t_longhands { // Substep 9 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); - } } } } From 5fbaf6da5fe2407a13924e6667882a469e79e574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 14 Jun 2017 12:55:50 +0200 Subject: [PATCH 3/3] style: Speed up LonghandId::is_longhand_of. By looking at the shorthands, not the other way around, given a longhand uses to not appear in multiple longhands. This makes the testcase go to 430ms on my machine. This could be even faster having a static LonghandIdSet per shorthand, I guess, but this is not done in this PR. --- .../style/properties/properties.mako.rs | 77 ++++++++++--------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 44d0d55a1c3d..6b0e26d81fcc 100644 --- a/components/style/properties/properties.mako.rs +++ b/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 - <% - 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: - 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(_, _) => &[] + match self.id() { + PropertyDeclarationId::Longhand(id) => id.shorthands(), + PropertyDeclarationId::Custom(..) => &[], } }