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
This commit is contained in:
Oriol Brufau 2023-05-16 23:20:00 +02:00
parent a7ca8022d3
commit e23a8bf0ad
5 changed files with 96 additions and 8 deletions

View file

@ -212,6 +212,12 @@ macro_rules! with_all_bounds {
/// pseudo-elements /// pseudo-elements
type PseudoElement: $($CommonBounds)* + PseudoElement<Impl = Self>; type PseudoElement: $($CommonBounds)* + PseudoElement<Impl = Self>;
/// 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 => { Component::Class(ref class) if quirks_mode != QuirksMode::Quirks => {
class.precomputed_hash() 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) => { Component::Is(ref list) | Component::Where(ref list) => {
// :where and :is OR their selectors, so we can't put any hash // :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 // in the filter if there's more than one selector, as that'd

View file

@ -102,6 +102,15 @@ impl<E: TElement> PushedElement<E> {
} }
} }
/// 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<E, F>(element: E, mut f: F) fn each_relevant_element_hash<E, F>(element: E, mut f: F)
where where
E: TElement, E: TElement,
@ -115,6 +124,14 @@ where
} }
element.each_class(|class| f(class.get_hash())); 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<E: TElement> Drop for StyleBloom<E> { impl<E: TElement> Drop for StyleBloom<E> {

View file

@ -519,6 +519,11 @@ pub trait TElement:
{ {
} }
/// Internal iterator for the attribute names of this element.
fn each_attr_name<F>(&self, callback: F)
where
F: FnMut(&AtomIdent);
/// Internal iterator for the part names that this element exports for a /// Internal iterator for the part names that this element exports for a
/// given part name. /// given part name.
fn each_exported_part<F>(&self, _name: &AtomIdent, _callback: F) fn each_exported_part<F>(&self, _name: &AtomIdent, _callback: F)

View file

@ -247,6 +247,11 @@ impl ::selectors::SelectorImpl for SelectorImpl {
type PseudoElement = PseudoElement; type PseudoElement = PseudoElement;
type NonTSPseudoClass = NonTSPseudoClass; 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> { impl<'a> SelectorParser<'a> {

View file

@ -570,7 +570,7 @@ impl<'le> GeckoElement<'le> {
} }
#[inline(always)] #[inline(always)]
fn attrs(&self) -> &[structs::AttrArray_InternalAttr] { fn non_mapped_attrs(&self) -> &[structs::AttrArray_InternalAttr] {
unsafe { unsafe {
let attrs = match self.0.mAttrs.mImpl.mPtr.as_ref() { let attrs = match self.0.mAttrs.mImpl.mPtr.as_ref() {
Some(attrs) => attrs, 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)] #[inline(always)]
fn get_part_attr(&self) -> Option<&structs::nsAttrValue> { fn get_part_attr(&self) -> Option<&structs::nsAttrValue> {
if !self.has_part_attr() { if !self.has_part_attr() {
return None; return None;
} }
snapshot_helpers::find_attr(self.attrs(), &atom!("part")) snapshot_helpers::find_attr(self.non_mapped_attrs(), &atom!("part"))
} }
#[inline(always)] #[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] #[inline]
@ -1167,7 +1184,7 @@ impl<'le> TElement for GeckoElement<'le> {
#[inline] #[inline]
fn exports_any_part(&self) -> bool { 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. // FIXME(emilio): we should probably just return a reference to the Atom.
@ -1177,7 +1194,25 @@ impl<'le> TElement for GeckoElement<'le> {
return None; return None;
} }
snapshot_helpers::get_id(self.attrs()) snapshot_helpers::get_id(self.non_mapped_attrs())
}
fn each_attr_name<F>(&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<F>(&self, callback: F) fn each_class<F>(&self, callback: F)
@ -1197,7 +1232,7 @@ impl<'le> TElement for GeckoElement<'le> {
where where
F: FnMut(&AtomIdent), 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<F>(&self, callback: F) fn each_part<F>(&self, callback: F)
@ -2058,7 +2093,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
return false; 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, Some(id) => id,
None => return false, None => return false,
}; };
@ -2078,7 +2113,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
#[inline] #[inline]
fn imported_part(&self, name: &AtomIdent) -> Option<AtomIdent> { fn imported_part(&self, name: &AtomIdent) -> Option<AtomIdent> {
snapshot_helpers::imported_part(self.attrs(), name) snapshot_helpers::imported_part(self.non_mapped_attrs(), name)
} }
#[inline(always)] #[inline(always)]