From f9205440b5cf50220bf1cc9a84eeb1eb22f99e8b Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 5 Jun 2017 16:31:52 -0400 Subject: [PATCH] Parent mismatch should not clear style sharing cache. We can have cousins in the cache whose parent doesn't match ours, and other cousins whose parent does. When we encounter one of the former, that's not a reason to stop lookin for the latter. Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1369620 --- components/style/bloom.rs | 6 ++++++ components/style/matching.rs | 4 +++- components/style/sharing/mod.rs | 33 ++++++++++++++++++++------------- 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/components/style/bloom.rs b/components/style/bloom.rs index 8ffe2d2a968..46df21261bc 100644 --- a/components/style/bloom.rs +++ b/components/style/bloom.rs @@ -122,6 +122,12 @@ impl StyleBloom { self.elements.is_empty() } + /// Returns the DOM depth of elements that can be correctly + /// matched against the bloom filter (that is, the number of + /// elements in our list). + pub fn matching_depth(&self) -> usize { + self.elements.len() + } /// Clears the bloom filter. pub fn clear(&mut self) { diff --git a/components/style/matching.rs b/components/style/matching.rs index 24ac00e4abc..19a731f138c 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -883,12 +883,14 @@ pub trait MatchMethods : TElement { .validation_data .take(); + let dom_depth = context.thread_local.bloom_filter.matching_depth(); context.thread_local .style_sharing_candidate_cache .insert_if_possible(self, data.styles().primary.values(), primary_results.relations, - validation_data); + validation_data, + dom_depth); } child_cascade_requirement diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index a6598ec6769..f996538fb6b 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -326,15 +326,20 @@ impl StyleSharingTarget { data: &mut ElementData) -> StyleSharingResult { + let cache = &mut context.thread_local.style_sharing_candidate_cache; let shared_context = &context.shared; let selector_flags_map = &mut context.thread_local.selector_flags; let bloom_filter = &context.thread_local.bloom_filter; + 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; + } debug_assert_eq!(bloom_filter.current_parent(), self.element.parent_element()); - let result = context.thread_local - .style_sharing_candidate_cache + let result = cache .share_style_if_possible(shared_context, selector_flags_map, bloom_filter, @@ -398,6 +403,10 @@ pub enum StyleSharingResult { /// storing nodes here temporarily is safe. pub struct StyleSharingCandidateCache { cache: LRUCache<[StyleSharingCandidate; STYLE_SHARING_CANDIDATE_CACHE_SIZE + 1]>, + /// The DOM depth we're currently at. This is used as an optimization to + /// clear the cache when we change depths, since we know at that point + /// nothing in the cache will match. + dom_depth: usize, } impl StyleSharingCandidateCache { @@ -405,6 +414,7 @@ impl StyleSharingCandidateCache { pub fn new() -> Self { StyleSharingCandidateCache { cache: LRUCache::new(), + dom_depth: 0, } } @@ -424,7 +434,8 @@ impl StyleSharingCandidateCache { element: &E, style: &ComputedValues, relations: StyleRelations, - mut validation_data: ValidationData) { + mut validation_data: ValidationData, + dom_depth: usize) { use selectors::matching::AFFECTED_BY_PRESENTATIONAL_HINTS; let parent = match element.parent_element() { @@ -467,6 +478,12 @@ impl StyleSharingCandidateCache { debug!("Inserting into cache: {:?} with parent {:?}", element, parent); + if self.dom_depth != dom_depth { + debug!("Clearing cache because depth changed from {:?} to {:?}, element: {:?}", + self.dom_depth, dom_depth, element); + self.clear(); + self.dom_depth = dom_depth; + } self.cache.insert(StyleSharingCandidate { element: unsafe { SendElement::new(*element) }, validation_data: validation_data, @@ -509,7 +526,6 @@ impl StyleSharingCandidateCache { return StyleSharingResult::CannotShare; } - let mut should_clear_cache = false; for (i, candidate) in self.iter_mut().enumerate() { let sharing_result = Self::test_candidate( @@ -553,11 +569,6 @@ impl StyleSharingCandidateCache { // Cache miss, let's see what kind of failure to decide // whether we keep trying or not. match miss { - // Cache miss because of parent, clear the candidate cache. - CacheMiss::Parent => { - should_clear_cache = true; - break; - }, // Too expensive failure, give up, we don't want another // one of these. CacheMiss::PresHints | @@ -571,10 +582,6 @@ impl StyleSharingCandidateCache { debug!("{:?} Cannot share style: {} cache entries", target.element, self.cache.num_entries()); - if should_clear_cache { - self.clear(); - } - StyleSharingResult::CannotShare }