From 7d90cc01b6945c47348e967530fcdbff2f4bb045 Mon Sep 17 00:00:00 2001 From: S Pradeep Kumar Date: Mon, 18 Nov 2013 17:05:20 +0900 Subject: [PATCH 1/5] Insert rules in SelectorMap manually to avoid unnecessary allocation. --- src/components/style/selector_matching.rs | 34 ++++++++++++++++------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index 3585bb16b64..a3c4ce78e50 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -138,19 +138,28 @@ impl SelectorMap { // is required because Arc makes Rule non-copyable) match SelectorMap::get_id_name(rule.clone()) { Some(id_name) => { - // TODO: Is this is a wasteful allocation of a list (~[rule])? - self.id_hash.insert_or_update_with(id_name, - ~[rule.clone()], - |_, v| v.push(rule.clone())); + match self.id_hash.find_mut(&id_name) { + Some(rules) => { + rules.push(rule.clone()); + return; + } + None => {} + } + self.id_hash.insert(id_name, ~[rule.clone()]); return; } None => {} } match SelectorMap::get_class_name(rule.clone()) { Some(class_name) => { - self.class_hash.insert_or_update_with(class_name, - ~[rule.clone()], - |_, v| v.push(rule.clone())); + match self.class_hash.find_mut(&class_name) { + Some(rules) => { + rules.push(rule.clone()); + return; + } + None => {} + } + self.class_hash.insert(class_name, ~[rule.clone()]); return; } None => {} @@ -158,9 +167,14 @@ impl SelectorMap { match SelectorMap::get_element_name(rule.clone()) { Some(element_name) => { - self.element_hash.insert_or_update_with(element_name, - ~[rule.clone()], - |_, v| v.push(rule.clone())); + match self.element_hash.find_mut(&element_name) { + Some(rules) => { + rules.push(rule.clone()); + return; + } + None => {} + } + self.element_hash.insert(element_name, ~[rule.clone()]); return; } None => {} From 92f4a430ea163af3d4ab969b8a1d919ccb287d79 Mon Sep 17 00:00:00 2001 From: S Pradeep Kumar Date: Mon, 18 Nov 2013 17:31:45 +0900 Subject: [PATCH 2/5] Eliminate use of `map` by using a for loop. --- src/components/style/selector_matching.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index a3c4ce78e50..7d83819828c 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -304,8 +304,14 @@ impl Stylist { // Keeping this as a separate step because we will need it for further // optimizations regarding grouping of Rules having the same Selector. - let declarations_list = matching_rules_list.map( - |rules| rules.map(|r| r.declarations.clone())); + let mut declarations_list = ~[]; + for rules in matching_rules_list.iter() { + declarations_list.push(~[]); + let curr_declarations = &mut declarations_list[declarations_list.len() - 1]; + for rule in rules.iter() { + curr_declarations.push(rule.declarations.clone()); + } + } let mut applicable_declarations = ~[]; applicable_declarations.push_all_move(declarations_list.slice(0, 3).concat_vec()); From 1d1a2597ca0ec138889da8054ea7302b0ba8a4da Mon Sep 17 00:00:00 2001 From: S Pradeep Kumar Date: Thu, 21 Nov 2013 17:02:46 +0900 Subject: [PATCH 3/5] Avoid unnecessary cloning of Rule in SelectorMap code. Do this by receiving a borrowed pointer in get_id_name(), etc.. Also, move the received argument in insert() method (instead of cloning it). --- src/components/style/selector_matching.rs | 47 +++++++++++------------ 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index 7d83819828c..456a704547a 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -134,59 +134,56 @@ impl SelectorMap { /// Insert rule into the correct hash. /// Order in which to try: id_hash, class_hash, element_hash, universal_rules. fn insert(&mut self, rule: Rule) { - // TODO: Can we avoid the repeated clones? (IMHO, the clone() - // is required because Arc makes Rule non-copyable) - match SelectorMap::get_id_name(rule.clone()) { + match SelectorMap::get_id_name(&rule) { Some(id_name) => { match self.id_hash.find_mut(&id_name) { Some(rules) => { - rules.push(rule.clone()); + rules.push(rule); return; } None => {} } - self.id_hash.insert(id_name, ~[rule.clone()]); + self.id_hash.insert(id_name, ~[rule]); return; } None => {} } - match SelectorMap::get_class_name(rule.clone()) { + match SelectorMap::get_class_name(&rule) { Some(class_name) => { match self.class_hash.find_mut(&class_name) { Some(rules) => { - rules.push(rule.clone()); + rules.push(rule); return; } None => {} } - self.class_hash.insert(class_name, ~[rule.clone()]); + self.class_hash.insert(class_name, ~[rule]); return; } None => {} } - match SelectorMap::get_element_name(rule.clone()) { + match SelectorMap::get_element_name(&rule) { Some(element_name) => { match self.element_hash.find_mut(&element_name) { Some(rules) => { - rules.push(rule.clone()); + rules.push(rule); return; } None => {} } - self.element_hash.insert(element_name, ~[rule.clone()]); + self.element_hash.insert(element_name, ~[rule]); return; } None => {} } - self.universal_rules.push(rule.clone()); + self.universal_rules.push(rule); } /// Retrieve the first ID name in Rule, or None otherwise. - fn get_id_name(rule: Rule) -> Option<~str> { - let selector = rule.selector; - let simple_selector_sequence = &selector.compound_selectors.simple_selectors; + fn get_id_name(rule: &Rule) -> Option<~str> { + let simple_selector_sequence = &rule.selector.compound_selectors.simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { // TODO: Implement case-sensitivity based on the document type and quirks mode @@ -198,7 +195,7 @@ impl SelectorMap { } /// Retrieve the FIRST class name in Rule, or None otherwise. - fn get_class_name(rule: Rule) -> Option<~str> { + fn get_class_name(rule: &Rule) -> Option<~str> { let simple_selector_sequence = &rule.selector.compound_selectors.simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { @@ -211,7 +208,7 @@ impl SelectorMap { } /// Retrieve the name if it is a type selector, or None otherwise. - fn get_element_name(rule: Rule) -> Option<~str> { + fn get_element_name(rule: &Rule) -> Option<~str> { let simple_selector_sequence = &rule.selector.compound_selectors.simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { @@ -620,24 +617,24 @@ fn test_rule_ordering_same_specificity(){ #[test] fn test_get_id_name(){ let rules_list = get_mock_rules([".intro", "#top"]); - assert_eq!(SelectorMap::get_id_name(rules_list[0][0].clone()), None); - assert_eq!(SelectorMap::get_id_name(rules_list[1][0].clone()), Some(~"top")); + assert_eq!(SelectorMap::get_id_name(&rules_list[0][0]), None); + assert_eq!(SelectorMap::get_id_name(&rules_list[1][0]), Some(~"top")); } #[test] fn test_get_class_name(){ let rules_list = get_mock_rules([".intro.foo", "#top"]); - assert_eq!(SelectorMap::get_class_name(rules_list[0][0].clone()), Some(~"intro")); - assert_eq!(SelectorMap::get_class_name(rules_list[1][0].clone()), None); + assert_eq!(SelectorMap::get_class_name(&rules_list[0][0]), Some(~"intro")); + assert_eq!(SelectorMap::get_class_name(&rules_list[1][0]), None); } #[test] fn test_get_element_name(){ let rules_list = get_mock_rules(["img.foo", "#top", "IMG", "ImG"]); - assert_eq!(SelectorMap::get_element_name(rules_list[0][0].clone()), Some(~"img")); - assert_eq!(SelectorMap::get_element_name(rules_list[1][0].clone()), None); - assert_eq!(SelectorMap::get_element_name(rules_list[2][0].clone()), Some(~"img")); - assert_eq!(SelectorMap::get_element_name(rules_list[3][0].clone()), Some(~"img")); + assert_eq!(SelectorMap::get_element_name(&rules_list[0][0]), Some(~"img")); + assert_eq!(SelectorMap::get_element_name(&rules_list[1][0]), None); + assert_eq!(SelectorMap::get_element_name(&rules_list[2][0]), Some(~"img")); + assert_eq!(SelectorMap::get_element_name(&rules_list[3][0]), Some(~"img")); } #[test] From cae74b4f863cf8bdcf9d54bc41589d26c1f78a84 Mon Sep 17 00:00:00 2001 From: S Pradeep Kumar Date: Fri, 22 Nov 2013 11:04:47 +0900 Subject: [PATCH 4/5] Make Rule have Arc to eliminate allocation while cloning. --- src/components/style/selector_matching.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index 456a704547a..8772cdac0a8 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -124,7 +124,7 @@ impl SelectorMap { rules: &[Rule], matching_rules: &mut ~[Rule]) { for rule in rules.iter() { - if matches_selector(&rule.selector, node, pseudo_element) { + if matches_selector(rule.selector.get(), node, pseudo_element) { // TODO: Is the cloning inefficient? matching_rules.push(rule.clone()); } @@ -183,7 +183,7 @@ impl SelectorMap { /// Retrieve the first ID name in Rule, or None otherwise. fn get_id_name(rule: &Rule) -> Option<~str> { - let simple_selector_sequence = &rule.selector.compound_selectors.simple_selectors; + let simple_selector_sequence = &rule.selector.get().compound_selectors.simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { // TODO: Implement case-sensitivity based on the document type and quirks mode @@ -196,7 +196,7 @@ impl SelectorMap { /// Retrieve the FIRST class name in Rule, or None otherwise. fn get_class_name(rule: &Rule) -> Option<~str> { - let simple_selector_sequence = &rule.selector.compound_selectors.simple_selectors; + let simple_selector_sequence = &rule.selector.get().compound_selectors.simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { // TODO: Implement case-sensitivity based on the document type and quirks mode @@ -209,7 +209,7 @@ impl SelectorMap { /// Retrieve the name if it is a type selector, or None otherwise. fn get_element_name(rule: &Rule) -> Option<~str> { - let simple_selector_sequence = &rule.selector.compound_selectors.simple_selectors; + let simple_selector_sequence = &rule.selector.get().compound_selectors.simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { // HTML elements in HTML documents must be matched case-insensitively @@ -259,7 +259,7 @@ impl Stylist { for selector in style_rule.selectors.iter() { // TODO: avoid copying? rule_map.$priority.insert(Rule { - selector: selector.clone(), + selector: Arc::new(selector.clone()), declarations: Arc::new(style_rule.declarations.$priority.clone()), index: style_rule_index, stylesheet_index: self.stylesheet_index, @@ -350,7 +350,10 @@ impl PerOriginSelectorMap { #[deriving(Clone)] struct Rule { - selector: Selector, + // This is an Arc because Rule will essentially be cloned for every node + // that it matches. Selector contains an owned vector (through + // CompoundSelector) and we want to avoid the allocation. + selector: Arc, declarations: Arc<~[PropertyDeclaration]>, // Index of the parent StyleRule in the parent Stylesheet (useful for // breaking ties while cascading). @@ -363,8 +366,8 @@ struct Rule { impl Ord for Rule { #[inline] fn lt(&self, other: &Rule) -> bool { - let this_rank = (self.selector.specificity, self.stylesheet_index, self.index); - let other_rank = (other.selector.specificity, other.stylesheet_index, other.index); + let this_rank = (self.selector.get().specificity, self.stylesheet_index, self.index); + let other_rank = (other.selector.get().specificity, other.stylesheet_index, other.index); this_rank < other_rank } } @@ -589,7 +592,7 @@ fn get_rules(css_string: &str) -> ~[~[Rule]] { let mut results = ~[]; do iter_style_rules(sheet.rules.as_slice(), device) |style_rule| { results.push(style_rule.selectors.iter().map(|s| Rule { - selector: s.clone(), + selector: Arc::new(s.clone()), declarations: Arc::new(style_rule.declarations.normal.clone()), index: index, stylesheet_index: 0u, From ae1f16887d863aee545d3ea6768c66590eaa3d43 Mon Sep 17 00:00:00 2001 From: S Pradeep Kumar Date: Fri, 22 Nov 2013 17:24:32 +0900 Subject: [PATCH 5/5] Use push instead of referring to the last element. --- src/components/style/selector_matching.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index 8772cdac0a8..46252bb1643 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -303,11 +303,11 @@ impl Stylist { // optimizations regarding grouping of Rules having the same Selector. let mut declarations_list = ~[]; for rules in matching_rules_list.iter() { - declarations_list.push(~[]); - let curr_declarations = &mut declarations_list[declarations_list.len() - 1]; + let mut curr_declarations = ~[]; for rule in rules.iter() { curr_declarations.push(rule.declarations.clone()); } + declarations_list.push(curr_declarations); } let mut applicable_declarations = ~[];