From 40d2cbe8edb4894c29efb83d864cedc4ca401450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 2 Sep 2018 23:03:47 +0000 Subject: [PATCH] style: Shrink CascadeData by turning the bloom filters into hash sets. We're wasting 1kb there, which is kind of stupid. The only advantage of using a bloom filter is that memory usage doesn't increase even if there's a gazillion attribute selectors and such. But: * For IDs we already have a bunch of data structures for invalidation and such which key on the id, so the bloom filter would be a very minor thing. * For attribute selectors we don't have such a data structure, but if people used a gazillion attribute selectors we should! Differential Revision: https://phabricator.services.mozilla.com/D4668 --- components/style/stylist.rs | 63 +++++++++++-------------------------- 1 file changed, 18 insertions(+), 45 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 53e2a43b1d5..fbf2b18036e 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -24,11 +24,11 @@ use properties::{self, CascadeMode, ComputedValues}; use properties::{AnimationRules, PropertyDeclarationBlock}; use rule_cache::{RuleCache, RuleCacheConditions}; use rule_tree::{CascadeLevel, RuleTree, ShadowCascadeOrder, StrongRuleNode, StyleSource}; -use selector_map::{PrecomputedHashMap, SelectorMap, SelectorMapEntry}; +use selector_map::{PrecomputedHashMap, PrecomputedHashSet, SelectorMap, SelectorMapEntry}; use selector_parser::{PerPseudoElementMap, PseudoElement, SelectorImpl, SnapshotMap}; use selectors::NthIndexCache; use selectors::attr::{CaseSensitivity, NamespaceConstraint}; -use selectors::bloom::{BloomFilter, NonCountingBloomFilter}; +use selectors::bloom::BloomFilter; use selectors::matching::{matches_selector, ElementSelectorFlags, MatchingContext, MatchingMode}; use selectors::matching::VisitedHandlingMode; use selectors::parser::{AncestorHashes, Combinator, Component, Selector}; @@ -1396,8 +1396,7 @@ impl Stylist { CaseSensitivity::CaseSensitive => {}, } - let hash = id.get_hash(); - self.any_applicable_rule_data(element, |data| data.mapped_ids.might_contain_hash(hash)) + self.any_applicable_rule_data(element, |data| data.mapped_ids.contains(id)) } /// Returns the registered `@keyframes` animation for the specified name. @@ -1753,11 +1752,9 @@ struct StylistSelectorVisitor<'a> { passed_rightmost_selector: bool, /// The filter with all the id's getting referenced from rightmost /// selectors. - mapped_ids: &'a mut NonCountingBloomFilter, + mapped_ids: &'a mut PrecomputedHashSet, /// The filter with the local names of attributes there are selectors for. - attribute_dependencies: &'a mut NonCountingBloomFilter, - /// Whether there's any attribute selector for the [style] attribute. - style_attribute_dependency: &'a mut bool, + attribute_dependencies: &'a mut PrecomputedHashSet, /// All the states selectors in the page reference. state_dependencies: &'a mut ElementState, /// All the document states selectors in the page reference. @@ -1822,13 +1819,8 @@ impl<'a> SelectorVisitor for StylistSelectorVisitor<'a> { name: &LocalName, lower_name: &LocalName, ) -> bool { - if *lower_name == local_name!("style") { - *self.style_attribute_dependency = true; - } else { - self.attribute_dependencies.insert_hash(name.get_hash()); - self.attribute_dependencies - .insert_hash(lower_name.get_hash()); - } + self.attribute_dependencies.insert(name.clone()); + self.attribute_dependencies.insert(lower_name.clone()); true } @@ -1854,7 +1846,7 @@ impl<'a> SelectorVisitor for StylistSelectorVisitor<'a> { // // NOTE(emilio): See the comment regarding on when this may // break in visit_complex_selector. - self.mapped_ids.insert_hash(id.get_hash()); + self.mapped_ids.insert(id.clone()); }, _ => {}, } @@ -1954,18 +1946,9 @@ pub struct CascadeData { /// 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.) - #[ignore_malloc_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, + /// (We don't bother storing the namespace, since namespaced attributes are + /// rare.) + attribute_dependencies: PrecomputedHashSet, /// The element state bits that are relied on by selectors. Like /// `attribute_dependencies`, this is used to avoid taking element snapshots @@ -1981,8 +1964,7 @@ pub struct CascadeData { /// 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. - #[ignore_malloc_size_of = "just an array"] - mapped_ids: NonCountingBloomFilter, + mapped_ids: PrecomputedHashSet, /// Selectors that require explicit cache revalidation (i.e. which depend /// on state that is not otherwise visible to the cache, like attributes or @@ -2019,11 +2001,10 @@ impl CascadeData { host_rules: None, slotted_rules: None, invalidation_map: InvalidationMap::new(), - attribute_dependencies: NonCountingBloomFilter::new(), - style_attribute_dependency: false, + attribute_dependencies: PrecomputedHashSet::default(), state_dependencies: ElementState::empty(), document_state_dependencies: DocumentState::empty(), - mapped_ids: NonCountingBloomFilter::new(), + mapped_ids: PrecomputedHashSet::default(), selectors_for_cache_revalidation: SelectorMap::new(), animations: Default::default(), extra_data: ExtraStyleData::default(), @@ -2088,13 +2069,9 @@ impl CascadeData { /// selector of some rule. #[inline] pub fn might_have_attribute_dependency(&self, local_name: &LocalName) -> bool { - if *local_name == local_name!("style") { - return self.style_attribute_dependency; - } - - self.attribute_dependencies - .might_contain_hash(local_name.get_hash()) + self.attribute_dependencies.contains(local_name) } + #[inline] fn normal_rules(&self, pseudo: Option<&PseudoElement>) -> Option<&SelectorMap> { self.normal_rules.rules(pseudo) @@ -2221,7 +2198,6 @@ impl CascadeData { 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, document_state_dependencies: &mut self.document_state_dependencies, mapped_ids: &mut self.mapped_ids, @@ -2431,7 +2407,6 @@ impl CascadeData { self.clear_cascade_data(); self.invalidation_map.clear(); self.attribute_dependencies.clear(); - self.style_attribute_dependency = false; self.state_dependencies = ElementState::empty(); self.document_state_dependencies = DocumentState::empty(); self.mapped_ids.clear(); @@ -2527,16 +2502,14 @@ 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 = NonCountingBloomFilter::new(); - let mut mapped_ids = NonCountingBloomFilter::new(); - let mut style_attribute_dependency = false; + let mut attribute_dependencies = Default::default(); + let mut mapped_ids = Default::default(); let mut state_dependencies = ElementState::empty(); let mut document_state_dependencies = DocumentState::empty(); let mut visitor = StylistSelectorVisitor { needs_revalidation: false, passed_rightmost_selector: false, attribute_dependencies: &mut attribute_dependencies, - style_attribute_dependency: &mut style_attribute_dependency, state_dependencies: &mut state_dependencies, document_state_dependencies: &mut document_state_dependencies, mapped_ids: &mut mapped_ids,