From 6623b1f80d2cdf531f94f30a9dfd00d2f6bc4eff Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Sun, 10 Sep 2017 11:33:51 -0700 Subject: [PATCH 1/5] Drive-by: Add size_of test for ComputedValues. MozReview-Commit-ID: Ks4km7R9VGr --- tests/unit/stylo/size_of.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/stylo/size_of.rs b/tests/unit/stylo/size_of.rs index 8ec8e8c0fd1..df415cfca11 100644 --- a/tests/unit/stylo/size_of.rs +++ b/tests/unit/stylo/size_of.rs @@ -31,6 +31,9 @@ fn size_of_selectors_dummy_types() { // selectors (with the inline hashes) with as few cache misses as possible. size_of_test!(test_size_of_rule, style::stylist::Rule, 32); +// Large pages generate tens of thousands of ComputedValues. +size_of_test!(test_size_of_cv, ComputedValues, 272); + size_of_test!(test_size_of_option_arc_cv, Option>, 8); size_of_test!(test_size_of_option_rule_node, Option, 8); From db67cd1759084c10b4ab54f050a21962947975f6 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 7 Sep 2017 15:06:57 -0700 Subject: [PATCH 2/5] Shorten the naming around the style sharing cache. MozReview-Commit-ID: EcVQDLoxwFP --- components/style/context.rs | 8 ++++---- components/style/sharing/mod.rs | 10 +++++----- components/style/traversal.rs | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index d7db7a5ab61..de8c95bb430 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -26,7 +26,7 @@ use selectors::matching::ElementSelectorFlags; use servo_arc::Arc; #[cfg(feature = "servo")] use servo_atoms::Atom; use shared_lock::StylesheetGuards; -use sharing::StyleSharingCandidateCache; +use sharing::StyleSharingCache; use std::fmt; use std::ops; #[cfg(feature = "servo")] use std::sync::Mutex; @@ -679,7 +679,7 @@ impl StackLimitChecker { /// thread in order to be able to mutate it without locking. pub struct ThreadLocalStyleContext { /// A cache to share style among siblings. - pub style_sharing_candidate_cache: StyleSharingCandidateCache, + pub sharing_cache: StyleSharingCache, /// The bloom filter used to fast-reject selector-matching. pub bloom_filter: StyleBloom, /// A channel on which new animations that have been triggered by style @@ -716,7 +716,7 @@ impl ThreadLocalStyleContext { #[cfg(feature = "servo")] pub fn new(shared: &SharedStyleContext) -> Self { ThreadLocalStyleContext { - style_sharing_candidate_cache: StyleSharingCandidateCache::new(), + sharing_cache: StyleSharingCache::new(), bloom_filter: StyleBloom::new(), new_animations_sender: shared.local_context_creation_data.lock().unwrap().new_animations_sender.clone(), tasks: SequentialTaskList(Vec::new()), @@ -733,7 +733,7 @@ impl ThreadLocalStyleContext { /// Creates a new `ThreadLocalStyleContext` from a shared one. pub fn new(shared: &SharedStyleContext) -> Self { ThreadLocalStyleContext { - style_sharing_candidate_cache: StyleSharingCandidateCache::new(), + sharing_cache: StyleSharingCache::new(), bloom_filter: StyleBloom::new(), tasks: SequentialTaskList(Vec::new()), selector_flags: SelectorFlagsMap::new(), diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index bf81915ce47..43095fbd781 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -343,7 +343,7 @@ impl StyleSharingTarget { &mut self, context: &mut StyleContext, ) -> StyleSharingResult { - let cache = &mut context.thread_local.style_sharing_candidate_cache; + let cache = &mut context.thread_local.sharing_cache; let shared_context = &context.shared; let selector_flags_map = &mut context.thread_local.selector_flags; let bloom_filter = &context.thread_local.bloom_filter; @@ -436,7 +436,7 @@ thread_local!(static SHARING_CACHE_KEY: StoredSharingCache = /// /// Note that this cache is flushed every time we steal work from the queue, so /// storing nodes here temporarily is safe. -pub struct StyleSharingCandidateCache { +pub struct StyleSharingCache { /// The LRU cache, with the type cast away to allow persisting the allocation. cache_typeless: OwningHandle>, /// Bind this structure to the lifetime of E, since that's what we effectively store. @@ -447,13 +447,13 @@ pub struct StyleSharingCandidateCache { dom_depth: usize, } -impl Drop for StyleSharingCandidateCache { +impl Drop for StyleSharingCache { fn drop(&mut self) { self.clear(); } } -impl StyleSharingCandidateCache { +impl StyleSharingCache { fn cache(&self) -> &SharingCache { let base: &TypelessSharingCache = &*self.cache_typeless; unsafe { mem::transmute(base) } @@ -472,7 +472,7 @@ impl StyleSharingCandidateCache { let cache = OwningHandle::new_with_fn(cache_arc, |x| unsafe { x.as_ref() }.unwrap().borrow_mut()); debug_assert_eq!(cache.num_entries(), 0); - StyleSharingCandidateCache { + StyleSharingCache { cache_typeless: cache, marker: PhantomData, dom_depth: 0, diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 4c066f87684..6ae81436972 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -658,7 +658,7 @@ where match target.share_style_if_possible(context) { StyleWasShared(index, styles) => { context.thread_local.statistics.styles_shared += 1; - context.thread_local.style_sharing_candidate_cache.touch(index); + context.thread_local.sharing_cache.touch(index); styles } CannotShare => { @@ -677,7 +677,7 @@ where }; context.thread_local - .style_sharing_candidate_cache + .sharing_cache .insert_if_possible( &element, new_styles.primary(), From c409c2089c3686d2a6a25b21eb27cef052faf421 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 11 Sep 2017 16:12:32 -0700 Subject: [PATCH 3/5] Fix an awful bug in LRUCache::touch(). MozReview-Commit-ID: 3l7L0MfdOxh --- components/style/cache.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/components/style/cache.rs b/components/style/cache.rs index 8e4c7371938..013c267c165 100644 --- a/components/style/cache.rs +++ b/components/style/cache.rs @@ -39,8 +39,7 @@ impl LRUCache { #[inline] /// Touch a given entry, putting it first in the list. pub fn touch(&mut self, pos: usize) { - let last_index = self.entries.len() - 1; - if pos != last_index { + if pos != 0 { let entry = self.entries.remove(pos).unwrap(); self.entries.push_front(entry); } From 74b4f95d71bb2dc9be8b079b5ab494fc160beaf8 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 11 Sep 2017 11:22:46 -0700 Subject: [PATCH 4/5] Encapsulate the sharing cache backend better. MozReview-Commit-ID: 2x9BIhmwH83 --- components/style/cache.rs | 10 ++ components/style/sharing/mod.rs | 216 +++++++++++++++----------------- components/style/traversal.rs | 6 +- 3 files changed, 114 insertions(+), 118 deletions(-) diff --git a/components/style/cache.rs b/components/style/cache.rs index 013c267c165..843f614f087 100644 --- a/components/style/cache.rs +++ b/components/style/cache.rs @@ -45,6 +45,16 @@ impl LRUCache { } } + /// Returns the front entry in the list (most recently used). + pub fn front(&self) -> Option<&K::Item> { + self.entries.get(0) + } + + /// Returns a mutable reference to the front entry in the list (most recently used). + pub fn front_mut(&mut self) -> Option<&mut K::Item> { + self.entries.get_mut(0) + } + /// Iterate over the contents of this cache, from more to less recently /// used. pub fn iter(&self) -> arraydeque::Iter { diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 43095fbd781..f251716b5e4 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -68,7 +68,7 @@ use Atom; use applicable_declarations::ApplicableDeclarationBlock; use atomic_refcell::{AtomicRefCell, AtomicRefMut}; use bloom::StyleBloom; -use cache::{LRUCache, LRUCacheMutIterator}; +use cache::LRUCache; use context::{SelectorFlagsMap, SharedStyleContext, StyleContext}; use data::ElementStyles; use dom::{TElement, SendElement}; @@ -342,7 +342,7 @@ impl StyleSharingTarget { pub fn share_style_if_possible( &mut self, context: &mut StyleContext, - ) -> StyleSharingResult { + ) -> Option { let cache = &mut context.thread_local.sharing_cache; let shared_context = &context.shared; let selector_flags_map = &mut context.thread_local.selector_flags; @@ -351,7 +351,7 @@ impl StyleSharingTarget { if cache.dom_depth != bloom_filter.matching_depth() { debug!("Can't share style, because DOM depth changed from {:?} to {:?}, element: {:?}", cache.dom_depth, bloom_filter.matching_depth(), self.element); - return StyleSharingResult::CannotShare; + return None; } debug_assert_eq!(bloom_filter.current_parent(), self.element.traversal_parent()); @@ -370,45 +370,58 @@ impl StyleSharingTarget { } } -/// A cache miss result. -#[derive(Clone, Debug)] -pub enum CacheMiss { - /// The parents don't match. - Parent, - /// One element was NAC, while the other wasn't. - NativeAnonymousContent, - /// The local name of the element and the candidate don't match. - LocalName, - /// The namespace of the element and the candidate don't match. - Namespace, - /// One of the element or the candidate was a link, but the other one - /// wasn't. - Link, - /// The element and the candidate match different kind of rules. This can - /// only happen in Gecko. - UserAndAuthorRules, - /// The element and the candidate are in a different state. - State, - /// The element had an id attribute, which qualifies for a unique style. - IdAttr, - /// The element had a style attribute, which qualifies for a unique style. - StyleAttr, - /// The element and the candidate class names didn't match. - Class, - /// The presentation hints didn't match. - PresHints, - /// The element and the candidate didn't match the same set of revalidation - /// selectors. - Revalidation, +struct SharingCacheBase { + entries: LRUCache<[Candidate; SHARING_CACHE_BACKING_STORE_SIZE]>, } -/// The results of attempting to share a style. -pub enum StyleSharingResult { - /// We didn't find anybody to share the style with. - CannotShare, - /// The node's style can be shared. The integer specifies the index in the - /// LRU cache that was hit and the damage that was done. - StyleWasShared(usize, ElementStyles), +impl Default for SharingCacheBase { + fn default() -> Self { + Self { + entries: LRUCache::new(), + } + } +} + +impl SharingCacheBase { + fn clear(&mut self) { + self.entries.evict_all(); + } + + fn is_empty(&self) -> bool { + self.entries.num_entries() == 0 + } +} + +impl SharingCache { + fn insert(&mut self, el: E, validation_data: ValidationData) { + self.entries.insert(StyleSharingCandidate { + element: el, + validation_data: validation_data, + }); + } + + fn lookup(&mut self, mut is_match: F) -> Option + where + F: FnMut(&mut StyleSharingCandidate) -> bool + { + let mut index = None; + for (i, candidate) in self.entries.iter_mut().enumerate() { + if is_match(candidate) { + index = Some(i); + break; + } + }; + + match index { + None => None, + Some(i) => { + self.entries.touch(i); + let front = self.entries.front_mut().unwrap(); + debug_assert!(is_match(front)); + Some(front.element.borrow_data().unwrap().styles.clone()) + } + } + } } /// Style sharing caches are are large allocations, so we store them in thread-local @@ -423,13 +436,12 @@ pub enum StyleSharingResult { /// /// [1] https://github.com/rust-lang/rust/issues/42763 /// [2] https://github.com/rust-lang/rust/issues/13707 -type SharingCacheBase = LRUCache<[Candidate; SHARING_CACHE_BACKING_STORE_SIZE]>; type SharingCache = SharingCacheBase>; type TypelessSharingCache = SharingCacheBase; type StoredSharingCache = Arc>; thread_local!(static SHARING_CACHE_KEY: StoredSharingCache = - Arc::new(AtomicRefCell::new(LRUCache::new()))); + Arc::new(AtomicRefCell::new(TypelessSharingCache::default()))); /// An LRU cache of the last few nodes seen, so that we can aggressively try to /// reuse their styles. @@ -454,6 +466,7 @@ impl Drop for StyleSharingCache { } impl StyleSharingCache { + #[allow(dead_code)] fn cache(&self) -> &SharingCache { let base: &TypelessSharingCache = &*self.cache_typeless; unsafe { mem::transmute(base) } @@ -470,7 +483,7 @@ impl StyleSharingCache { assert_eq!(mem::align_of::>(), mem::align_of::()); let cache_arc = SHARING_CACHE_KEY.with(|c| c.clone()); let cache = OwningHandle::new_with_fn(cache_arc, |x| unsafe { x.as_ref() }.unwrap().borrow_mut()); - debug_assert_eq!(cache.num_entries(), 0); + debug_assert!(cache.is_empty()); StyleSharingCache { cache_typeless: cache, @@ -479,15 +492,6 @@ impl StyleSharingCache { } } - /// Returns the number of entries in the cache. - pub fn num_entries(&self) -> usize { - self.cache().num_entries() - } - - fn iter_mut(&mut self) -> LRUCacheMutIterator> { - self.cache_mut().iter_mut() - } - /// Tries to insert an element in the style sharing cache. /// /// Fails if we know it should never be in the cache. @@ -547,20 +551,12 @@ impl StyleSharingCache { self.clear(); self.dom_depth = dom_depth; } - self.cache_mut().insert(StyleSharingCandidate { - element: *element, - validation_data: validation_data, - }); - } - - /// Touch a given index in the style sharing candidate cache. - pub fn touch(&mut self, index: usize) { - self.cache_mut().touch(index); + self.cache_mut().insert(*element, validation_data); } /// Clear the style sharing candidate cache. pub fn clear(&mut self) { - self.cache_mut().evict_all() + self.cache_mut().clear(); } /// Attempts to share a style with another node. @@ -570,48 +566,33 @@ impl StyleSharingCache { selector_flags_map: &mut SelectorFlagsMap, bloom_filter: &StyleBloom, target: &mut StyleSharingTarget, - ) -> StyleSharingResult { + ) -> Option { if shared_context.options.disable_style_sharing_cache { debug!("{:?} Cannot share style: style sharing cache disabled", target.element); - return StyleSharingResult::CannotShare + return None; } if target.traversal_parent().is_none() { debug!("{:?} Cannot share style: element has no parent", target.element); - return StyleSharingResult::CannotShare + return None; } if target.is_native_anonymous() { debug!("{:?} Cannot share style: NAC", target.element); - return StyleSharingResult::CannotShare; + return None; } - for (i, candidate) in self.iter_mut().enumerate() { - let sharing_result = - Self::test_candidate( - target, - candidate, - &shared_context, - bloom_filter, - selector_flags_map - ); - - match sharing_result { - Ok(shared_styles) => { - return StyleSharingResult::StyleWasShared(i, shared_styles) - } - Err(miss) => { - debug!("Cache miss: {:?}", miss); - } - } - } - - debug!("{:?} Cannot share style: {} cache entries", target.element, - self.cache().num_entries()); - - StyleSharingResult::CannotShare + self.cache_mut().lookup(|candidate| { + Self::test_candidate( + target, + candidate, + &shared_context, + bloom_filter, + selector_flags_map + ) + }) } fn test_candidate( @@ -620,13 +601,7 @@ impl StyleSharingCache { shared: &SharedStyleContext, bloom: &StyleBloom, selector_flags_map: &mut SelectorFlagsMap - ) -> Result { - macro_rules! miss { - ($miss: ident) => { - return Err(CacheMiss::$miss); - } - } - + ) -> bool { // Check that we have the same parent, or at least that the parents // share styles and permit sharing across their children. The latter // check allows us to share style between cousins if the parents @@ -635,37 +610,44 @@ impl StyleSharingCache { let candidate_parent = candidate.element.traversal_parent(); if parent != candidate_parent && !checks::can_share_style_across_parents(parent, candidate_parent) { - miss!(Parent) + trace!("Miss: Parent"); + return false; } if target.is_native_anonymous() { debug_assert!(!candidate.element.is_native_anonymous(), "Why inserting NAC into the cache?"); - miss!(NativeAnonymousContent) + trace!("Miss: Native Anonymous Content"); + return false; } if *target.get_local_name() != *candidate.element.get_local_name() { - miss!(LocalName) + trace!("Miss: Local Name"); + return false; } if *target.get_namespace() != *candidate.element.get_namespace() { - miss!(Namespace) + trace!("Miss: Namespace"); + return false; } if target.is_link() != candidate.element.is_link() { - miss!(Link) + trace!("Miss: Link"); + return false; } if target.matches_user_and_author_rules() != candidate.element.matches_user_and_author_rules() { - miss!(UserAndAuthorRules) + trace!("Miss: User and Author Rules"); + return false; } // We do not ignore visited state here, because Gecko // needs to store extra bits on visited style contexts, // so these contexts cannot be shared if target.element.get_state() != candidate.get_state() { - miss!(State) + trace!("Miss: User and Author State"); + return false; } let element_id = target.element.get_id(); @@ -674,32 +656,38 @@ impl StyleSharingCache { // It's possible that there are no styles for either id. if checks::may_have_rules_for_ids(shared, element_id.as_ref(), candidate_id.as_ref()) { - miss!(IdAttr) + trace!("Miss: ID Attr"); + return false; } } if !checks::have_same_style_attribute(target, candidate) { - miss!(StyleAttr) + trace!("Miss: Style Attr"); + return false; } if !checks::have_same_class(target, candidate) { - miss!(Class) + trace!("Miss: Class"); + return false; } if !checks::have_same_presentational_hints(target, candidate) { - miss!(PresHints) + trace!("Miss: Pres Hints"); + return false; } if !checks::revalidate(target, candidate, shared, bloom, selector_flags_map) { - miss!(Revalidation) + trace!("Miss: Revalidation"); + return false; } - let data = candidate.element.borrow_data().unwrap(); - debug_assert!(target.has_current_styles_for_traversal(&data, shared.traversal_flags)); + debug_assert!(target.has_current_styles_for_traversal( + &candidate.element.borrow_data().unwrap(), + shared.traversal_flags) + ); + debug!("Sharing allowed between {:?} and {:?}", target.element, candidate.element); - debug!("Sharing style between {:?} and {:?}", - target.element, candidate.element); - Ok(data.styles.clone()) + true } } diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 6ae81436972..9fb6be98a22 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -624,7 +624,6 @@ where E: TElement, { use data::RestyleKind::*; - use sharing::StyleSharingResult::*; context.thread_local.statistics.elements_styled += 1; let kind = data.restyle_kind(context.shared); @@ -656,12 +655,11 @@ where // Now that our bloom filter is set up, try the style sharing // cache. match target.share_style_if_possible(context) { - StyleWasShared(index, styles) => { + Some(styles) => { context.thread_local.statistics.styles_shared += 1; - context.thread_local.sharing_cache.touch(index); styles } - CannotShare => { + None => { context.thread_local.statistics.elements_matched += 1; // Perform the matching and cascading. let new_styles = { From 8268217767ffb5480b05007a1a2a126df0a1ae51 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 11 Sep 2017 13:54:28 -0700 Subject: [PATCH 5/5] Avoid memmoving ValidationData more than necessary. MozReview-Commit-ID: 70w3bZ2FU0W --- components/style/sharing/mod.rs | 11 +++++++---- components/style/traversal.rs | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index f251716b5e4..e42a4a8b19e 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -393,10 +393,10 @@ impl SharingCacheBase { } impl SharingCache { - fn insert(&mut self, el: E, validation_data: ValidationData) { + fn insert(&mut self, el: E, validation_data_holder: &mut StyleSharingTarget) { self.entries.insert(StyleSharingCandidate { element: el, - validation_data: validation_data, + validation_data: validation_data_holder.take_validation_data(), }); } @@ -495,10 +495,13 @@ impl StyleSharingCache { /// Tries to insert an element in the style sharing cache. /// /// Fails if we know it should never be in the cache. + /// + /// NB: We pass a source for the validation data, rather than the data itself, + /// to avoid memmoving at each function call. See rust issue #42763. pub fn insert_if_possible(&mut self, element: &E, style: &ComputedValues, - validation_data: ValidationData, + validation_data_holder: &mut StyleSharingTarget, dom_depth: usize) { let parent = match element.traversal_parent() { Some(element) => element, @@ -551,7 +554,7 @@ impl StyleSharingCache { self.clear(); self.dom_depth = dom_depth; } - self.cache_mut().insert(*element, validation_data); + self.cache_mut().insert(*element, validation_data_holder); } /// Clear the style sharing candidate cache. diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 9fb6be98a22..efe7d8f68b3 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -679,7 +679,7 @@ where .insert_if_possible( &element, new_styles.primary(), - target.take_validation_data(), + &mut target, context.thread_local.bloom_filter.matching_depth(), );