From 8b6c5988b53a662318c3d2214eb3c4abc97b7750 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 20 Sep 2017 18:38:20 -0700 Subject: [PATCH 1/5] Hoist the LRU cache into its own crate to share it with selectors. --- Cargo.lock | 9 +++++++++ components/lru_cache/Cargo.toml | 13 +++++++++++++ components/{style/cache.rs => lru_cache/lib.rs} | 2 ++ components/selectors/Cargo.toml | 1 + components/selectors/lib.rs | 1 + components/style/Cargo.toml | 1 + components/style/context.rs | 2 +- components/style/lib.rs | 2 +- components/style/sharing/mod.rs | 2 +- 9 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 components/lru_cache/Cargo.toml rename components/{style/cache.rs => lru_cache/lib.rs} (99%) diff --git a/Cargo.lock b/Cargo.lock index 0d771ea371d..aec56362afe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1704,6 +1704,13 @@ name = "log" version = "0.3.8" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "lru_cache" +version = "0.0.1" +dependencies = [ + "arrayvec 0.3.23 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "lzw" version = "0.10.0" @@ -2712,6 +2719,7 @@ dependencies = [ "cssparser 0.21.2 (registry+https://github.com/rust-lang/crates.io-index)", "fnv 1.0.5 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)", + "lru_cache 0.0.1", "malloc_size_of 0.0.1", "malloc_size_of_derive 0.0.1", "matches 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", @@ -3128,6 +3136,7 @@ dependencies = [ "kernel32-sys 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 0.2.8 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)", + "lru_cache 0.0.1", "malloc_size_of 0.0.1", "malloc_size_of_derive 0.0.1", "matches 0.1.4 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/components/lru_cache/Cargo.toml b/components/lru_cache/Cargo.toml new file mode 100644 index 00000000000..7e8f0693f7e --- /dev/null +++ b/components/lru_cache/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "lru_cache" +version = "0.0.1" +authors = ["The Servo Project Developers"] +license = "MPL-2.0" +publish = false # We should publish this at some point + +[lib] +name = "lru_cache" +path = "lib.rs" + +[dependencies] +arrayvec = "0.3.20" diff --git a/components/style/cache.rs b/components/lru_cache/lib.rs similarity index 99% rename from components/style/cache.rs rename to components/lru_cache/lib.rs index d43a4a71955..ffffca9a579 100644 --- a/components/style/cache.rs +++ b/components/lru_cache/lib.rs @@ -4,6 +4,8 @@ //! A simple LRU cache. +extern crate arrayvec; + use arrayvec::{Array, ArrayVec}; /// A LRU cache using a statically-sized array for storage. diff --git a/components/selectors/Cargo.toml b/components/selectors/Cargo.toml index 0fa01f994b3..da9adffde93 100644 --- a/components/selectors/Cargo.toml +++ b/components/selectors/Cargo.toml @@ -30,6 +30,7 @@ log = "0.3" fnv = "1.0" malloc_size_of = { path = "../malloc_size_of" } malloc_size_of_derive = { path = "../malloc_size_of_derive" } +lru_cache = { path = "../lru_cache" } phf = "0.7.18" precomputed-hash = "0.1" servo_arc = { path = "../servo_arc" } diff --git a/components/selectors/lib.rs b/components/selectors/lib.rs index 870e1c3ca0c..2184dfc64e7 100644 --- a/components/selectors/lib.rs +++ b/components/selectors/lib.rs @@ -10,6 +10,7 @@ #[macro_use] extern crate log; #[macro_use] extern crate matches; extern crate fnv; +extern crate lru_cache; extern crate malloc_size_of; #[macro_use] extern crate malloc_size_of_derive; extern crate phf; diff --git a/components/style/Cargo.toml b/components/style/Cargo.toml index 44062782184..d88ae34cd78 100644 --- a/components/style/Cargo.toml +++ b/components/style/Cargo.toml @@ -48,6 +48,7 @@ itertools = "0.5" itoa = "0.3" html5ever = {version = "0.19", optional = true} lazy_static = "0.2" +lru_cache = { path = "../lru_cache" } log = "0.3" malloc_size_of = { path = "../malloc_size_of", optional=true } malloc_size_of_derive = { path = "../malloc_size_of_derive", optional=true } diff --git a/components/style/context.rs b/components/style/context.rs index e88382e4cd4..390ac09a5c4 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -8,13 +8,13 @@ #[cfg(feature = "servo")] use animation::PropertyAnimation; use app_units::Au; use bloom::StyleBloom; -use cache::{Entry, LRUCache}; use data::{EagerPseudoStyles, ElementData}; use dom::{OpaqueNode, TNode, TElement, SendElement}; use euclid::ScaleFactor; use euclid::Size2D; use fnv::FnvHashMap; use font_metrics::FontMetricsProvider; +use lru_cache::{Entry, LRUCache}; #[cfg(feature = "gecko")] use gecko_bindings::structs; use parallel::{STACK_SAFETY_MARGIN_KB, STYLE_THREAD_STACK_SIZE_KB}; #[cfg(feature = "servo")] use parking_lot::RwLock; diff --git a/components/style/lib.rs b/components/style/lib.rs index 5ae4f1633a6..af150229f79 100644 --- a/components/style/lib.rs +++ b/components/style/lib.rs @@ -59,6 +59,7 @@ extern crate itoa; extern crate lazy_static; #[macro_use] extern crate log; +extern crate lru_cache; #[cfg(feature = "gecko")] #[macro_use] extern crate malloc_size_of; #[cfg(feature = "gecko")] #[macro_use] extern crate malloc_size_of_derive; #[allow(unused_extern_crates)] @@ -102,7 +103,6 @@ pub mod applicable_declarations; #[cfg(feature = "servo")] pub mod attr; pub mod bezier; pub mod bloom; -pub mod cache; pub mod context; pub mod counter_style; pub mod custom_properties; diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 72214d79f90..0c652265c0e 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -68,9 +68,9 @@ use Atom; use applicable_declarations::ApplicableDeclarationBlock; use atomic_refcell::{AtomicRefCell, AtomicRefMut}; use bloom::StyleBloom; -use cache::{LRUCache, Entry}; use context::{SelectorFlagsMap, SharedStyleContext, StyleContext}; use dom::{TElement, SendElement}; +use lru_cache::{LRUCache, Entry}; use matching::MatchMethods; use owning_ref::OwningHandle; use properties::ComputedValues; From 13a3cf27a8ed3ef486a5759bf2afb7b6a32b1626 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 20 Sep 2017 19:00:15 -0700 Subject: [PATCH 2/5] LRUCache::new -> LRUCache::default. MozReview-Commit-ID: KouOaYTluRx --- components/lru_cache/lib.rs | 7 ++++--- components/style/context.rs | 2 +- components/style/sharing/mod.rs | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/components/lru_cache/lib.rs b/components/lru_cache/lib.rs index ffffca9a579..ada1ad3a950 100644 --- a/components/lru_cache/lib.rs +++ b/components/lru_cache/lib.rs @@ -33,9 +33,8 @@ pub struct Entry { next: u16, } -impl>> LRUCache { - /// Create an empty LRU cache. - pub fn new() -> Self { +impl>> Default for LRUCache { + fn default() -> Self { let cache = LRUCache { entries: ArrayVec::new(), head: 0, @@ -44,7 +43,9 @@ impl>> LRUCache { assert!(cache.entries.capacity() < u16::max_value() as usize, "Capacity overflow"); cache } +} +impl>> LRUCache { /// Returns the number of elements in the cache. pub fn num_entries(&self) -> usize { self.entries.len() diff --git a/components/style/context.rs b/components/style/context.rs index 390ac09a5c4..8b26754e0cb 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -546,7 +546,7 @@ impl SelectorFlagsMap { pub fn new() -> Self { SelectorFlagsMap { map: FnvHashMap::default(), - cache: LRUCache::new(), + cache: LRUCache::default(), } } diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 0c652265c0e..e6dabc7f153 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -415,7 +415,7 @@ struct SharingCacheBase { impl Default for SharingCacheBase { fn default() -> Self { Self { - entries: LRUCache::new(), + entries: LRUCache::default(), } } } From 05c03d5104ae6d0790c153e4fa8df7ac7015db18 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 20 Sep 2017 19:07:12 -0700 Subject: [PATCH 3/5] Hoist lookup() into lru_cache. MozReview-Commit-ID: F5FbKFqpXEh --- components/lru_cache/lib.rs | 25 +++++++++++++++++++++++++ components/style/sharing/mod.rs | 27 ++------------------------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/components/lru_cache/lib.rs b/components/lru_cache/lib.rs index ada1ad3a950..53a165f2dde 100644 --- a/components/lru_cache/lib.rs +++ b/components/lru_cache/lib.rs @@ -89,6 +89,31 @@ impl>> LRUCache { } } + /// Performs a lookup on the cache with the given test routine. Touches + /// the result on a hit. + pub fn lookup(&mut self, mut test_one: F) -> Option + where + F: FnMut(&mut T) -> Option + { + let mut result = None; + for (i, candidate) in self.iter_mut() { + if let Some(r) = test_one(candidate) { + result = Some((i, r)); + break; + } + }; + + match result { + None => None, + Some((i, r)) => { + self.touch(i); + let front = self.front_mut().unwrap(); + debug_assert!(test_one(front).is_some()); + Some(r) + } + } + } + /// Insert a given key in the cache. pub fn insert(&mut self, val: T) { let entry = Entry { val, prev: 0, next: 0 }; diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index e6dabc7f153..c1b843993a7 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -442,29 +442,6 @@ impl SharingCache { }; self.entries.insert(StyleSharingCandidate { element, validation_data }); } - - fn lookup(&mut self, mut test_one: F) -> Option - where - F: FnMut(&mut StyleSharingCandidate) -> Option - { - let mut result = None; - for (i, candidate) in self.entries.iter_mut() { - if let Some(r) = test_one(candidate) { - result = Some((i, r)); - break; - } - }; - - match result { - None => None, - Some((i, r)) => { - self.entries.touch(i); - let front = self.entries.front_mut().unwrap(); - debug_assert!(test_one(front).is_some()); - Some(r) - } - } - } } /// Style sharing caches are are large allocations, so we store them in thread-local @@ -638,7 +615,7 @@ impl StyleSharingCache { return None; } - self.cache_mut().lookup(|candidate| { + self.cache_mut().entries.lookup(|candidate| { Self::test_candidate( target, candidate, @@ -755,7 +732,7 @@ impl StyleSharingCache { return None; } - self.cache_mut().lookup(|candidate| { + self.cache_mut().entries.lookup(|candidate| { debug_assert_ne!(candidate.element, target); if !candidate.parent_style_identity().eq(inherited) { return None; From 48466bf8761d3a9c6064eb2b39fc74e790e26cb7 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 20 Sep 2017 12:43:29 -0700 Subject: [PATCH 4/5] Introduce an NthIndexCache type and pipe it from ThreadLocalStyleContext to MatchingContext. Some future refactoring here to pass fewer things as parameters would be nice. --- components/script/dom/element.rs | 6 ++++-- components/script/dom/node.rs | 7 +++++-- components/selectors/context.rs | 13 ++++++++++++- components/selectors/matching.rs | 17 +++++++++-------- components/style/context.rs | 5 +++++ .../style/invalidation/element/invalidator.rs | 8 ++++++-- components/style/sharing/checks.rs | 16 +++++++++++++--- components/style/sharing/mod.rs | 14 +++++++++++++- components/style/style_resolver.rs | 4 ++++ components/style/stylist.rs | 15 +++++++++++---- ports/geckolib/glue.rs | 2 +- 11 files changed, 83 insertions(+), 24 deletions(-) diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 195884bebab..76990dc9aa6 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -2188,7 +2188,8 @@ impl ElementMethods for Element { Err(_) => Err(Error::Syntax), Ok(selectors) => { let quirks_mode = document_from_node(self).quirks_mode(); - let mut ctx = MatchingContext::new(MatchingMode::Normal, None, + // FIXME(bholley): Consider an nth-index cache here. + let mut ctx = MatchingContext::new(MatchingMode::Normal, None, None, quirks_mode); Ok(matches_selector_list(&selectors, &Root::from_ref(self), &mut ctx)) } @@ -2209,7 +2210,8 @@ impl ElementMethods for Element { for element in root.inclusive_ancestors() { if let Some(element) = Root::downcast::(element) { let quirks_mode = document_from_node(self).quirks_mode(); - let mut ctx = MatchingContext::new(MatchingMode::Normal, None, + // FIXME(bholley): Consider an nth-index cache here. + let mut ctx = MatchingContext::new(MatchingMode::Normal, None, None, quirks_mode); if matches_selector_list(&selectors, &element, &mut ctx) { return Ok(Some(element)); diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index b83cdcbd37e..6b16cc242db 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -363,7 +363,9 @@ impl<'a> Iterator for QuerySelectorIterator { self.iterator.by_ref().filter_map(|node| { // TODO(cgaebel): Is it worth it to build a bloom filter here // (instead of passing `None`)? Probably. - let mut ctx = MatchingContext::new(MatchingMode::Normal, None, + // + // FIXME(bholley): Consider an nth-index cache here. + let mut ctx = MatchingContext::new(MatchingMode::Normal, None, None, node.owner_doc().quirks_mode()); if let Some(element) = Root::downcast(node) { if matches_selector_list(selectors, &element, &mut ctx) { @@ -754,7 +756,8 @@ impl Node { Err(_) => Err(Error::Syntax), // Step 3. Ok(selectors) => { - let mut ctx = MatchingContext::new(MatchingMode::Normal, None, + // FIXME(bholley): Consider an nth-index cache here. + let mut ctx = MatchingContext::new(MatchingMode::Normal, None, None, self.owner_doc().quirks_mode()); Ok(self.traverse_preorder().filter_map(Root::downcast).find(|element| { matches_selector_list(&selectors, element, &mut ctx) diff --git a/components/selectors/context.rs b/components/selectors/context.rs index 31ce04b37a1..46991d349ce 100644 --- a/components/selectors/context.rs +++ b/components/selectors/context.rs @@ -69,15 +69,22 @@ impl QuirksMode { } } +/// A cache to speed up matching of nth-index-like selectors. +/// +/// NB: This is just a dummy type right now, it will be fleshed out in later patches. +#[derive(Default)] +pub struct NthIndexCache(usize); + /// Data associated with the matching process for a element. This context is /// used across many selectors for an element, so it's not appropriate for /// transient data that applies to only a single selector. -#[derive(Clone)] pub struct MatchingContext<'a> { /// Input with the matching mode we should use when matching selectors. pub 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. + nth_index_cache: Option<&'a mut NthIndexCache>, /// Input that controls how matching for links is handled. pub visited_handling: VisitedHandlingMode, /// Output that records whether we encountered a "relevant link" while @@ -94,12 +101,14 @@ impl<'a> MatchingContext<'a> { /// Constructs a new `MatchingContext`. pub fn new(matching_mode: MatchingMode, bloom_filter: Option<&'a BloomFilter>, + nth_index_cache: Option<&'a mut NthIndexCache>, quirks_mode: QuirksMode) -> Self { Self { matching_mode: matching_mode, bloom_filter: bloom_filter, + nth_index_cache: nth_index_cache, visited_handling: VisitedHandlingMode::AllLinksUnvisited, relevant_link_found: false, quirks_mode: quirks_mode, @@ -110,6 +119,7 @@ impl<'a> MatchingContext<'a> { /// Constructs a new `MatchingContext` for use in visited matching. pub fn new_for_visited(matching_mode: MatchingMode, bloom_filter: Option<&'a BloomFilter>, + nth_index_cache: Option<&'a mut NthIndexCache>, visited_handling: VisitedHandlingMode, quirks_mode: QuirksMode) -> Self @@ -119,6 +129,7 @@ impl<'a> MatchingContext<'a> { bloom_filter: bloom_filter, visited_handling: visited_handling, relevant_link_found: false, + nth_index_cache: nth_index_cache, quirks_mode: quirks_mode, classes_and_ids_case_sensitivity: quirks_mode.classes_and_ids_case_sensitivity(), } diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index b9b2da857d8..fad3ce181ae 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -724,26 +724,26 @@ fn matches_simple_selector( element.is_empty() } Component::NthChild(a, b) => { - matches_generic_nth_child(element, a, b, false, false, flags_setter) + matches_generic_nth_child(element, context, a, b, false, false, flags_setter) } Component::NthLastChild(a, b) => { - matches_generic_nth_child(element, a, b, false, true, flags_setter) + matches_generic_nth_child(element, context, a, b, false, true, flags_setter) } Component::NthOfType(a, b) => { - matches_generic_nth_child(element, a, b, true, false, flags_setter) + matches_generic_nth_child(element, context, a, b, true, false, flags_setter) } Component::NthLastOfType(a, b) => { - matches_generic_nth_child(element, a, b, true, true, flags_setter) + matches_generic_nth_child(element, context, a, b, true, true, flags_setter) } Component::FirstOfType => { - matches_generic_nth_child(element, 0, 1, true, false, flags_setter) + matches_generic_nth_child(element, context, 0, 1, true, false, flags_setter) } Component::LastOfType => { - matches_generic_nth_child(element, 0, 1, true, true, flags_setter) + matches_generic_nth_child(element, context, 0, 1, true, true, flags_setter) } Component::OnlyOfType => { - matches_generic_nth_child(element, 0, 1, true, false, flags_setter) && - matches_generic_nth_child(element, 0, 1, true, true, flags_setter) + matches_generic_nth_child(element, context, 0, 1, true, false, flags_setter) && + matches_generic_nth_child(element, context, 0, 1, true, true, flags_setter) } Component::Negation(ref negated) => { context.nesting_level += 1; @@ -767,6 +767,7 @@ fn select_name<'a, T>(is_html: bool, local_name: &'a T, local_name_lower: &'a T) #[inline] fn matches_generic_nth_child(element: &E, + context: &mut LocalMatchingContext, a: i32, b: i32, is_of_type: bool, diff --git a/components/style/context.rs b/components/style/context.rs index 8b26754e0cb..ff2551c0b54 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -23,6 +23,7 @@ use properties::ComputedValues; use rule_cache::RuleCache; use rule_tree::StrongRuleNode; use selector_parser::{EAGER_PSEUDO_COUNT, SnapshotMap}; +use selectors::context::NthIndexCache; use selectors::matching::ElementSelectorFlags; use servo_arc::Arc; #[cfg(feature = "servo")] use servo_atoms::Atom; @@ -719,6 +720,8 @@ pub struct ThreadLocalStyleContext { /// A checker used to ensure that parallel.rs does not recurse indefinitely /// even on arbitrarily deep trees. See Gecko bug 1376883. pub stack_limit_checker: StackLimitChecker, + /// A cache for nth-index-like selectors. + pub nth_index_cache: NthIndexCache, } impl ThreadLocalStyleContext { @@ -737,6 +740,7 @@ impl ThreadLocalStyleContext { font_metrics_provider: E::FontMetricsProvider::create_from(shared), stack_limit_checker: StackLimitChecker::new( (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024), + nth_index_cache: NthIndexCache::default(), } } @@ -754,6 +758,7 @@ impl ThreadLocalStyleContext { font_metrics_provider: E::FontMetricsProvider::create_from(shared), stack_limit_checker: StackLimitChecker::new( (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024), + nth_index_cache: NthIndexCache::default(), } } diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index 5338bf22e3a..2080ba7676e 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -633,10 +633,12 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> debug!("TreeStyleInvalidator::process_invalidation({:?}, {:?}, {:?})", self.element, invalidation, invalidation_kind); + // FIXME(bholley): Consider passing an nth-index cache here. let mut context = MatchingContext::new_for_visited( MatchingMode::Normal, None, + None, VisitedHandlingMode::AllLinksVisitedAndUnvisited, self.shared_context.quirks_mode(), ); @@ -946,12 +948,14 @@ impl<'a, 'b: 'a, E> InvalidationCollector<'a, 'b, E> // whether any parent had a snapshot, and whether those snapshots were // taken due to an element class/id change, but it's not clear it'd be // worth it. + // + // FIXME(bholley): Consider passing an nth-index cache here. let mut now_context = - MatchingContext::new_for_visited(MatchingMode::Normal, None, + MatchingContext::new_for_visited(MatchingMode::Normal, None, None, VisitedHandlingMode::AllLinksUnvisited, self.shared_context.quirks_mode()); let mut then_context = - MatchingContext::new_for_visited(MatchingMode::Normal, None, + MatchingContext::new_for_visited(MatchingMode::Normal, None, None, VisitedHandlingMode::AllLinksUnvisited, self.shared_context.quirks_mode()); diff --git a/components/style/sharing/checks.rs b/components/style/sharing/checks.rs index d5607590cd3..2120bbc6f8c 100644 --- a/components/style/sharing/checks.rs +++ b/components/style/sharing/checks.rs @@ -10,6 +10,7 @@ use Atom; use bloom::StyleBloom; use context::{SelectorFlagsMap, SharedStyleContext}; use dom::TElement; +use selectors::context::NthIndexCache; use sharing::{StyleSharingCandidate, StyleSharingTarget}; /// Determines whether a target and a candidate have compatible parents for sharing. @@ -99,16 +100,25 @@ pub fn revalidate(target: &mut StyleSharingTarget, candidate: &mut StyleSharingCandidate, shared_context: &SharedStyleContext, bloom: &StyleBloom, + nth_index_cache: &mut NthIndexCache, selector_flags_map: &mut SelectorFlagsMap) -> bool where E: TElement, { let stylist = &shared_context.stylist; - let for_element = - target.revalidation_match_results(stylist, bloom, selector_flags_map); + let for_element = target.revalidation_match_results( + stylist, + bloom, + nth_index_cache, + selector_flags_map, + ); - let for_candidate = candidate.revalidation_match_results(stylist, bloom); + let for_candidate = candidate.revalidation_match_results( + stylist, + bloom, + nth_index_cache, + ); // This assert "ensures", to some extent, that the two candidates have // matched the same rulehash buckets, and as such, that the bits we're diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index c1b843993a7..8e2c9763b1d 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -75,6 +75,7 @@ use matching::MatchMethods; use owning_ref::OwningHandle; use properties::ComputedValues; use rule_tree::StrongRuleNode; +use selectors::context::NthIndexCache; use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode}; use servo_arc::{Arc, NonZeroPtrMut}; use smallbitvec::SmallBitVec; @@ -203,6 +204,7 @@ impl ValidationData { element: E, stylist: &Stylist, bloom: &StyleBloom, + nth_index_cache: &mut NthIndexCache, bloom_known_valid: bool, flags_setter: &mut F ) -> &SmallBitVec @@ -230,6 +232,7 @@ impl ValidationData { self.revalidation_match_results = Some(stylist.match_revalidation_selectors(&element, bloom_to_use, + nth_index_cache, flags_setter)); } @@ -289,11 +292,13 @@ impl StyleSharingCandidate { &mut self, stylist: &Stylist, bloom: &StyleBloom, + nth_index_cache: &mut NthIndexCache, ) -> &SmallBitVec { self.validation_data.revalidation_match_results( self.element, stylist, bloom, + nth_index_cache, /* bloom_known_valid = */ false, &mut |_, _| {}) } @@ -346,6 +351,7 @@ impl StyleSharingTarget { &mut self, stylist: &Stylist, bloom: &StyleBloom, + nth_index_cache: &mut NthIndexCache, selector_flags_map: &mut SelectorFlagsMap ) -> &SmallBitVec { // It's important to set the selector flags. Otherwise, if we succeed in @@ -372,6 +378,7 @@ impl StyleSharingTarget { self.element, stylist, bloom, + nth_index_cache, /* bloom_known_valid = */ true, &mut set_selector_flags) } @@ -385,6 +392,7 @@ impl StyleSharingTarget { let shared_context = &context.shared; let selector_flags_map = &mut context.thread_local.selector_flags; let bloom_filter = &context.thread_local.bloom_filter; + let nth_index_cache = &mut context.thread_local.nth_index_cache; if cache.dom_depth != bloom_filter.matching_depth() { debug!("Can't share style, because DOM depth changed from {:?} to {:?}, element: {:?}", @@ -398,6 +406,7 @@ impl StyleSharingTarget { shared_context, selector_flags_map, bloom_filter, + nth_index_cache, self ) } @@ -596,6 +605,7 @@ impl StyleSharingCache { shared_context: &SharedStyleContext, selector_flags_map: &mut SelectorFlagsMap, bloom_filter: &StyleBloom, + nth_index_cache: &mut NthIndexCache, target: &mut StyleSharingTarget, ) -> Option { if shared_context.options.disable_style_sharing_cache { @@ -621,6 +631,7 @@ impl StyleSharingCache { candidate, &shared_context, bloom_filter, + nth_index_cache, selector_flags_map ) }) @@ -631,6 +642,7 @@ impl StyleSharingCache { candidate: &mut StyleSharingCandidate, shared: &SharedStyleContext, bloom: &StyleBloom, + nth_index_cache: &mut NthIndexCache, selector_flags_map: &mut SelectorFlagsMap ) -> Option { // Check that we have the same parent, or at least that the parents @@ -705,7 +717,7 @@ impl StyleSharingCache { } if !checks::revalidate(target, candidate, shared, bloom, - selector_flags_map) { + nth_index_cache, selector_flags_map) { trace!("Miss: Revalidation"); return None; } diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index ee66b56a1d9..8c026a28d83 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -413,10 +413,12 @@ where let map = &mut self.context.thread_local.selector_flags; let bloom_filter = self.context.thread_local.bloom_filter.filter(); + let nth_index_cache = &mut self.context.thread_local.nth_index_cache; let mut matching_context = MatchingContext::new_for_visited( MatchingMode::Normal, Some(bloom_filter), + Some(nth_index_cache), visited_handling, self.context.shared.quirks_mode(), ); @@ -486,11 +488,13 @@ where } let bloom_filter = self.context.thread_local.bloom_filter.filter(); + let nth_index_cache = &mut self.context.thread_local.nth_index_cache; let mut matching_context = MatchingContext::new_for_visited( MatchingMode::ForStatelessPseudoElement, Some(bloom_filter), + Some(nth_index_cache), visited_handling, self.context.shared.quirks_mode(), ); diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 60b3d1b520d..b398ff851bc 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -30,7 +30,7 @@ use selector_map::{PrecomputedHashMap, SelectorMap, SelectorMapEntry}; use selector_parser::{SelectorImpl, PerPseudoElementMap, PseudoElement}; use selectors::attr::NamespaceConstraint; use selectors::bloom::{BloomFilter, NonCountingBloomFilter}; -use selectors::matching::{ElementSelectorFlags, matches_selector, MatchingContext, MatchingMode}; +use selectors::matching::{ElementSelectorFlags, matches_selector, MatchingContext, MatchingMode, NthIndexCache}; use selectors::matching::VisitedHandlingMode; use selectors::parser::{AncestorHashes, Combinator, Component, Selector}; use selectors::parser::{SelectorIter, SelectorMethods}; @@ -1027,6 +1027,7 @@ impl Stylist { let mut declarations = ApplicableDeclarationList::new(); let mut matching_context = MatchingContext::new(MatchingMode::ForStatelessPseudoElement, + None, None, self.quirks_mode); @@ -1061,6 +1062,7 @@ impl Stylist { MatchingContext::new_for_visited( MatchingMode::ForStatelessPseudoElement, None, + None, VisitedHandlingMode::RelevantLinkVisited, self.quirks_mode, ); @@ -1203,7 +1205,7 @@ impl Stylist { V: Push + VecLike, { let mut matching_context = - MatchingContext::new(MatchingMode::Normal, None, self.quirks_mode); + MatchingContext::new(MatchingMode::Normal, None, None, self.quirks_mode); let mut dummy_flag_setter = |_: &E, _: ElementSelectorFlags| {}; let rule_hash_target = element.rule_hash_target(); @@ -1427,6 +1429,7 @@ impl Stylist { &self, element: &E, bloom: Option<&BloomFilter>, + nth_index_cache: &mut NthIndexCache, flags_setter: &mut F ) -> SmallBitVec where @@ -1435,8 +1438,12 @@ impl Stylist { { // NB: `MatchingMode` doesn't really matter, given we don't share style // between pseudos. - let mut matching_context = - MatchingContext::new(MatchingMode::Normal, bloom, self.quirks_mode); + let mut matching_context = MatchingContext::new( + MatchingMode::Normal, + bloom, + Some(nth_index_cache), + self.quirks_mode + ); // Note that, by the time we're revalidating, we're guaranteed that the // candidate and the entry have the same id, classes, and local name. diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index eccfc810cfe..0409ead88f5 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -1503,7 +1503,7 @@ pub extern "C" fn Servo_StyleRule_SelectorMatchesElement(rule: RawServoStyleRule }; let element = GeckoElement(element); - let mut ctx = MatchingContext::new(matching_mode, None, element.owner_document_quirks_mode()); + let mut ctx = MatchingContext::new(matching_mode, None, None, element.owner_document_quirks_mode()); matches_selector(selector, 0, None, &element, &mut ctx, &mut |_, _| {}) }) } From ab9edf3d69354e7aee87374bc7d9e63ba518049b Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 20 Sep 2017 13:50:05 -0700 Subject: [PATCH 5/5] Hoist index computation into a helper. MozReview-Commit-ID: ALeggMDh92m --- components/selectors/matching.rs | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index fad3ce181ae..afff6cd1e92 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -787,6 +787,27 @@ fn matches_generic_nth_child(element: &E, HAS_SLOW_SELECTOR_LATER_SIBLINGS }); + let index = nth_child_index(element, is_of_type, is_from_end); + + // Is there a non-negative integer n such that An+B=index? + match index.checked_sub(b) { + None => false, + Some(an) => match an.checked_div(a) { + Some(n) => n >= 0 && a * n == an, + None /* a == 0 */ => an == 0, + }, + } +} + +#[inline] +fn nth_child_index( + element: &E, + is_of_type: bool, + is_from_end: bool, +) -> i32 +where + E: Element, +{ let mut index: i32 = 1; let mut next_sibling = if is_from_end { element.next_sibling_element() @@ -815,14 +836,7 @@ fn matches_generic_nth_child(element: &E, }; } - // Is there a non-negative integer n such that An+B=index? - match index.checked_sub(b) { - None => false, - Some(an) => match an.checked_div(a) { - Some(n) => n >= 0 && a * n == an, - None /* a == 0 */ => an == 0, - }, - } + index } #[inline]