From 7bd3a20ff468d58213696a6c40aa5d1e2b708e7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 3 Sep 2021 15:57:30 +0000 Subject: [PATCH] Bug 1728851 - Add attributes to the rule hash. r=boris See the discussion here: https://twitter.com/Rich_Harris/status/1433153204678799365 This should make attribute selectors roughly as fast as class selectors. I think it's worth trying and see if perf bots complain on micro-benchmarks and stylebench and such. I made attributes more specific than local names, but less specific than classes, which I think makes sense. When doing something like foo[data-bar], filtering by data-bar seems likely to yield less elements than filtering by foo. While at it, remove the bloom filter pref since we shipped it in bug 1704551 for 87 and we haven't heard complaints. Differential Revision: https://phabricator.services.mozilla.com/D124383 --- modules/libpref/init/StaticPrefList.yaml | 4 +- servo/components/style/bloom.rs | 12 +- .../components/style/gecko/selector_parser.rs | 3 +- servo/components/style/selector_map.rs | 110 +++++++++++++++--- servo/components/style/sharing/mod.rs | 5 +- servo/components/style/stylist.rs | 10 +- 6 files changed, 112 insertions(+), 32 deletions(-) diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 228201a74353..7d24e0bcdfec 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -6960,9 +6960,9 @@ mirror: always rust: true -# Whether the bloom filter optimization is applied to attribute names too, not +# Whether the rule hash is applied to attribute names too, not # only classes / id / namespaces / etc. -- name: layout.css.bloom-filter-attribute-names.enabled +- name: layout.css.bucket-attribute-names.enabled type: RelaxedAtomicBool value: true mirror: always diff --git a/servo/components/style/bloom.rs b/servo/components/style/bloom.rs index d75abaa4f932..c111454392e4 100644 --- a/servo/components/style/bloom.rs +++ b/servo/components/style/bloom.rs @@ -125,13 +125,11 @@ where element.each_class(|class| f(class.get_hash())); - if static_prefs::pref!("layout.css.bloom-filter-attribute-names.enabled") { - element.each_attr_name(|name| { - if !is_attr_name_excluded_from_filter(name) { - f(name.get_hash()) - } - }); - } + element.each_attr_name(|name| { + if !is_attr_name_excluded_from_filter(name) { + f(name.get_hash()) + } + }); } impl Drop for StyleBloom { diff --git a/servo/components/style/gecko/selector_parser.rs b/servo/components/style/gecko/selector_parser.rs index 5379454daa08..7028d554aa2d 100644 --- a/servo/components/style/gecko/selector_parser.rs +++ b/servo/components/style/gecko/selector_parser.rs @@ -249,8 +249,7 @@ impl ::selectors::SelectorImpl for SelectorImpl { type NonTSPseudoClass = NonTSPseudoClass; fn should_collect_attr_hash(name: &AtomIdent) -> bool { - static_prefs::pref!("layout.css.bloom-filter-attribute-names.enabled") && - !crate::bloom::is_attr_name_excluded_from_filter(name) + !crate::bloom::is_attr_name_excluded_from_filter(name) } } diff --git a/servo/components/style/selector_map.rs b/servo/components/style/selector_map.rs index 40806ed47afa..48ab5cdbe66b 100644 --- a/servo/components/style/selector_map.rs +++ b/servo/components/style/selector_map.rs @@ -104,10 +104,14 @@ pub struct SelectorMap { pub class_hash: MaybeCaseInsensitiveHashMap>, /// A hash from local name to rules which contain that local name selector. pub local_name_hash: PrecomputedHashMap>, + /// A hash from attributes to rules which contain that attribute selector. + pub attribute_hash: PrecomputedHashMap>, /// A hash from namespace to rules which contain that namespace selector. pub namespace_hash: PrecomputedHashMap>, /// All other rules. pub other: SmallVec<[T; 1]>, + /// Whether we should bucket by attribute names. + bucket_attributes: bool, /// The number of entries in this map. pub count: usize, } @@ -129,18 +133,29 @@ impl SelectorMap { root: SmallVec::new(), id_hash: MaybeCaseInsensitiveHashMap::new(), class_hash: MaybeCaseInsensitiveHashMap::new(), + attribute_hash: HashMap::default(), local_name_hash: HashMap::default(), namespace_hash: HashMap::default(), other: SmallVec::new(), + bucket_attributes: static_prefs::pref!("layout.css.bucket-attribute-names.enabled"), count: 0, } } + /// Trivially constructs an empty `SelectorMap`, with attribute bucketing + /// explicitly disabled. + pub fn new_without_attribute_bucketing() -> Self { + let mut ret = Self::new(); + ret.bucket_attributes = false; + ret + } + /// Clears the hashmap retaining storage. pub fn clear(&mut self) { self.root.clear(); self.id_hash.clear(); self.class_hash.clear(); + self.attribute_hash.clear(); self.local_name_hash.clear(); self.namespace_hash.clear(); self.other.clear(); @@ -218,6 +233,21 @@ impl SelectorMap { } }); + if self.bucket_attributes { + rule_hash_target.each_attr_name(|name| { + if let Some(rules) = self.attribute_hash.get(name) { + SelectorMap::get_matching_rules( + element, + rules, + matching_rules_list, + context, + flags_setter, + cascade_level, + ) + } + }); + } + if let Some(rules) = self.local_name_hash.get(rule_hash_target.local_name()) { SelectorMap::get_matching_rules( element, @@ -302,6 +332,7 @@ impl SelectorMap { .class_hash .try_entry(class.clone(), quirks_mode)? .or_insert_with(SmallVec::new), + Bucket::Attribute { name, lower_name } | Bucket::LocalName { name, lower_name } => { // If the local name in the selector isn't lowercase, // insert it into the rule hash twice. This means that, @@ -316,13 +347,19 @@ impl SelectorMap { // selector, the rulehash lookup may produce superfluous // selectors, but the subsequent selector matching work // will filter them out. + let is_attribute = matches!($bucket, Bucket::Attribute { .. }); + let hash = if is_attribute { + &mut self.attribute_hash + } else { + &mut self.local_name_hash + }; if name != lower_name { - self.local_name_hash + hash .try_entry(lower_name.clone())? .or_insert_with(SmallVec::new) .try_push($entry.clone())?; } - self.local_name_hash + hash .try_entry(name.clone())? .or_insert_with(SmallVec::new) }, @@ -338,7 +375,7 @@ impl SelectorMap { let bucket = { let mut disjoint_buckets = SmallVec::new(); - let bucket = find_bucket(entry.selector(), &mut disjoint_buckets); + let bucket = find_bucket(entry.selector(), &mut disjoint_buckets, self.bucket_attributes); // See if inserting this selector in multiple entries in the // selector map would be worth it. Consider a case like: @@ -409,8 +446,29 @@ impl SelectorMap { let mut done = false; element.each_class(|class| { - if !done { - if let Some(v) = self.class_hash.get(class, quirks_mode) { + if done { + return; + } + if let Some(v) = self.class_hash.get(class, quirks_mode) { + for entry in v.iter() { + if !f(&entry) { + done = true; + return; + } + } + } + }); + + if done { + return false; + } + + if self.bucket_attributes { + element.each_attr_name(|name| { + if done { + return; + } + if let Some(v) = self.attribute_hash.get(name) { for entry in v.iter() { if !f(&entry) { done = true; @@ -418,10 +476,11 @@ impl SelectorMap { } } } + }); + + if done { + return false; } - }); - if done { - return false; } if let Some(v) = self.local_name_hash.get(element.local_name()) { @@ -507,6 +566,10 @@ enum Bucket<'a> { name: &'a LocalName, lower_name: &'a LocalName, }, + Attribute { + name: &'a LocalName, + lower_name: &'a LocalName, + }, Class(&'a Atom), ID(&'a Atom), Root, @@ -520,9 +583,10 @@ impl<'a> Bucket<'a> { Bucket::Universal => 0, Bucket::Namespace(..) => 1, Bucket::LocalName { .. } => 2, - Bucket::Class(..) => 3, - Bucket::ID(..) => 4, - Bucket::Root => 5, + Bucket::Attribute { .. } => 3, + Bucket::Class(..) => 4, + Bucket::ID(..) => 5, + Bucket::Root => 6, } } @@ -537,11 +601,24 @@ type DisjointBuckets<'a> = SmallVec<[Bucket<'a>; 5]>; fn specific_bucket_for<'a>( component: &'a Component, disjoint_buckets: &mut DisjointBuckets<'a>, + bucket_attributes: bool, ) -> Bucket<'a> { match *component { Component::Root => Bucket::Root, Component::ID(ref id) => Bucket::ID(id), Component::Class(ref class) => Bucket::Class(class), + Component::AttributeInNoNamespace { ref local_name, .. } if bucket_attributes => Bucket::Attribute { + name: local_name, + lower_name: local_name, + }, + Component::AttributeInNoNamespaceExists { ref local_name, ref local_name_lower } if bucket_attributes => Bucket::Attribute { + name: local_name, + lower_name: local_name_lower, + }, + Component::AttributeOther(ref selector) if bucket_attributes => Bucket::Attribute { + name: &selector.local_name, + lower_name: &selector.local_name_lower, + }, Component::LocalName(ref selector) => Bucket::LocalName { name: &selector.name, lower_name: &selector.lower_name, @@ -567,14 +644,14 @@ fn specific_bucket_for<'a>( // // So inserting `span` in the rule hash makes sense since we want to // match the slotted . - Component::Slotted(ref selector) => find_bucket(selector.iter(), disjoint_buckets), - Component::Host(Some(ref selector)) => find_bucket(selector.iter(), disjoint_buckets), + Component::Slotted(ref selector) => find_bucket(selector.iter(), disjoint_buckets, bucket_attributes), + Component::Host(Some(ref selector)) => find_bucket(selector.iter(), disjoint_buckets, bucket_attributes), Component::Is(ref list) | Component::Where(ref list) => { if list.len() == 1 { - find_bucket(list[0].iter(), disjoint_buckets) + find_bucket(list[0].iter(), disjoint_buckets, bucket_attributes) } else { for selector in &**list { - let bucket = find_bucket(selector.iter(), disjoint_buckets); + let bucket = find_bucket(selector.iter(), disjoint_buckets, bucket_attributes); disjoint_buckets.push(bucket); } Bucket::Universal @@ -593,12 +670,13 @@ fn specific_bucket_for<'a>( fn find_bucket<'a>( mut iter: SelectorIter<'a, SelectorImpl>, disjoint_buckets: &mut DisjointBuckets<'a>, + bucket_attributes: bool, ) -> Bucket<'a> { let mut current_bucket = Bucket::Universal; loop { for ss in &mut iter { - let new_bucket = specific_bucket_for(ss, disjoint_buckets); + let new_bucket = specific_bucket_for(ss, disjoint_buckets, bucket_attributes); if new_bucket.more_specific_than(¤t_bucket) { current_bucket = new_bucket; } diff --git a/servo/components/style/sharing/mod.rs b/servo/components/style/sharing/mod.rs index fde72f22dc7a..4c974b388dd4 100644 --- a/servo/components/style/sharing/mod.rs +++ b/servo/components/style/sharing/mod.rs @@ -786,10 +786,7 @@ impl StyleSharingCache { } // It's possible that there are no styles for either id. - let may_match_different_id_rules = - checks::may_match_different_id_rules(shared, target.element, candidate.element); - - if may_match_different_id_rules { + if checks::may_match_different_id_rules(shared, target.element, candidate.element) { trace!("Miss: ID Attr"); return None; } diff --git a/servo/components/style/stylist.rs b/servo/components/style/stylist.rs index 547f5b48fca6..90f0ed8c0ae4 100644 --- a/servo/components/style/stylist.rs +++ b/servo/components/style/stylist.rs @@ -1986,7 +1986,15 @@ impl CascadeData { state_dependencies: ElementState::empty(), document_state_dependencies: DocumentState::empty(), mapped_ids: PrecomputedHashSet::default(), - selectors_for_cache_revalidation: SelectorMap::new(), + // NOTE: We disable attribute bucketing for revalidation because we + // rely on the buckets to match, but we don't want to just not share + // style across elements with different attributes. + // + // An alternative to this would be to perform a style sharing check + // like may_match_different_id_rules which would check that the + // attribute buckets match on all scopes. But that seems + // somewhat gnarly. + selectors_for_cache_revalidation: SelectorMap::new_without_attribute_bucketing(), animations: Default::default(), extra_data: ExtraStyleData::default(), effective_media_query_results: EffectiveMediaQueryResults::new(),