From b4d05919e2b6ffe1d4cdb684c1dbf747ad840e00 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Sat, 29 Apr 2017 16:07:08 -0700 Subject: [PATCH 1/5] Unbust unit tests. Not sure how this managed to get past bors before. --- components/selectors/parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index f53380c2b22..66867000fda 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -1457,7 +1457,7 @@ pub mod tests { where V: SelectorVisitor { true } } - #[derive(PartialEq, Debug)] + #[derive(Clone, PartialEq, Debug)] pub struct DummySelectorImpl; #[derive(Default)] From 080ff126b57248898422b1118f3776863b047aef Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 27 Apr 2017 14:25:47 -0700 Subject: [PATCH 2/5] Factor out the various things stylist does per-selector into helpers. This makes things more readable, and easier to profile (since the Gecko profiler doesn't have any intra-function granularity, so I tend to make helpers non-inline when drilling down into what's expensive). It allows us to iterate the selector list once, which could help cache locality for rules with many selectors. MozReview-Commit-ID: GdFtgrbYCIu --- components/style/stylist.rs | 63 ++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index cde51a8f3fb..d26d617d8c9 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -313,38 +313,13 @@ impl Stylist { CssRule::Style(ref locked) => { let style_rule = locked.read_with(&guard); self.num_declarations += style_rule.block.read_with(&guard).len(); - for selector in &style_rule.selectors.0 { self.num_selectors += 1; - let map = if let Some(ref pseudo) = selector.pseudo_element { - self.pseudos_map - .entry(pseudo.clone()) - .or_insert_with(PerPseudoElementSelectorMap::new) - .borrow_for_origin(&stylesheet.origin) - } else { - self.element_map.borrow_for_origin(&stylesheet.origin) - }; - - map.insert(Rule::new(guard, - selector.inner.clone(), - locked.clone(), - self.rules_source_order, - selector.specificity)); + self.add_rule_to_map(guard, selector, locked, stylesheet); + self.dependencies.note_selector(selector); + self.note_for_revalidation(selector); } self.rules_source_order += 1; - - for selector in &style_rule.selectors.0 { - self.dependencies.note_selector(selector); - - if needs_revalidation(selector) { - // For revalidation, we can skip everything left of - // the first ancestor combinator. - let revalidation_sel = - selector.inner.slice_to_first_ancestor_combinator(); - - self.selectors_for_cache_revalidation.push(revalidation_sel); - } - } } CssRule::Import(ref import) => { let import = import.read_with(guard); @@ -374,6 +349,38 @@ impl Stylist { }); } + #[inline] + fn add_rule_to_map(&mut self, + guard: &SharedRwLockReadGuard, + selector: &Selector, + rule: &Arc>, + stylesheet: &Stylesheet) + { + let map = if let Some(ref pseudo) = selector.pseudo_element { + self.pseudos_map + .entry(pseudo.clone()) + .or_insert_with(PerPseudoElementSelectorMap::new) + .borrow_for_origin(&stylesheet.origin) + } else { + self.element_map.borrow_for_origin(&stylesheet.origin) + }; + + map.insert(Rule::new(guard, + selector.inner.clone(), + rule.clone(), + self.rules_source_order, + selector.specificity)); + } + + #[inline] + fn note_for_revalidation(&mut self, selector: &Selector) { + if needs_revalidation(selector) { + // For revalidation, we can skip everything left of the first ancestor + // combinator. + let revalidation_sel = selector.inner.slice_to_first_ancestor_combinator(); + self.selectors_for_cache_revalidation.push(revalidation_sel); + } + } /// Computes the style for a given "precomputed" pseudo-element, taking the /// universal rules and applying them. From 71ecff849bf7fb6e40dcb74092b36315c3ba5c33 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 27 Apr 2017 14:51:23 -0700 Subject: [PATCH 3/5] Use a single rule hash for both lower_name and name. Right now in the common case we're doing twice the work during stylist update, and also checking is_html_element_in_html_document twice during lookup. This patch aligns us with what Gecko does. MozReview-Commit-ID: D4TyG30BP8C --- components/style/stylist.rs | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index d26d617d8c9..65d04630fd3 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1050,9 +1050,6 @@ pub struct SelectorMap { pub class_hash: FnvHashMap>, /// A hash from local name to rules which contain that local name selector. pub local_name_hash: FnvHashMap>, - /// Same as local_name_hash, but keys are lower-cased. - /// For HTML elements in HTML documents. - pub lower_local_name_hash: FnvHashMap>, /// Rules that don't have ID, class, or element selectors. pub other_rules: Vec, /// Whether this hash is empty. @@ -1071,7 +1068,6 @@ impl SelectorMap { id_hash: HashMap::default(), class_hash: HashMap::default(), local_name_hash: HashMap::default(), - lower_local_name_hash: HashMap::default(), other_rules: Vec::new(), empty: true, } @@ -1120,14 +1116,9 @@ impl SelectorMap { cascade_level); }); - let local_name_hash = if element.is_html_element_in_html_document() { - &self.lower_local_name_hash - } else { - &self.local_name_hash - }; SelectorMap::get_matching_rules_from_hash(element, parent_bf, - local_name_hash, + &self.local_name_hash, element.get_local_name(), matching_rules_list, relations, @@ -1260,8 +1251,22 @@ impl SelectorMap { } if let Some(LocalNameSelector { name, lower_name }) = SelectorMap::get_local_name(&rule) { - find_push(&mut self.local_name_hash, name, rule.clone()); - find_push(&mut self.lower_local_name_hash, lower_name, rule); + // If the local name in the selector isn't lowercase, insert it into + // the rule hash twice. This means that, during lookup, we can always + // find the rules based on the local name of the element, regardless + // of whether it's an html element in an html document (in which case + // we match against lower_name) or not (in which case we match against + // name). + // + // In the case of a non-html-element-in-html-document with a + // lowercase localname and a non-lowercase selector, the rulehash + // lookup may produce superfluous selectors, but the subsequent + // selector matching work will filter them out. + if name != lower_name { + find_push(&mut self.local_name_hash, lower_name, rule.clone()); + } + find_push(&mut self.local_name_hash, name, rule); + return; } From b4a2e2f17c4618f03cfe7d6654c3332b1309a426 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 27 Apr 2017 17:52:37 -0700 Subject: [PATCH 4/5] Clean up note_selector a bit and stop handling combinators in two places. MozReview-Commit-ID: HhVi72K4yp0 --- components/style/restyle_hints.rs | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 9d3d8814ac7..8710e590421 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -18,7 +18,7 @@ use selectors::{Element, MatchAttr}; use selectors::matching::{ElementSelectorFlags, StyleRelations}; use selectors::matching::matches_selector; use selectors::parser::{AttrSelector, Combinator, Component, Selector}; -use selectors::parser::{SelectorInner, SelectorIter, SelectorMethods}; +use selectors::parser::{SelectorInner, SelectorMethods}; use selectors::visitor::SelectorVisitor; use std::clone::Clone; @@ -482,27 +482,13 @@ struct Dependency { /// of them is sensitive to attribute or state changes. struct SensitivitiesVisitor { sensitivities: Sensitivities, - hint: RestyleHint, } impl SelectorVisitor for SensitivitiesVisitor { type Impl = SelectorImpl; - - fn visit_complex_selector(&mut self, - _: SelectorIter, - combinator: Option) -> bool { - self.hint |= combinator_to_restyle_hint(combinator); - - true - } - fn visit_simple_selector(&mut self, s: &Component) -> bool { self.sensitivities.states.insert(selector_to_state(s)); - - if !self.sensitivities.attrs { - self.sensitivities.attrs = is_attr_selector(s); - } - + self.sensitivities.attrs |= is_attr_selector(s); true } } @@ -547,9 +533,9 @@ impl DependencySet { while let Some(current) = next.take() { // Set up our visitor. let mut visitor = SensitivitiesVisitor { - sensitivities: Sensitivities::new(), - hint: combinator_to_restyle_hint(combinator), + sensitivities: Sensitivities::new() }; + let mut hint = combinator_to_restyle_hint(combinator); if is_pseudo_element { // TODO(emilio): use more fancy restyle hints to avoid restyling @@ -560,7 +546,7 @@ impl DependencySet { // restyle_descendants to handle all of them (::before and // ::after, because we find them in the subtree, and other lazy // pseudos for the same reason). - visitor.hint |= RESTYLE_SELF | RESTYLE_DESCENDANTS; + hint |= RESTYLE_SELF | RESTYLE_DESCENDANTS; is_pseudo_element = false; } @@ -584,7 +570,7 @@ impl DependencySet { if !visitor.sensitivities.is_empty() { self.add_dependency(Dependency { sensitivities: visitor.sensitivities, - hint: visitor.hint, + hint: hint, selector: SelectorInner::new(current), }) } From a6ef89a7fee2e168f46722df452f44d1fc6b3d1e Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 27 Apr 2017 18:09:06 -0700 Subject: [PATCH 5/5] Make note_selector more efficient. My measurements are inconclusive as to whether this really moves the needle, but it seems like the right way to do it in any case. MozReview-Commit-ID: 7OuCc2aQ7jH --- components/style/restyle_hints.rs | 79 +++++++++++++++++-------------- 1 file changed, 43 insertions(+), 36 deletions(-) diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 8710e590421..193b7e82a9d 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -525,56 +525,63 @@ impl DependencySet { /// Adds a selector to this `DependencySet`. pub fn note_selector(&mut self, selector: &Selector) { - let mut is_pseudo_element = selector.pseudo_element.is_some(); - - let mut next = Some(selector.inner.complex.clone()); let mut combinator = None; + let mut iter = selector.inner.complex.iter(); + let mut index = 0; - while let Some(current) = next.take() { - // Set up our visitor. + loop { + let sequence_start = index; let mut visitor = SensitivitiesVisitor { sensitivities: Sensitivities::new() }; - let mut hint = combinator_to_restyle_hint(combinator); - if is_pseudo_element { - // TODO(emilio): use more fancy restyle hints to avoid restyling - // the whole subtree when pseudos change. - // - // We currently need is_pseudo_element to handle eager pseudos - // (so the style the parent stores doesn't become stale), and - // restyle_descendants to handle all of them (::before and - // ::after, because we find them in the subtree, and other lazy - // pseudos for the same reason). - hint |= RESTYLE_SELF | RESTYLE_DESCENDANTS; - is_pseudo_element = false; + // Visit all the simple selectors in this sequence. + // + // Note that this works because we can't have combinators nested + // inside simple selectors (i.e. in :not() or :-moz-any()). If we + // ever support that we'll need to visit complex selectors as well. + for ss in &mut iter { + ss.visit(&mut visitor); + index += 1; // Account for the simple selector. } - { - // Visit all the simple selectors. - let mut iter = current.iter(); - let mut index = 0usize; - for ss in &mut iter { - ss.visit(&mut visitor); - index += 1; - } - - // Prepare the next sequence of simple selectors. - if let Some(next_combinator) = iter.next_sequence() { - next = Some(current.slice_from(index + 1)); - combinator = Some(next_combinator); - } - } - - // Note what we found. + // If we found a sensitivity, add an entry in the dependency set. if !visitor.sensitivities.is_empty() { + let mut hint = combinator_to_restyle_hint(combinator); + let dep_selector; + if sequence_start == 0 { + if selector.pseudo_element.is_some() { + // TODO(emilio): use more fancy restyle hints to avoid + // restyling the whole subtree when pseudos change. + // + // We currently need is_pseudo_element to handle eager + // pseudos (so the style the parent stores doesn't + // become stale), and restyle_descendants to handle all + // of them (::before and ::after, because we find them + // in the subtree, and other lazy pseudos for the same + // reason). + hint |= RESTYLE_SELF | RESTYLE_DESCENDANTS; + } + + // Reuse the bloom hashes if this is the base selector. + dep_selector = selector.inner.clone(); + } else { + dep_selector = SelectorInner::new(selector.inner.complex.slice_from(sequence_start)); + } + self.add_dependency(Dependency { sensitivities: visitor.sensitivities, hint: hint, - selector: SelectorInner::new(current), - }) + selector: dep_selector, + }); } + combinator = iter.next_sequence(); + if combinator.is_none() { + break; + } + + index += 1; // Account for the combinator. } }