Skip to content

Commit

Permalink
Auto merge of #17315 - emilio:quadratic-serialization, r=SimonSapin
Browse files Browse the repository at this point in the history 
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 -->
  • Loading branch information
bors-servo committed Jun 14, 2017
2 parents 18653f6 + 5fbaf6d commit d3cc41c
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 58 deletions.
53 changes: 31 additions & 22 deletions components/style/properties/declaration_block.rs
View file
Expand Up @@ -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};
Expand Down Expand Up @@ -590,37 +591,46 @@ 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 {
// Step 3.1
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
let mut longhands = self.declarations.iter()
.filter(|d| !already_serialized.contains(&d.0.id()))
.collect::<Vec<_>>();
// 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() {
Expand All @@ -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() {
Expand All @@ -642,10 +656,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;
}
Expand Down Expand Up @@ -725,18 +739,13 @@ impl ToCss for PropertyDeclarationBlock {

for current_longhand in &current_longhands {
// Substep 9
already_serialized.push(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);
}
already_serialized.insert(current_longhand.id());
}
}
}

// Step 3.3.4
if already_serialized.contains(&property) {
if already_serialized.contains(property) {
continue;
}

Expand All @@ -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
Expand Down
77 changes: 41 additions & 36 deletions components/style/properties/properties.mako.rs
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
}
}
Expand All @@ -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,
}
}
Expand Down Expand Up @@ -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(..) => &[],
}
}

Expand Down

0 comments on commit d3cc41c

Please sign in to comment.