From e23a8bf0ad5fcfc23395b74dccb571fe360ee0bb Mon Sep 17 00:00:00 2001 From: Oriol Brufau Date: Tue, 16 May 2023 23:20:00 +0200 Subject: [PATCH] style: Add attribute names to the bloom filter Safari does this. This reduces the runtime in the example linked from comment 0 quite a lot (40ms on a local opt build, from ~130ms on a release nightly build). I added a pref because there's a slight chance of performance regressions on pages that do not use attribute selectors, as we're now doing more unconditional work per element (adding the attributes to the bloom filter). But the trade-off should be worth it, I think. Differential Revision: https://phabricator.services.mozilla.com/D111689 --- components/selectors/parser.rs | 26 ++++++++++++ components/style/bloom.rs | 17 ++++++++ components/style/dom.rs | 5 +++ components/style/gecko/selector_parser.rs | 5 +++ components/style/gecko/wrapper.rs | 51 +++++++++++++++++++---- 5 files changed, 96 insertions(+), 8 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index e99b1b18412..73c1239035c 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -212,6 +212,12 @@ macro_rules! with_all_bounds { /// pseudo-elements type PseudoElement: $($CommonBounds)* + PseudoElement; + + /// Whether attribute hashes should be collected for filtering + /// purposes. + fn should_collect_attr_hash(_name: &Self::LocalName) -> bool { + false + } } } } @@ -482,6 +488,26 @@ where Component::Class(ref class) if quirks_mode != QuirksMode::Quirks => { class.precomputed_hash() }, + Component::AttributeInNoNamespace { ref local_name, .. } if Impl::should_collect_attr_hash(local_name) => { + // AttributeInNoNamespace is only used when local_name == + // local_name_lower. + local_name.precomputed_hash() + }, + Component::AttributeInNoNamespaceExists { ref local_name, ref local_name_lower, .. } => { + // Only insert the local-name into the filter if it's all + // lowercase. Otherwise we would need to test both hashes, and + // our data structures aren't really set up for that. + if local_name != local_name_lower || !Impl::should_collect_attr_hash(local_name) { + continue; + } + local_name.precomputed_hash() + }, + Component::AttributeOther(ref selector) => { + if selector.local_name != selector.local_name_lower || !Impl::should_collect_attr_hash(&selector.local_name) { + continue; + } + selector.local_name.precomputed_hash() + }, Component::Is(ref list) | Component::Where(ref list) => { // :where and :is OR their selectors, so we can't put any hash // in the filter if there's more than one selector, as that'd diff --git a/components/style/bloom.rs b/components/style/bloom.rs index c17b31d1bee..d75abaa4f93 100644 --- a/components/style/bloom.rs +++ b/components/style/bloom.rs @@ -102,6 +102,15 @@ impl PushedElement { } } +/// Returns whether the attribute name is excluded from the bloom filter. +/// +/// We do this for attributes that are very common but not commonly used in +/// selectors. +#[inline] +pub fn is_attr_name_excluded_from_filter(atom: &crate::Atom) -> bool { + *atom == atom!("class") || *atom == atom!("id") || *atom == atom!("style") +} + fn each_relevant_element_hash(element: E, mut f: F) where E: TElement, @@ -115,6 +124,14 @@ 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()) + } + }); + } } impl Drop for StyleBloom { diff --git a/components/style/dom.rs b/components/style/dom.rs index c23b985a628..f16e45ce681 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -519,6 +519,11 @@ pub trait TElement: { } + /// Internal iterator for the attribute names of this element. + fn each_attr_name(&self, callback: F) + where + F: FnMut(&AtomIdent); + /// Internal iterator for the part names that this element exports for a /// given part name. fn each_exported_part(&self, _name: &AtomIdent, _callback: F) diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index a9083581234..b2fe46a9a2c 100644 --- a/components/style/gecko/selector_parser.rs +++ b/components/style/gecko/selector_parser.rs @@ -247,6 +247,11 @@ impl ::selectors::SelectorImpl for SelectorImpl { type PseudoElement = PseudoElement; 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) + } } impl<'a> SelectorParser<'a> { diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 7d4b4e03f73..3f23981a823 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -570,7 +570,7 @@ impl<'le> GeckoElement<'le> { } #[inline(always)] - fn attrs(&self) -> &[structs::AttrArray_InternalAttr] { + fn non_mapped_attrs(&self) -> &[structs::AttrArray_InternalAttr] { unsafe { let attrs = match self.0.mAttrs.mImpl.mPtr.as_ref() { Some(attrs) => attrs, @@ -581,12 +581,29 @@ impl<'le> GeckoElement<'le> { } } + #[inline(always)] + fn mapped_attrs(&self) -> &[structs::AttrArray_InternalAttr] { + unsafe { + let attrs = match self.0.mAttrs.mImpl.mPtr.as_ref() { + Some(attrs) => attrs, + None => return &[], + }; + + let attrs = match attrs.mMappedAttrs.as_ref() { + Some(attrs) => attrs, + None => return &[], + }; + + attrs.mBuffer.as_slice(attrs.mAttrCount as usize) + } + } + #[inline(always)] fn get_part_attr(&self) -> Option<&structs::nsAttrValue> { if !self.has_part_attr() { return None; } - snapshot_helpers::find_attr(self.attrs(), &atom!("part")) + snapshot_helpers::find_attr(self.non_mapped_attrs(), &atom!("part")) } #[inline(always)] @@ -602,7 +619,7 @@ impl<'le> GeckoElement<'le> { } } - snapshot_helpers::find_attr(self.attrs(), &atom!("class")) + snapshot_helpers::find_attr(self.non_mapped_attrs(), &atom!("class")) } #[inline] @@ -1167,7 +1184,7 @@ impl<'le> TElement for GeckoElement<'le> { #[inline] fn exports_any_part(&self) -> bool { - snapshot_helpers::find_attr(self.attrs(), &atom!("exportparts")).is_some() + snapshot_helpers::find_attr(self.non_mapped_attrs(), &atom!("exportparts")).is_some() } // FIXME(emilio): we should probably just return a reference to the Atom. @@ -1177,7 +1194,25 @@ impl<'le> TElement for GeckoElement<'le> { return None; } - snapshot_helpers::get_id(self.attrs()) + snapshot_helpers::get_id(self.non_mapped_attrs()) + } + + fn each_attr_name(&self, mut callback: F) + where + F: FnMut(&AtomIdent), + { + for attr in self.non_mapped_attrs().iter().chain(self.mapped_attrs().iter()) { + let is_nodeinfo = attr.mName.mBits & 1 != 0; + unsafe { + let atom = if is_nodeinfo { + let node_info = &*((attr.mName.mBits & !1) as *const structs::NodeInfo); + node_info.mInner.mName + } else { + attr.mName.mBits as *const nsAtom + }; + AtomIdent::with(atom, |a| callback(a)) + } + } } fn each_class(&self, callback: F) @@ -1197,7 +1232,7 @@ impl<'le> TElement for GeckoElement<'le> { where F: FnMut(&AtomIdent), { - snapshot_helpers::each_exported_part(self.attrs(), name, callback) + snapshot_helpers::each_exported_part(self.non_mapped_attrs(), name, callback) } fn each_part(&self, callback: F) @@ -2058,7 +2093,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { return false; } - let element_id = match snapshot_helpers::get_id(self.attrs()) { + let element_id = match snapshot_helpers::get_id(self.non_mapped_attrs()) { Some(id) => id, None => return false, }; @@ -2078,7 +2113,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { #[inline] fn imported_part(&self, name: &AtomIdent) -> Option { - snapshot_helpers::imported_part(self.attrs(), name) + snapshot_helpers::imported_part(self.non_mapped_attrs(), name) } #[inline(always)]