From 3076481c52eff296bd784a010cfdf2683107681a Mon Sep 17 00:00:00 2001 From: Zach Hoffman Date: Mon, 16 Jan 2023 11:26:41 +0000 Subject: [PATCH] style: Implement selector matching for :nth-child(An+B of selector list) and :nth-last-child(An+B of selector list) Since we have been using a single hash map to cache all :nth-child indices (with no selector list), each different selector will need its own cache. As a side note, this patch does not address invalidation. Differential Revision: https://phabricator.services.mozilla.com/D166266 --- components/selectors/context.rs | 23 ++- components/selectors/matching.rs | 168 +++++++++++++----- components/selectors/nth_index_cache.rs | 64 ++++++- components/style/dom_apis.rs | 14 +- .../invalidation/element/document_state.rs | 10 +- .../element/state_and_attributes.rs | 2 +- components/style/style_resolver.rs | 4 +- components/style/stylist.rs | 9 +- 8 files changed, 223 insertions(+), 71 deletions(-) diff --git a/components/selectors/context.rs b/components/selectors/context.rs index e29abe639a1..ffb97c94e8b 100644 --- a/components/selectors/context.rs +++ b/components/selectors/context.rs @@ -4,8 +4,8 @@ use crate::attr::CaseSensitivity; use crate::bloom::BloomFilter; -use crate::nth_index_cache::NthIndexCache; -use crate::parser::SelectorImpl; +use crate::nth_index_cache::{NthIndexCache, NthIndexCacheInner}; +use crate::parser::{Selector, SelectorImpl}; use crate::tree::{Element, OpaqueElement}; /// What kind of selector matching mode we should use. @@ -110,8 +110,8 @@ where matching_mode: MatchingMode, /// Input with the bloom filter used to fast-reject selectors. pub bloom_filter: Option<&'a BloomFilter>, - /// An optional cache to speed up nth-index-like selectors. - pub nth_index_cache: Option<&'a mut NthIndexCache>, + /// A cache to speed up nth-index-like selectors. + pub nth_index_cache: &'a mut NthIndexCache, /// The element which is going to match :scope pseudo-class. It can be /// either one :scope element, or the scoping element. /// @@ -161,7 +161,7 @@ where pub fn new( matching_mode: MatchingMode, bloom_filter: Option<&'a BloomFilter>, - nth_index_cache: Option<&'a mut NthIndexCache>, + nth_index_cache: &'a mut NthIndexCache, quirks_mode: QuirksMode, needs_selector_flags: NeedsSelectorFlags, ) -> Self { @@ -179,7 +179,7 @@ where pub fn new_for_visited( matching_mode: MatchingMode, bloom_filter: Option<&'a BloomFilter>, - nth_index_cache: Option<&'a mut NthIndexCache>, + nth_index_cache: &'a mut NthIndexCache, visited_handling: VisitedHandlingMode, quirks_mode: QuirksMode, needs_selector_flags: NeedsSelectorFlags, @@ -202,6 +202,17 @@ where } } + // Grab a reference to the appropriate cache. + #[inline] + pub fn nth_index_cache( + &mut self, + is_of_type: bool, + is_from_end: bool, + selectors: &[Selector], + ) -> &mut NthIndexCacheInner { + self.nth_index_cache.get(is_of_type, is_from_end, selectors) + } + /// Whether we're matching a nested selector. #[inline] pub fn is_nested(&self) -> bool { diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index cf8a1da09b8..e82a61f6fa6 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -7,7 +7,6 @@ use crate::attr::{ ParsedCaseSensitivity, }; use crate::bloom::{BloomFilter, BLOOM_HASH_MASK}; -use crate::nth_index_cache::NthIndexCacheInner; use crate::parser::{AncestorHashes, Combinator, Component, LocalName, NthSelectorData}; use crate::parser::{NonTSPseudoClass, Selector, SelectorImpl, SelectorIter, SelectorList}; use crate::tree::Element; @@ -328,6 +327,21 @@ where matches!(result, SelectorMatchingResult::Matched) } +/// Matches each selector of a list as a complex selector +#[inline(always)] +pub fn list_matches_complex_selector( + list: &[Selector], + element: &E, + context: &mut MatchingContext, +) -> bool { + for selector in list { + if matches_complex_selector(selector.iter(), element, context) { + return true; + } + } + false +} + /// Traverse all descendents of the given element and return true as soon as any of them match /// the given list of selectors. fn has_children_matching( @@ -802,12 +816,14 @@ where None => element.is_root(), }, Component::Nth(ref nth_data) => { - matches_generic_nth_child(element, context.shared, nth_data) - }, - Component::NthOf(ref nth_of_data) => { - // TODO(zrhoffman, bug 1808228): Use selectors() when matching - matches_generic_nth_child(element, context.shared, nth_of_data.nth_data()) + matches_generic_nth_child(element, context.shared, nth_data, &[]) }, + Component::NthOf(ref nth_of_data) => matches_generic_nth_child( + element, + context.shared, + nth_of_data.nth_data(), + nth_of_data.selectors(), + ), Component::Is(ref list) | Component::Where(ref list) => context.shared.nest(|context| { for selector in &**list { if matches_complex_selector(selector.iter(), element, context) { @@ -870,6 +886,7 @@ fn matches_generic_nth_child( element: &E, context: &mut MatchingContext, nth_data: &NthSelectorData, + selectors: &[Selector], ) -> bool where E: Element, @@ -877,19 +894,44 @@ where if element.ignores_nth_child_selectors() { return false; } + /* + * This condition could be bound as element_matches_selectors and used in + * nth_child_index() as element_matches_selectors && + * list_matches_complex_selector(selectors, &curr, context) + * , but since element_matches_selectors would need still need to be + * computed in that case in order to utilize the cache, there would be no + * performance benefit for building up nth-{,last-}child(.. of ..) caches + * where the element does not match the selector list. + */ + if !selectors.is_empty() && !list_matches_complex_selector(selectors, element, context) { + return false; + } let NthSelectorData { ty, a, b, .. } = *nth_data; let is_of_type = ty.is_of_type(); if ty.is_only() { - return matches_generic_nth_child(element, context, &NthSelectorData::first(is_of_type)) && - matches_generic_nth_child(element, context, &NthSelectorData::last(is_of_type)); + debug_assert!( + selectors.is_empty(), + ":only-child and :only-of-type cannot have a selector list!" + ); + return matches_generic_nth_child( + element, + context, + &NthSelectorData::first(is_of_type), + selectors, + ) && matches_generic_nth_child( + element, + context, + &NthSelectorData::last(is_of_type), + selectors, + ); } let is_from_end = ty.is_from_end(); // It's useful to know whether this can only select the first/last element // child for optimization purposes, see the `HAS_EDGE_CHILD_SELECTOR` flag. - let is_edge_child_selector = a == 0 && b == 1 && !is_of_type; + let is_edge_child_selector = a == 0 && b == 1 && !is_of_type && selectors.is_empty(); if context.needs_selector_flags() { element.apply_selector_flags(if is_edge_child_selector { @@ -912,25 +954,36 @@ where .is_none(); } - // Grab a reference to the appropriate cache. - let mut cache = context - .nth_index_cache - .as_mut() - .map(|c| c.get(is_of_type, is_from_end)); - // Lookup or compute the index. - let index = if let Some(i) = cache.as_mut().and_then(|c| c.lookup(element.opaque())) { + let index = if let Some(i) = context + .nth_index_cache(is_of_type, is_from_end, selectors) + .lookup(element.opaque()) + { i } else { - let i = nth_child_index(element, is_of_type, is_from_end, cache.as_deref_mut()); - if let Some(c) = cache.as_mut() { - c.insert(element.opaque(), i) - } + let i = nth_child_index( + element, + context, + selectors, + is_of_type, + is_from_end, + /* check_cache = */ true, + ); + context + .nth_index_cache(is_of_type, is_from_end, selectors) + .insert(element.opaque(), i); i }; debug_assert_eq!( index, - nth_child_index(element, is_of_type, is_from_end, None), + nth_child_index( + element, + context, + selectors, + is_of_type, + is_from_end, + /* check_cache = */ false + ), "invalid cache" ); @@ -947,9 +1000,11 @@ where #[inline] fn nth_child_index( element: &E, + context: &mut MatchingContext, + selectors: &[Selector], is_of_type: bool, is_from_end: bool, - mut cache: Option<&mut NthIndexCacheInner>, + check_cache: bool, ) -> i32 where E: Element, @@ -960,19 +1015,33 @@ where // siblings to the left checking the cache in the is_from_end case (this // matches what Gecko does). The indices-from-the-left is handled during the // regular look further below. - if let Some(ref mut c) = cache { - if is_from_end && !c.is_empty() { - let mut index: i32 = 1; - let mut curr = element.clone(); - while let Some(e) = curr.prev_sibling_element() { - curr = e; - if !is_of_type || element.is_same_type(&curr) { - if let Some(i) = c.lookup(curr.opaque()) { - return i - index; - } - index += 1; - } + if check_cache && + is_from_end && + !context + .nth_index_cache(is_of_type, is_from_end, selectors) + .is_empty() + { + let mut index: i32 = 1; + let mut curr = element.clone(); + while let Some(e) = curr.prev_sibling_element() { + curr = e; + let matches = if is_of_type { + element.is_same_type(&curr) + } else if !selectors.is_empty() { + list_matches_complex_selector(selectors, &curr, context) + } else { + true + }; + if !matches { + continue; } + if let Some(i) = context + .nth_index_cache(is_of_type, is_from_end, selectors) + .lookup(curr.opaque()) + { + return i - index; + } + index += 1; } } @@ -987,17 +1056,28 @@ where }; while let Some(e) = next(curr) { curr = e; - if !is_of_type || element.is_same_type(&curr) { - // If we're computing indices from the left, check each element in the - // cache. We handle the indices-from-the-right case at the top of this - // function. - if !is_from_end { - if let Some(i) = cache.as_mut().and_then(|c| c.lookup(curr.opaque())) { - return i + index; - } - } - index += 1; + let matches = if is_of_type { + element.is_same_type(&curr) + } else if !selectors.is_empty() { + list_matches_complex_selector(selectors, &curr, context) + } else { + true + }; + if !matches { + continue; } + // If we're computing indices from the left, check each element in the + // cache. We handle the indices-from-the-right case at the top of this + // function. + if !is_from_end && check_cache { + if let Some(i) = context + .nth_index_cache(is_of_type, is_from_end, selectors) + .lookup(curr.opaque()) + { + return i + index; + } + } + index += 1; } index diff --git a/components/selectors/nth_index_cache.rs b/components/selectors/nth_index_cache.rs index 979c3100175..b4b41578d02 100644 --- a/components/selectors/nth_index_cache.rs +++ b/components/selectors/nth_index_cache.rs @@ -2,7 +2,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use crate::tree::OpaqueElement; +use std::hash::Hash; + +use crate::{parser::Selector, tree::OpaqueElement, SelectorImpl}; use fxhash::FxHashMap; /// A cache to speed up matching of nth-index-like selectors. @@ -13,20 +15,68 @@ use fxhash::FxHashMap; #[derive(Default)] pub struct NthIndexCache { nth: NthIndexCacheInner, + nth_of_selectors: NthIndexOfSelectorsCaches, nth_last: NthIndexCacheInner, + nth_last_of_selectors: NthIndexOfSelectorsCaches, nth_of_type: NthIndexCacheInner, nth_last_of_type: NthIndexCacheInner, } impl NthIndexCache { /// Gets the appropriate cache for the given parameters. - pub fn get(&mut self, is_of_type: bool, is_from_end: bool) -> &mut NthIndexCacheInner { - match (is_of_type, is_from_end) { - (false, false) => &mut self.nth, - (false, true) => &mut self.nth_last, - (true, false) => &mut self.nth_of_type, - (true, true) => &mut self.nth_last_of_type, + pub fn get( + &mut self, + is_of_type: bool, + is_from_end: bool, + selectors: &[Selector], + ) -> &mut NthIndexCacheInner { + if is_of_type { + return if is_from_end { + &mut self.nth_last_of_type + } else { + &mut self.nth_of_type + }; } + if !selectors.is_empty() { + return if is_from_end { + self.nth_last_of_selectors.lookup(selectors) + } else { + self.nth_of_selectors.lookup(selectors) + }; + } + if is_from_end { + &mut self.nth_last + } else { + &mut self.nth + } + } +} + +#[derive(Hash, Eq, PartialEq)] +struct SelectorListCacheKey(usize); + +/// Use the selector list's pointer as the cache key +impl SelectorListCacheKey { + // :nth-child of selectors are reference-counted with `ThinArc`, so we know their pointers are stable. + fn new(selectors: &[Selector]) -> Self { + Self(selectors.as_ptr() as usize) + } +} + +/// Use a different map of cached indices per :nth-child's or :nth-last-child's selector list +#[derive(Default)] +pub struct NthIndexOfSelectorsCaches(FxHashMap); + +/// Get or insert a map of cached incides for the selector list of this +/// particular :nth-child or :nth-last-child pseudoclass +impl NthIndexOfSelectorsCaches { + pub fn lookup( + &mut self, + selectors: &[Selector], + ) -> &mut NthIndexCacheInner { + self.0 + .entry(SelectorListCacheKey::new(selectors)) + .or_default() } } diff --git a/components/style/dom_apis.rs b/components/style/dom_apis.rs index eeadc1ee53f..30d77eca187 100644 --- a/components/style/dom_apis.rs +++ b/components/style/dom_apis.rs @@ -14,7 +14,7 @@ use crate::values::AtomIdent; use selectors::attr::CaseSensitivity; use selectors::matching::{self, MatchingContext, MatchingMode, NeedsSelectorFlags}; use selectors::parser::{Combinator, Component, LocalName, SelectorImpl}; -use selectors::{Element, NthIndexCache, SelectorList}; +use selectors::{Element, SelectorList}; use smallvec::SmallVec; /// @@ -26,10 +26,12 @@ pub fn element_matches( where E: Element, { + let mut nth_index_cache = Default::default(); + let mut context = MatchingContext::new( MatchingMode::Normal, None, - None, + &mut nth_index_cache, quirks_mode, NeedsSelectorFlags::No, ); @@ -47,12 +49,12 @@ pub fn element_closest( where E: Element, { - let mut nth_index_cache = NthIndexCache::default(); + let mut nth_index_cache = Default::default(); let mut context = MatchingContext::new( MatchingMode::Normal, None, - Some(&mut nth_index_cache), + &mut nth_index_cache, quirks_mode, NeedsSelectorFlags::No, ); @@ -621,13 +623,13 @@ pub fn query_selector( { use crate::invalidation::element::invalidator::TreeStyleInvalidator; + let mut nth_index_cache = Default::default(); let quirks_mode = root.owner_doc().quirks_mode(); - let mut nth_index_cache = NthIndexCache::default(); let mut matching_context = MatchingContext::new( MatchingMode::Normal, None, - Some(&mut nth_index_cache), + &mut nth_index_cache, quirks_mode, NeedsSelectorFlags::No, ); diff --git a/components/style/invalidation/element/document_state.rs b/components/style/invalidation/element/document_state.rs index 110394549ad..b0bbce3b855 100644 --- a/components/style/invalidation/element/document_state.rs +++ b/components/style/invalidation/element/document_state.rs @@ -13,6 +13,7 @@ use crate::stylist::CascadeData; use selectors::matching::{ MatchingContext, MatchingMode, NeedsSelectorFlags, QuirksMode, VisitedHandlingMode, }; +use selectors::NthIndexCache; use style_traits::dom::DocumentState; /// A struct holding the members necessary to invalidate document state @@ -43,11 +44,16 @@ pub struct DocumentStateInvalidationProcessor<'a, E: TElement, I> { impl<'a, E: TElement, I> DocumentStateInvalidationProcessor<'a, E, I> { /// Creates a new DocumentStateInvalidationProcessor. #[inline] - pub fn new(rules: I, document_states_changed: DocumentState, quirks_mode: QuirksMode) -> Self { + pub fn new( + rules: I, + document_states_changed: DocumentState, + nth_index_cache: &'a mut NthIndexCache, + quirks_mode: QuirksMode, + ) -> Self { let mut matching_context = MatchingContext::<'a, E::Impl>::new_for_visited( MatchingMode::Normal, None, - None, + nth_index_cache, VisitedHandlingMode::AllLinksVisitedAndUnvisited, quirks_mode, NeedsSelectorFlags::No, diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index f659d81b0d1..43f7993ad70 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -65,7 +65,7 @@ impl<'a, 'b: 'a, E: TElement + 'b> StateAndAttrInvalidationProcessor<'a, 'b, E> let matching_context = MatchingContext::new_for_visited( MatchingMode::Normal, None, - Some(nth_index_cache), + nth_index_cache, VisitedHandlingMode::AllLinksVisitedAndUnvisited, shared_context.quirks_mode(), NeedsSelectorFlags::No, diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index 19a5338517d..dbd4ccb09e5 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -462,7 +462,7 @@ where let mut matching_context = MatchingContext::new_for_visited( MatchingMode::Normal, Some(bloom_filter), - Some(nth_index_cache), + nth_index_cache, visited_handling, self.context.shared.quirks_mode(), NeedsSelectorFlags::Yes, @@ -538,7 +538,7 @@ where let mut matching_context = MatchingContext::<'_, E::Impl>::new_for_visited( MatchingMode::ForStatelessPseudoElement, Some(bloom_filter), - Some(nth_index_cache), + nth_index_cache, visited_handling, self.context.shared.quirks_mode(), NeedsSelectorFlags::Yes, diff --git a/components/style/stylist.rs b/components/style/stylist.rs index f6329400b11..c0213d76c74 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1125,6 +1125,7 @@ impl Stylist { { debug_assert!(pseudo.is_lazy()); + let mut nth_index_cache = Default::default(); // No need to bother setting the selector flags when we're computing // default styles. let needs_selector_flags = if rule_inclusion == RuleInclusion::DefaultOnly { @@ -1137,7 +1138,7 @@ impl Stylist { let mut matching_context = MatchingContext::<'_, E::Impl>::new( MatchingMode::ForStatelessPseudoElement, None, - None, + &mut nth_index_cache, self.quirks_mode, needs_selector_flags, ); @@ -1166,10 +1167,12 @@ impl Stylist { let mut visited_rules = None; if parent_style.visited_style().is_some() { let mut declarations = ApplicableDeclarationList::new(); + let mut nth_index_cache = Default::default(); + let mut matching_context = MatchingContext::<'_, E::Impl>::new_for_visited( MatchingMode::ForStatelessPseudoElement, None, - None, + &mut nth_index_cache, VisitedHandlingMode::RelevantLinkVisited, self.quirks_mode, needs_selector_flags, @@ -1417,7 +1420,7 @@ impl Stylist { let mut matching_context = MatchingContext::new( MatchingMode::Normal, bloom, - Some(nth_index_cache), + nth_index_cache, self.quirks_mode, needs_selector_flags, );