From 6bdb0abebf5d5cb3017d8213a9576d2fc3158d37 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 15:15:39 +0800 Subject: [PATCH 01/12] style: Make CascadeDataIter iterate from highest to lowest level. --- components/style/stylist.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index b13fd7376cde..ccf7a196e1ad 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1635,6 +1635,11 @@ impl CascadeData { } } +/// Iterator over `PerOriginCascadeData`, from highest level (user) to lowest +/// (user agent). +/// +/// We rely on this specific order for correctly looking up animations +/// (prioritizing rules at higher cascade levels), among other things. struct CascadeDataIter<'a> { cascade_data: &'a CascadeData, cur: usize, @@ -1645,9 +1650,9 @@ impl<'a> Iterator for CascadeDataIter<'a> { fn next(&mut self) -> Option<&'a PerOriginCascadeData> { let result = match self.cur { - 0 => &self.cascade_data.user_agent, + 0 => &self.cascade_data.user, 1 => &self.cascade_data.author, - 2 => &self.cascade_data.user, + 2 => &self.cascade_data.user_agent, _ => return None, }; self.cur += 1; From 68268226eac23f064054be01d6839fbba45325be Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Mon, 7 Aug 2017 16:04:30 +0800 Subject: [PATCH 02/12] style: Tweak Stylist API for getting animations to avoid exposing the hash table. --- components/style/animation.rs | 4 ++-- components/style/stylist.rs | 6 +++--- ports/geckolib/glue.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/components/style/animation.rs b/components/style/animation.rs index 69b94edb5808..c1a49fca7a12 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -533,7 +533,7 @@ pub fn maybe_start_animations(context: &SharedStyleContext, continue } - if let Some(ref anim) = context.stylist.animations().get(name) { + if let Some(ref anim) = context.stylist.get_animation(name) { debug!("maybe_start_animations: animation {} found", name); // If this animation doesn't have any keyframe, we can just continue @@ -637,7 +637,7 @@ pub fn update_style_for_animation(context: &SharedStyleContext, KeyframesRunningState::Paused(progress) => started_at + duration * progress, }; - let animation = match context.stylist.animations().get(name) { + let animation = match context.stylist.get_animation(name) { None => { warn!("update_style_for_animation: Animation {:?} not found", name); return; diff --git a/components/style/stylist.rs b/components/style/stylist.rs index ccf7a196e1ad..4b26371add18 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1327,10 +1327,10 @@ impl Stylist { self.is_device_dirty } - /// Returns the map of registered `@keyframes` animations. + /// Returns the registered `@keyframes` animation for the specified name. #[inline] - pub fn animations(&self) -> &PrecomputedHashMap { - &self.animations + pub fn get_animation(&self, name: &Atom) -> Option<&KeyframesAnimation> { + self.animations.get(name) } /// Computes the match results of a given element against the set of diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index a8fadfc202c8..131644f22c5c 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -3286,7 +3286,7 @@ pub extern "C" fn Servo_StyleSet_GetKeyframesForName(raw_data: RawServoStyleSetB let data = PerDocumentStyleData::from_ffi(raw_data).borrow(); let name = unsafe { Atom::from(name.as_ref().unwrap().as_str_unchecked()) }; - let animation = match data.stylist.animations().get(&name) { + let animation = match data.stylist.get_animation(&name) { Some(animation) => animation, None => return false, }; From 7f47ae073072eade7fb44751ecefbf53b87b2785 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Tue, 8 Aug 2017 17:30:16 +0800 Subject: [PATCH 03/12] style: Slight refactoring of TraversalStatistics::finish. --- components/style/context.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index 675fff06cc75..ee1057fc7356 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -387,16 +387,16 @@ impl TraversalStatistics { D: DomTraversal, { let threshold = traversal.shared_context().options.style_statistics_threshold; + let stylist = traversal.shared_context().stylist; self.is_parallel = Some(traversal.is_parallel()); self.is_large = Some(self.elements_traversed as usize >= threshold); self.traversal_time_ms = (time::precise_time_s() - start) * 1000.0; - self.selectors = traversal.shared_context().stylist.num_selectors() as u32; - self.revalidation_selectors = traversal.shared_context().stylist.num_revalidation_selectors() as u32; - self.dependency_selectors = - traversal.shared_context().stylist.invalidation_map().len() as u32; - self.declarations = traversal.shared_context().stylist.num_declarations() as u32; - self.stylist_rebuilds = traversal.shared_context().stylist.num_rebuilds() as u32; + self.selectors = stylist.num_selectors() as u32; + self.revalidation_selectors = stylist.num_revalidation_selectors() as u32; + self.dependency_selectors = stylist.invalidation_map().len() as u32; + self.declarations = stylist.num_declarations() as u32; + self.stylist_rebuilds = stylist.num_rebuilds() as u32; } /// Returns whether this traversal is 'large' in order to avoid console spam From e294372a328ca9d3b308b2712b04232814eb7dfa Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 12:56:57 +0800 Subject: [PATCH 04/12] selectors: Genericize BloomFilter so we can easily define a non-counting version. --- components/selectors/bloom.rs | 171 ++++++++++++++++++++-------------- 1 file changed, 102 insertions(+), 69 deletions(-) diff --git a/components/selectors/bloom.rs b/components/selectors/bloom.rs index 0e8290fc8c92..3d90dc0b9437 100644 --- a/components/selectors/bloom.rs +++ b/components/selectors/bloom.rs @@ -2,7 +2,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -//! Simple counting bloom filters. +//! Counting Bloom filter tuned for use as an ancestor filter for selector +//! matching. use fnv::FnvHasher; use std::hash::{Hash, Hasher}; @@ -15,9 +16,12 @@ const KEY_SIZE: usize = 12; const ARRAY_SIZE: usize = 1 << KEY_SIZE; const KEY_MASK: u32 = (1 << KEY_SIZE) - 1; -/// A counting Bloom filter with 8-bit counters. For now we assume -/// that having two hash functions is enough, but we may revisit that -/// decision later. +/// A counting Bloom filter with 8-bit counters. +pub type BloomFilter = CountingBloomFilter; + +/// A counting Bloom filter with parameterized storage to handle +/// counters of different sizes. For now we assume that having two hash +/// functions is enough, but we may revisit that decision later. /// /// The filter uses an array with 2**KeySize entries. /// @@ -61,58 +65,30 @@ const KEY_MASK: u32 = (1 << KEY_SIZE) - 1; /// Similarly, using a KeySize of 10 would lead to a 4% false /// positive rate for N == 100 and to quite bad false positive /// rates for larger N. -pub struct BloomFilter { - counters: [u8; ARRAY_SIZE], -} - -impl Clone for BloomFilter { - #[inline] - fn clone(&self) -> BloomFilter { - BloomFilter { - counters: self.counters, - } - } +#[derive(Clone)] +pub struct CountingBloomFilter where S: BloomStorage { + storage: S, } -impl BloomFilter { +impl CountingBloomFilter where S: BloomStorage { /// Creates a new bloom filter. #[inline] - pub fn new() -> BloomFilter { - BloomFilter { - counters: [0; ARRAY_SIZE], + pub fn new() -> Self { + CountingBloomFilter { + storage: Default::default(), } } - #[inline] - fn first_slot(&self, hash: u32) -> &u8 { - &self.counters[hash1(hash) as usize] - } - - #[inline] - fn first_mut_slot(&mut self, hash: u32) -> &mut u8 { - &mut self.counters[hash1(hash) as usize] - } - - #[inline] - fn second_slot(&self, hash: u32) -> &u8 { - &self.counters[hash2(hash) as usize] - } - - #[inline] - fn second_mut_slot(&mut self, hash: u32) -> &mut u8 { - &mut self.counters[hash2(hash) as usize] - } - #[inline] pub fn clear(&mut self) { - self.counters = [0; ARRAY_SIZE] + self.storage = Default::default(); } // Slow linear accessor to make sure the bloom filter is zeroed. This should // never be used in release builds. #[cfg(debug_assertions)] pub fn is_zeroed(&self) -> bool { - self.counters.iter().all(|x| *x == 0) + self.storage.is_zeroed() } #[cfg(not(debug_assertions))] @@ -122,18 +98,8 @@ impl BloomFilter { #[inline] pub fn insert_hash(&mut self, hash: u32) { - { - let slot1 = self.first_mut_slot(hash); - if !full(slot1) { - *slot1 += 1 - } - } - { - let slot2 = self.second_mut_slot(hash); - if !full(slot2) { - *slot2 += 1 - } - } + self.storage.adjust_first_slot(hash, true); + self.storage.adjust_second_slot(hash, true); } /// Inserts an item into the bloom filter. @@ -144,18 +110,8 @@ impl BloomFilter { #[inline] pub fn remove_hash(&mut self, hash: u32) { - { - let slot1 = self.first_mut_slot(hash); - if !full(slot1) { - *slot1 -= 1 - } - } - { - let slot2 = self.second_mut_slot(hash); - if !full(slot2) { - *slot2 -= 1 - } - } + self.storage.adjust_first_slot(hash, false); + self.storage.adjust_second_slot(hash, false); } /// Removes an item from the bloom filter. @@ -166,7 +122,8 @@ impl BloomFilter { #[inline] pub fn might_contain_hash(&self, hash: u32) -> bool { - *self.first_slot(hash) != 0 && *self.second_slot(hash) != 0 + !self.storage.first_slot_is_empty(hash) && + !self.storage.second_slot_is_empty(hash) } /// Check whether the filter might contain an item. This can @@ -179,9 +136,85 @@ impl BloomFilter { } } -#[inline] -fn full(slot: &u8) -> bool { - *slot == 0xff +pub trait BloomStorage : Clone + Default { + fn slot_is_empty(&self, index: usize) -> bool; + fn adjust_slot(&mut self, index: usize, increment: bool); + fn is_zeroed(&self) -> bool; + + #[inline] + fn first_slot_is_empty(&self, hash: u32) -> bool { + self.slot_is_empty(Self::first_slot_index(hash)) + } + + #[inline] + fn second_slot_is_empty(&self, hash: u32) -> bool { + self.slot_is_empty(Self::second_slot_index(hash)) + } + + #[inline] + fn adjust_first_slot(&mut self, hash: u32, increment: bool) { + self.adjust_slot(Self::first_slot_index(hash), increment) + } + + #[inline] + fn adjust_second_slot(&mut self, hash: u32, increment: bool) { + self.adjust_slot(Self::second_slot_index(hash), increment) + } + + #[inline] + fn first_slot_index(hash: u32) -> usize { + hash1(hash) as usize + } + + #[inline] + fn second_slot_index(hash: u32) -> usize { + hash2(hash) as usize + } +} + +/// Storage class for a CountingBloomFilter that has 8-bit counters. +pub struct BloomStorageU8 { + counters: [u8; ARRAY_SIZE], +} + +impl BloomStorage for BloomStorageU8 { + #[inline] + fn adjust_slot(&mut self, index: usize, increment: bool) { + let slot = &mut self.counters[index]; + if *slot != 0xff { // full + if increment { + *slot += 1; + } else { + *slot -= 1; + } + } + } + + #[inline] + fn slot_is_empty(&self, index: usize) -> bool { + self.counters[index] == 0 + } + + #[inline] + fn is_zeroed(&self) -> bool { + self.counters.iter().all(|x| *x == 0) + } +} + +impl Default for BloomStorageU8 { + fn default() -> Self { + BloomStorageU8 { + counters: [0; ARRAY_SIZE], + } + } +} + +impl Clone for BloomStorageU8 { + fn clone(&self) -> Self { + BloomStorageU8 { + counters: self.counters, + } + } } fn hash(elem: &T) -> u32 { From df2c8b2902523a02f5a51cbd47b797826a88d23e Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 13:17:23 +0800 Subject: [PATCH 05/12] selectors: Add a non-counting Bloom filter type. --- components/selectors/bloom.rs | 67 +++++++++++++++++++++++++++++++++-- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/components/selectors/bloom.rs b/components/selectors/bloom.rs index 3d90dc0b9437..9475676ec714 100644 --- a/components/selectors/bloom.rs +++ b/components/selectors/bloom.rs @@ -2,8 +2,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -//! Counting Bloom filter tuned for use as an ancestor filter for selector -//! matching. +//! Counting and non-counting Bloom filters tuned for use as ancestor filters +//! for selector matching. use fnv::FnvHasher; use std::hash::{Hash, Hasher}; @@ -19,6 +19,11 @@ const KEY_MASK: u32 = (1 << KEY_SIZE) - 1; /// A counting Bloom filter with 8-bit counters. pub type BloomFilter = CountingBloomFilter; +/// A non-counting Bloom filter. +/// +/// Effectively a counting Bloom filter with 1-bit counters. +pub type NonCountingBloomFilter = CountingBloomFilter; + /// A counting Bloom filter with parameterized storage to handle /// counters of different sizes. For now we assume that having two hash /// functions is enough, but we may revisit that decision later. @@ -217,6 +222,56 @@ impl Clone for BloomStorageU8 { } } +/// Storage class for a CountingBloomFilter that has 1-bit counters. +pub struct BloomStorageBool { + counters: [u8; ARRAY_SIZE / 8], +} + +impl BloomStorage for BloomStorageBool { + #[inline] + fn adjust_slot(&mut self, index: usize, increment: bool) { + let bit = 1 << (index % 8); + let byte = &mut self.counters[index / 8]; + + // Since we have only one bit for storage, decrementing it + // should never do anything. Assert against an accidental + // decrementing of a bit that was never set. + assert!(increment || (*byte & bit) != 0, + "should not decrement if slot is already false"); + + if increment { + *byte |= bit; + } + } + + #[inline] + fn slot_is_empty(&self, index: usize) -> bool { + let bit = 1 << (index % 8); + (self.counters[index / 8] & bit) == 0 + } + + #[inline] + fn is_zeroed(&self) -> bool { + self.counters.iter().all(|x| *x == 0) + } +} + +impl Default for BloomStorageBool { + fn default() -> Self { + BloomStorageBool { + counters: [0; ARRAY_SIZE / 8], + } + } +} + +impl Clone for BloomStorageBool { + fn clone(&self) -> Self { + BloomStorageBool { + counters: self.counters, + } + } +} + fn hash(elem: &T) -> u32 { let mut hasher = FnvHasher::default(); elem.hash(&mut hasher); @@ -236,8 +291,16 @@ fn hash2(hash: u32) -> u32 { #[test] fn create_and_insert_some_stuff() { + use std::mem::transmute; + let mut bf = BloomFilter::new(); + // Statically assert that ARRAY_SIZE is a multiple of 8, which + // BloomStorageBool relies on. + unsafe { + transmute::<[u8; ARRAY_SIZE % 8], [u8; 0]>([]); + } + for i in 0_usize .. 1000 { bf.insert(&i); } From b23f947d77e4299f547994ead9b5c703c37fa070 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 13:59:37 +0800 Subject: [PATCH 06/12] selectors: Add a simple Debug impl for CountingBloomFilter. --- components/selectors/bloom.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/components/selectors/bloom.rs b/components/selectors/bloom.rs index 9475676ec714..2b96e4f51f87 100644 --- a/components/selectors/bloom.rs +++ b/components/selectors/bloom.rs @@ -6,6 +6,7 @@ //! for selector matching. use fnv::FnvHasher; +use std::fmt::{self, Debug}; use std::hash::{Hash, Hasher}; // The top 8 bits of the 32-bit hash value are not used by the bloom filter. @@ -141,6 +142,18 @@ impl CountingBloomFilter where S: BloomStorage { } } +impl Debug for CountingBloomFilter where S: BloomStorage { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let mut slots_used = 0; + for i in 0..ARRAY_SIZE { + if !self.storage.slot_is_empty(i) { + slots_used += 1; + } + } + write!(f, "BloomFilter({}/{})", slots_used, ARRAY_SIZE) + } +} + pub trait BloomStorage : Clone + Default { fn slot_is_empty(&self, index: usize) -> bool; fn adjust_slot(&mut self, index: usize, increment: bool); From 781e755f9a6ca270d46e435013ae4381254b583d Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 13:34:01 +0800 Subject: [PATCH 07/12] style: Use non-counting Bloom filters in Stylist where appropriate. --- components/style/stylist.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 4b26371add18..8c148e354100 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -25,7 +25,7 @@ use rule_tree::{CascadeLevel, RuleTree, StyleSource}; use selector_map::{PrecomputedHashMap, SelectorMap, SelectorMapEntry}; use selector_parser::{SelectorImpl, PerPseudoElementMap, PseudoElement}; use selectors::attr::NamespaceConstraint; -use selectors::bloom::BloomFilter; +use selectors::bloom::{BloomFilter, NonCountingBloomFilter}; use selectors::matching::{ElementSelectorFlags, matches_selector, MatchingContext, MatchingMode}; use selectors::matching::VisitedHandlingMode; use selectors::parser::{AncestorHashes, Combinator, Component, Selector}; @@ -118,10 +118,8 @@ pub struct Stylist { /// to avoid taking element snapshots when an irrelevant attribute changes. /// (We don't bother storing the namespace, since namespaced attributes /// are rare.) - /// - /// FIXME(heycam): This doesn't really need to be a counting Bloom filter. #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] - attribute_dependencies: BloomFilter, + attribute_dependencies: NonCountingBloomFilter, /// Whether `"style"` appears in an attribute selector. This is not common, /// and by tracking this explicitly, we can avoid taking an element snapshot @@ -140,10 +138,8 @@ pub struct Stylist { /// hence in our selector maps). Used to determine when sharing styles is /// safe: we disallow style sharing for elements whose id matches this /// filter, and hence might be in one of our selector maps. - /// - /// FIXME(bz): This doesn't really need to be a counting Blooom filter. #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] - mapped_ids: BloomFilter, + mapped_ids: NonCountingBloomFilter, /// Selectors that require explicit cache revalidation (i.e. which depend /// on state that is not otherwise visible to the cache, like attributes or @@ -244,10 +240,10 @@ impl Stylist { rules_source_order: 0, rule_tree: RuleTree::new(), invalidation_map: InvalidationMap::new(), - attribute_dependencies: BloomFilter::new(), + attribute_dependencies: NonCountingBloomFilter::new(), style_attribute_dependency: false, state_dependencies: ElementState::empty(), - mapped_ids: BloomFilter::new(), + mapped_ids: NonCountingBloomFilter::new(), selectors_for_cache_revalidation: SelectorMap::new(), num_selectors: 0, num_declarations: 0, @@ -1470,9 +1466,9 @@ struct StylistSelectorVisitor<'a> { passed_rightmost_selector: bool, /// The filter with all the id's getting referenced from rightmost /// selectors. - mapped_ids: &'a mut BloomFilter, + mapped_ids: &'a mut NonCountingBloomFilter, /// The filter with the local names of attributes there are selectors for. - attribute_dependencies: &'a mut BloomFilter, + attribute_dependencies: &'a mut NonCountingBloomFilter, /// Whether there's any attribute selector for the [style] attribute. style_attribute_dependency: &'a mut bool, /// All the states selectors in the page reference. @@ -1766,8 +1762,8 @@ impl Rule { /// A function to be able to test the revalidation stuff. pub fn needs_revalidation_for_testing(s: &Selector) -> bool { - let mut attribute_dependencies = BloomFilter::new(); - let mut mapped_ids = BloomFilter::new(); + let mut attribute_dependencies = NonCountingBloomFilter::new(); + let mut mapped_ids = NonCountingBloomFilter::new(); let mut style_attribute_dependency = false; let mut state_dependencies = ElementState::empty(); let mut visitor = StylistSelectorVisitor { From 77c4a42e5d347b574b54309530af0b1c5b0947a2 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Mon, 7 Aug 2017 16:19:04 +0800 Subject: [PATCH 08/12] style: Move animations table into PerOriginCascadeData. --- components/style/stylist.rs | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 8c148e354100..9b3aaedc8f44 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -97,9 +97,6 @@ pub struct Stylist { /// The rule tree, that stores the results of selector matching. rule_tree: RuleTree, - /// A map with all the animations indexed by name. - animations: PrecomputedHashMap, - /// Applicable declarations for a given non-eagerly cascaded pseudo-element. /// These are eagerly computed once, and then used to resolve the new /// computed values on the fly on layout. @@ -235,7 +232,6 @@ impl Stylist { effective_media_query_results: EffectiveMediaQueryResults::new(), cascade_data: CascadeData::new(), - animations: Default::default(), precomputed_pseudo_element_decls: PerPseudoElementMap::default(), rules_source_order: 0, rule_tree: RuleTree::new(), @@ -303,7 +299,6 @@ impl Stylist { self.is_device_dirty = true; // preserve current quirks_mode value self.cascade_data.clear(); - self.animations.clear(); // Or set to Default::default()? self.precomputed_pseudo_element_decls.clear(); self.rules_source_order = 0; // We want to keep rule_tree around across stylist rebuilds. @@ -538,14 +533,15 @@ impl Stylist { debug!("Found valid keyframes rule: {:?}", *keyframes_rule); // Don't let a prefixed keyframes animation override a non-prefixed one. - let needs_insertion = keyframes_rule.vendor_prefix.is_none() || - self.animations.get(keyframes_rule.name.as_atom()).map_or(true, |rule| - rule.vendor_prefix.is_some()); + let needs_insertion = + keyframes_rule.vendor_prefix.is_none() || + origin_cascade_data.animations.get(keyframes_rule.name.as_atom()) + .map_or(true, |rule| rule.vendor_prefix.is_some()); if needs_insertion { let animation = KeyframesAnimation::from_keyframes( &keyframes_rule.keyframes, keyframes_rule.vendor_prefix.clone(), guard); debug!("Found valid keyframe animation: {:?}", animation); - self.animations.insert(keyframes_rule.name.as_atom().clone(), animation); + origin_cascade_data.animations.insert(keyframes_rule.name.as_atom().clone(), animation); } } #[cfg(feature = "gecko")] @@ -1326,7 +1322,10 @@ impl Stylist { /// Returns the registered `@keyframes` animation for the specified name. #[inline] pub fn get_animation(&self, name: &Atom) -> Option<&KeyframesAnimation> { - self.animations.get(name) + self.cascade_data + .iter_origins() + .filter_map(|d| d.animations.get(name)) + .next() } /// Computes the match results of a given element against the set of @@ -1667,6 +1666,10 @@ struct PerOriginCascadeData { /// Rules from stylesheets at this `CascadeData`'s origin that correspond /// to a given pseudo-element. pseudos_map: PerPseudoElementMap>, + + /// A map with all the animations at this `CascadeData`'s origin, indexed + /// by name. + animations: PrecomputedHashMap, } impl PerOriginCascadeData { @@ -1674,6 +1677,7 @@ impl PerOriginCascadeData { Self { element_map: SelectorMap::new(), pseudos_map: PerPseudoElementMap::default(), + animations: Default::default(), } } @@ -1686,7 +1690,9 @@ impl PerOriginCascadeData { } fn clear(&mut self) { - *self = Self::new(); + self.element_map = SelectorMap::new(); + self.pseudos_map = Default::default(); + self.animations = Default::default(); } fn has_rules_for_pseudo(&self, pseudo: &PseudoElement) -> bool { From 16937ba7cd71fa3b7b184ff56a7598ca3e12a97d Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 10:43:07 +0800 Subject: [PATCH 09/12] style: Move invalidation map into PerOriginCascadeData. --- components/style/context.rs | 2 +- .../invalidation/element/invalidation_map.rs | 3 +- .../style/invalidation/element/invalidator.rs | 12 +++---- components/style/stylist.rs | 34 ++++++++++++++----- 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index ee1057fc7356..4ee4f3b39593 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -394,7 +394,7 @@ impl TraversalStatistics { self.traversal_time_ms = (time::precise_time_s() - start) * 1000.0; self.selectors = stylist.num_selectors() as u32; self.revalidation_selectors = stylist.num_revalidation_selectors() as u32; - self.dependency_selectors = stylist.invalidation_map().len() as u32; + self.dependency_selectors = stylist.num_invalidations() as u32; self.declarations = stylist.num_declarations() as u32; self.stylist_rebuilds = stylist.num_rebuilds() as u32; } diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index e5c6c63bfcee..81f966a5df32 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -109,7 +109,7 @@ impl SelectorMapEntry for Dependency { /// The same, but for state selectors, which can track more exactly what state /// do they track. -#[derive(Clone)] +#[derive(Clone, Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct StateDependency { /// The other dependency fields. @@ -132,6 +132,7 @@ impl SelectorMapEntry for StateDependency { /// In particular, we want to lookup as few things as possible to get the fewer /// selectors the better, so this looks up by id, class, or looks at the list of /// state/other attribute affecting selectors. +#[derive(Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct InvalidationMap { /// A map from a given class name to all the selectors with that class diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index 055e3e6f3ac8..46b1d53bc1d1 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -213,9 +213,9 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> invalidates_self: false, }; - collector.collect_dependencies_in_invalidation_map( - shared_context.stylist.invalidation_map(), - ); + shared_context.stylist.each_invalidation_map(|invalidation_map| { + collector.collect_dependencies_in_invalidation_map(invalidation_map); + }); // TODO(emilio): Consider storing dependencies from the UA sheet in // a different map. If we do that, we can skip the stuff on the @@ -223,9 +223,9 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E> // just at that map. let _cut_off_inheritance = self.element.each_xbl_stylist(|stylist| { - collector.collect_dependencies_in_invalidation_map( - stylist.invalidation_map(), - ); + stylist.each_invalidation_map(|invalidation_map| { + collector.collect_dependencies_in_invalidation_map(invalidation_map); + }); }); collector.invalidates_self diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 9b3aaedc8f44..cf4df1f5a559 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -108,9 +108,6 @@ pub struct Stylist { /// style rule appears in a stylesheet, needed to sort them by source order. rules_source_order: u32, - /// The invalidation map for this document. - invalidation_map: InvalidationMap, - /// The attribute local names that appear in attribute selectors. Used /// to avoid taking element snapshots when an irrelevant attribute changes. /// (We don't bother storing the namespace, since namespaced attributes @@ -235,7 +232,6 @@ impl Stylist { precomputed_pseudo_element_decls: PerPseudoElementMap::default(), rules_source_order: 0, rule_tree: RuleTree::new(), - invalidation_map: InvalidationMap::new(), attribute_dependencies: NonCountingBloomFilter::new(), style_attribute_dependency: false, state_dependencies: ElementState::empty(), @@ -269,9 +265,23 @@ impl Stylist { self.selectors_for_cache_revalidation.len() } - /// Gets a reference to the invalidation map. - pub fn invalidation_map(&self) -> &InvalidationMap { - &self.invalidation_map + /// Returns the number of entries in invalidation maps. + pub fn num_invalidations(&self) -> usize { + self.cascade_data.iter_origins() + .map(|d| d.invalidation_map.len()).sum() + } + + /// Invokes `f` with the `InvalidationMap` for each origin. + /// + /// NOTE(heycam) This might be better as an `iter_invalidation_maps`, once + /// we have `impl trait` and can return that easily without bothering to + /// create a whole new iterator type. + pub fn each_invalidation_map(&self, mut f: F) + where F: FnMut(&InvalidationMap) + { + for origin_cascade_data in self.cascade_data.iter_origins() { + f(&origin_cascade_data.invalidation_map) + } } /// Clear the stylist's state, effectively resetting it to more or less @@ -302,7 +312,6 @@ impl Stylist { self.precomputed_pseudo_element_decls.clear(); self.rules_source_order = 0; // We want to keep rule_tree around across stylist rebuilds. - self.invalidation_map.clear(); self.attribute_dependencies.clear(); self.style_attribute_dependency = false; self.state_dependencies = ElementState::empty(); @@ -497,7 +506,9 @@ impl Stylist { map.insert(rule, self.quirks_mode); - self.invalidation_map.note_selector(selector, self.quirks_mode); + origin_cascade_data + .invalidation_map + .note_selector(selector, self.quirks_mode); let mut visitor = StylistSelectorVisitor { needs_revalidation: false, passed_rightmost_selector: false, @@ -1670,6 +1681,9 @@ struct PerOriginCascadeData { /// A map with all the animations at this `CascadeData`'s origin, indexed /// by name. animations: PrecomputedHashMap, + + /// The invalidation map for the rules at this origin. + invalidation_map: InvalidationMap, } impl PerOriginCascadeData { @@ -1678,6 +1692,7 @@ impl PerOriginCascadeData { element_map: SelectorMap::new(), pseudos_map: PerPseudoElementMap::default(), animations: Default::default(), + invalidation_map: InvalidationMap::new(), } } @@ -1693,6 +1708,7 @@ impl PerOriginCascadeData { self.element_map = SelectorMap::new(); self.pseudos_map = Default::default(); self.animations = Default::default(); + self.invalidation_map.clear(); } fn has_rules_for_pseudo(&self, pseudo: &PseudoElement) -> bool { From 30de56f2086246d2f978f7bcbf6dad98afdf3f42 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 14:00:09 +0800 Subject: [PATCH 10/12] style: Move attribute and state dependencies into PerOriginCascadeData. --- components/style/stylist.rs | 99 ++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 44 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index cf4df1f5a559..d8d5f4cd8af1 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -108,33 +108,6 @@ pub struct Stylist { /// style rule appears in a stylesheet, needed to sort them by source order. rules_source_order: u32, - /// The attribute local names that appear in attribute selectors. Used - /// to avoid taking element snapshots when an irrelevant attribute changes. - /// (We don't bother storing the namespace, since namespaced attributes - /// are rare.) - #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] - attribute_dependencies: NonCountingBloomFilter, - - /// Whether `"style"` appears in an attribute selector. This is not common, - /// and by tracking this explicitly, we can avoid taking an element snapshot - /// in the common case of style=""` changing due to modifying - /// `element.style`. (We could track this in `attribute_dependencies`, like - /// all other attributes, but we should probably not risk incorrectly - /// returning `true` for `"style"` just due to a hash collision.) - style_attribute_dependency: bool, - - /// The element state bits that are relied on by selectors. Like - /// `attribute_dependencies`, this is used to avoid taking element snapshots - /// when an irrelevant element state bit changes. - state_dependencies: ElementState, - - /// The ids that appear in the rightmost complex selector of selectors (and - /// hence in our selector maps). Used to determine when sharing styles is - /// safe: we disallow style sharing for elements whose id matches this - /// filter, and hence might be in one of our selector maps. - #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] - mapped_ids: NonCountingBloomFilter, - /// Selectors that require explicit cache revalidation (i.e. which depend /// on state that is not otherwise visible to the cache, like attributes or /// tree-structural state like child index and pseudos). @@ -232,10 +205,6 @@ impl Stylist { precomputed_pseudo_element_decls: PerPseudoElementMap::default(), rules_source_order: 0, rule_tree: RuleTree::new(), - attribute_dependencies: NonCountingBloomFilter::new(), - style_attribute_dependency: false, - state_dependencies: ElementState::empty(), - mapped_ids: NonCountingBloomFilter::new(), selectors_for_cache_revalidation: SelectorMap::new(), num_selectors: 0, num_declarations: 0, @@ -312,10 +281,6 @@ impl Stylist { self.precomputed_pseudo_element_decls.clear(); self.rules_source_order = 0; // We want to keep rule_tree around across stylist rebuilds. - self.attribute_dependencies.clear(); - self.style_attribute_dependency = false; - self.state_dependencies = ElementState::empty(); - self.mapped_ids.clear(); self.selectors_for_cache_revalidation = SelectorMap::new(); self.num_selectors = 0; self.num_declarations = 0; @@ -512,10 +477,10 @@ impl Stylist { let mut visitor = StylistSelectorVisitor { needs_revalidation: false, passed_rightmost_selector: false, - attribute_dependencies: &mut self.attribute_dependencies, - style_attribute_dependency: &mut self.style_attribute_dependency, - state_dependencies: &mut self.state_dependencies, - mapped_ids: &mut self.mapped_ids, + attribute_dependencies: &mut origin_cascade_data.attribute_dependencies, + style_attribute_dependency: &mut origin_cascade_data.style_attribute_dependency, + state_dependencies: &mut origin_cascade_data.state_dependencies, + mapped_ids: &mut origin_cascade_data.mapped_ids, }; selector.visit(&mut visitor); @@ -579,9 +544,16 @@ impl Stylist { // we rebuild. true } else if *local_name == local_name!("style") { - self.style_attribute_dependency + self.cascade_data + .iter_origins() + .any(|d| d.style_attribute_dependency) } else { - self.attribute_dependencies.might_contain_hash(local_name.get_hash()) + self.cascade_data + .iter_origins() + .any(|d| { + d.attribute_dependencies + .might_contain_hash(local_name.get_hash()) + }) } } @@ -593,14 +565,16 @@ impl Stylist { // rules rely on until we rebuild. true } else { - self.state_dependencies.intersects(state) + self.has_state_dependency(state) } } /// Returns whether the given ElementState bit is relied upon by a selector /// of some rule in the stylist. pub fn has_state_dependency(&self, state: ElementState) -> bool { - self.state_dependencies.intersects(state) + self.cascade_data + .iter_origins() + .any(|d| d.state_dependencies.intersects(state)) } /// Computes the style for a given "precomputed" pseudo-element, taking the @@ -1320,7 +1294,9 @@ impl Stylist { /// of our rule maps. #[inline] pub fn may_have_rules_for_id(&self, id: &Atom) -> bool { - self.mapped_ids.might_contain_hash(id.get_hash()) + self.cascade_data + .iter_origins() + .any(|d| d.mapped_ids.might_contain_hash(id.get_hash())) } /// Return whether the device is dirty, that is, whether the screen size or @@ -1684,6 +1660,33 @@ struct PerOriginCascadeData { /// The invalidation map for the rules at this origin. invalidation_map: InvalidationMap, + + /// The attribute local names that appear in attribute selectors. Used + /// to avoid taking element snapshots when an irrelevant attribute changes. + /// (We don't bother storing the namespace, since namespaced attributes + /// are rare.) + #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] + attribute_dependencies: NonCountingBloomFilter, + + /// Whether `"style"` appears in an attribute selector. This is not common, + /// and by tracking this explicitly, we can avoid taking an element snapshot + /// in the common case of style=""` changing due to modifying + /// `element.style`. (We could track this in `attribute_dependencies`, like + /// all other attributes, but we should probably not risk incorrectly + /// returning `true` for `"style"` just due to a hash collision.) + style_attribute_dependency: bool, + + /// The element state bits that are relied on by selectors. Like + /// `attribute_dependencies`, this is used to avoid taking element snapshots + /// when an irrelevant element state bit changes. + state_dependencies: ElementState, + + /// The ids that appear in the rightmost complex selector of selectors (and + /// hence in our selector maps). Used to determine when sharing styles is + /// safe: we disallow style sharing for elements whose id matches this + /// filter, and hence might be in one of our selector maps. + #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] + mapped_ids: NonCountingBloomFilter, } impl PerOriginCascadeData { @@ -1693,6 +1696,10 @@ impl PerOriginCascadeData { pseudos_map: PerPseudoElementMap::default(), animations: Default::default(), invalidation_map: InvalidationMap::new(), + attribute_dependencies: NonCountingBloomFilter::new(), + style_attribute_dependency: false, + state_dependencies: ElementState::empty(), + mapped_ids: NonCountingBloomFilter::new(), } } @@ -1709,6 +1716,10 @@ impl PerOriginCascadeData { self.pseudos_map = Default::default(); self.animations = Default::default(); self.invalidation_map.clear(); + self.attribute_dependencies.clear(); + self.style_attribute_dependency = false; + self.state_dependencies = ElementState::empty(); + self.mapped_ids.clear(); } fn has_rules_for_pseudo(&self, pseudo: &PseudoElement) -> bool { From cd5b2c9fbea1922d0a7896d039ead39c227f3783 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 14:32:40 +0800 Subject: [PATCH 11/12] style: Move revalidation selectors into PerOriginCascadeData. --- components/style/stylist.rs | 45 ++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index d8d5f4cd8af1..6844dd91a31f 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -108,12 +108,6 @@ pub struct Stylist { /// style rule appears in a stylesheet, needed to sort them by source order. rules_source_order: u32, - /// Selectors that require explicit cache revalidation (i.e. which depend - /// on state that is not otherwise visible to the cache, like attributes or - /// tree-structural state like child index and pseudos). - #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] - selectors_for_cache_revalidation: SelectorMap, - /// The total number of selectors. num_selectors: usize, @@ -205,7 +199,6 @@ impl Stylist { precomputed_pseudo_element_decls: PerPseudoElementMap::default(), rules_source_order: 0, rule_tree: RuleTree::new(), - selectors_for_cache_revalidation: SelectorMap::new(), num_selectors: 0, num_declarations: 0, num_rebuilds: 0, @@ -231,7 +224,8 @@ impl Stylist { /// Returns the number of revalidation_selectors. pub fn num_revalidation_selectors(&self) -> usize { - self.selectors_for_cache_revalidation.len() + self.cascade_data.iter_origins() + .map(|d| d.selectors_for_cache_revalidation.len()).sum() } /// Returns the number of entries in invalidation maps. @@ -281,7 +275,6 @@ impl Stylist { self.precomputed_pseudo_element_decls.clear(); self.rules_source_order = 0; // We want to keep rule_tree around across stylist rebuilds. - self.selectors_for_cache_revalidation = SelectorMap::new(); self.num_selectors = 0; self.num_declarations = 0; // preserve num_rebuilds value, since it should stay across @@ -486,7 +479,7 @@ impl Stylist { selector.visit(&mut visitor); if visitor.needs_revalidation { - self.selectors_for_cache_revalidation.insert( + origin_cascade_data.selectors_for_cache_revalidation.insert( RevalidationSelectorAndHashes::new(selector.clone(), hashes), self.quirks_mode); } @@ -1336,17 +1329,19 @@ impl Stylist { // the lookups, which means that the bitvecs are comparable. We verify // this in the caller by asserting that the bitvecs are same-length. let mut results = BitVec::new(); - self.selectors_for_cache_revalidation.lookup( - *element, self.quirks_mode, &mut |selector_and_hashes| { - results.push(matches_selector(&selector_and_hashes.selector, - selector_and_hashes.selector_offset, - Some(&selector_and_hashes.hashes), - element, - &mut matching_context, - flags_setter)); - true - } - ); + for origin_cascade_data in self.cascade_data.iter_origins() { + origin_cascade_data.selectors_for_cache_revalidation.lookup( + *element, self.quirks_mode, &mut |selector_and_hashes| { + results.push(matches_selector(&selector_and_hashes.selector, + selector_and_hashes.selector_offset, + Some(&selector_and_hashes.hashes), + element, + &mut matching_context, + flags_setter)); + true + } + ); + } results } @@ -1687,6 +1682,12 @@ struct PerOriginCascadeData { /// filter, and hence might be in one of our selector maps. #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] mapped_ids: NonCountingBloomFilter, + + /// Selectors that require explicit cache revalidation (i.e. which depend + /// on state that is not otherwise visible to the cache, like attributes or + /// tree-structural state like child index and pseudos). + #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] + selectors_for_cache_revalidation: SelectorMap, } impl PerOriginCascadeData { @@ -1700,6 +1701,7 @@ impl PerOriginCascadeData { style_attribute_dependency: false, state_dependencies: ElementState::empty(), mapped_ids: NonCountingBloomFilter::new(), + selectors_for_cache_revalidation: SelectorMap::new(), } } @@ -1720,6 +1722,7 @@ impl PerOriginCascadeData { self.style_attribute_dependency = false; self.state_dependencies = ElementState::empty(); self.mapped_ids.clear(); + self.selectors_for_cache_revalidation = SelectorMap::new(); } fn has_rules_for_pseudo(&self, pseudo: &PseudoElement) -> bool { From 6cbe55206b6e328c994f80e5e4b51fe2d4305658 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Wed, 9 Aug 2017 14:39:02 +0800 Subject: [PATCH 12/12] style: Move selector/declaration counts into PerOriginCascadeData. --- components/style/stylist.rs | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 6844dd91a31f..3000cd29e278 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -108,12 +108,6 @@ pub struct Stylist { /// style rule appears in a stylesheet, needed to sort them by source order. rules_source_order: u32, - /// The total number of selectors. - num_selectors: usize, - - /// The total number of declarations. - num_declarations: usize, - /// The total number of times the stylist has been rebuilt. num_rebuilds: usize, } @@ -199,8 +193,6 @@ impl Stylist { precomputed_pseudo_element_decls: PerPseudoElementMap::default(), rules_source_order: 0, rule_tree: RuleTree::new(), - num_selectors: 0, - num_declarations: 0, num_rebuilds: 0, } @@ -209,12 +201,12 @@ impl Stylist { /// Returns the number of selectors. pub fn num_selectors(&self) -> usize { - self.num_selectors + self.cascade_data.iter_origins().map(|d| d.num_selectors).sum() } /// Returns the number of declarations. pub fn num_declarations(&self) -> usize { - self.num_declarations + self.cascade_data.iter_origins().map(|d| d.num_declarations).sum() } /// Returns the number of times the stylist has been rebuilt. @@ -275,8 +267,6 @@ impl Stylist { self.precomputed_pseudo_element_decls.clear(); self.rules_source_order = 0; // We want to keep rule_tree around across stylist rebuilds. - self.num_selectors = 0; - self.num_declarations = 0; // preserve num_rebuilds value, since it should stay across // clear()/rebuild() cycles. } @@ -417,9 +407,10 @@ impl Stylist { match *rule { CssRule::Style(ref locked) => { let style_rule = locked.read_with(&guard); - self.num_declarations += style_rule.block.read_with(&guard).len(); + origin_cascade_data.num_declarations += + style_rule.block.read_with(&guard).len(); for selector in &style_rule.selectors.0 { - self.num_selectors += 1; + origin_cascade_data.num_selectors += 1; let map = match selector.pseudo_element() { Some(pseudo) if pseudo.is_precomputed() => { @@ -1688,6 +1679,12 @@ struct PerOriginCascadeData { /// tree-structural state like child index and pseudos). #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] selectors_for_cache_revalidation: SelectorMap, + + /// The total number of selectors. + num_selectors: usize, + + /// The total number of declarations. + num_declarations: usize, } impl PerOriginCascadeData { @@ -1702,6 +1699,8 @@ impl PerOriginCascadeData { state_dependencies: ElementState::empty(), mapped_ids: NonCountingBloomFilter::new(), selectors_for_cache_revalidation: SelectorMap::new(), + num_selectors: 0, + num_declarations: 0, } } @@ -1723,6 +1722,8 @@ impl PerOriginCascadeData { self.state_dependencies = ElementState::empty(); self.mapped_ids.clear(); self.selectors_for_cache_revalidation = SelectorMap::new(); + self.num_selectors = 0; + self.num_declarations = 0; } fn has_rules_for_pseudo(&self, pseudo: &PseudoElement) -> bool {