From f5c5be00a7df984647364dfc84c0c9bc6d3e2f34 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 28 Sep 2017 19:10:46 -0500 Subject: [PATCH] Diagnostic map semantics. MozReview-Commit-ID: C0a5g6xMPY0 --- components/hashglobe/src/diagnostic.rs | 143 ++++++++++++++++++ components/hashglobe/src/fake.rs | 14 ++ components/hashglobe/src/lib.rs | 1 + components/malloc_size_of/lib.rs | 19 +++ components/style/context.rs | 6 - components/style/custom_properties.rs | 17 ++- components/style/hash.rs | 7 +- .../invalidation/element/invalidation_map.rs | 34 +++-- components/style/selector_map.rs | 74 +++++---- components/style/selector_parser.rs | 9 ++ components/style/stylist.rs | 41 ++++- 11 files changed, 296 insertions(+), 69 deletions(-) create mode 100644 components/hashglobe/src/diagnostic.rs diff --git a/components/hashglobe/src/diagnostic.rs b/components/hashglobe/src/diagnostic.rs new file mode 100644 index 00000000000..498ede0ca72 --- /dev/null +++ b/components/hashglobe/src/diagnostic.rs @@ -0,0 +1,143 @@ +use hash_map::HashMap; +use std::borrow::Borrow; +use std::hash::{BuildHasher, Hash}; + +use FailedAllocationError; + +#[derive(Clone, Debug)] +pub struct DiagnosticHashMap + where K: Eq + Hash, + S: BuildHasher +{ + map: HashMap, + readonly: bool, +} + +impl DiagnosticHashMap + where K: Eq + Hash, + S: BuildHasher +{ + #[inline(always)] + pub fn inner(&self) -> &HashMap { + &self.map + } + + #[inline(always)] + pub fn begin_mutation(&mut self) { + assert!(self.readonly); + self.readonly = false; + } + + #[inline(always)] + pub fn end_mutation(&mut self) { + assert!(!self.readonly); + self.readonly = true; + } + + #[inline(always)] + pub fn with_hasher(hash_builder: S) -> Self { + Self { + map: HashMap::::with_hasher(hash_builder), + readonly: true, + } + } + + #[inline(always)] + pub fn len(&self) -> usize { + self.map.len() + } + + #[inline(always)] + pub fn is_empty(&self) -> bool { + self.map.is_empty() + } + + #[inline(always)] + pub fn contains_key(&self, k: &Q) -> bool + where K: Borrow, + Q: Hash + Eq + { + self.map.contains_key(k) + } + + #[inline(always)] + pub fn get(&self, k: &Q) -> Option<&V> + where K: Borrow, + Q: Hash + Eq + { + self.map.get(k) + } + + #[inline(always)] + pub fn try_get_or_insert_with V>( + &mut self, + key: K, + default: F + ) -> Result<&mut V, FailedAllocationError> { + assert!(!self.readonly); + let entry = self.map.try_entry(key)?; + Ok(entry.or_insert_with(default)) + } + + #[inline(always)] + pub fn try_insert(&mut self, k: K, v: V) -> Result, FailedAllocationError> { + assert!(!self.readonly); + self.map.try_insert(k, v) + } + + #[inline(always)] + pub fn remove(&mut self, k: &Q) -> Option + where K: Borrow, + Q: Hash + Eq + { + assert!(!self.readonly); + self.map.remove(k) + } + + #[inline(always)] + pub fn clear(&mut self) where K: 'static, V: 'static { + // We handle scoped mutations for the caller here, since callsites that + // invoke clear() don't benefit from the coalescing we do around insertion. + self.begin_mutation(); + self.map.clear(); + self.end_mutation(); + } +} + +impl PartialEq for DiagnosticHashMap + where K: Eq + Hash, + V: PartialEq, + S: BuildHasher +{ + fn eq(&self, other: &Self) -> bool { + self.map.eq(&other.map) + } +} + +impl Eq for DiagnosticHashMap + where K: Eq + Hash, + V: Eq, + S: BuildHasher +{ +} + +impl Default for DiagnosticHashMap + where K: Eq + Hash, + S: BuildHasher + Default +{ + fn default() -> Self { + Self { + map: HashMap::default(), + readonly: true, + } + } +} + +impl Drop for DiagnosticHashMap + where K: Eq + Hash, + S: BuildHasher +{ + fn drop(&mut self) { + debug_assert!(self.readonly, "Dropped while mutating"); + } +} diff --git a/components/hashglobe/src/fake.rs b/components/hashglobe/src/fake.rs index eed48fc9151..f83032d5a83 100644 --- a/components/hashglobe/src/fake.rs +++ b/components/hashglobe/src/fake.rs @@ -77,10 +77,24 @@ impl HashMap Ok(self.entry(key)) } + #[inline(always)] + pub fn try_get_or_insert_with V>( + &mut self, + key: K, + default: F + ) -> Result<&mut V, FailedAllocationError> { + Ok(self.entry(key).or_insert_with(default)) + } + #[inline] pub fn try_insert(&mut self, k: K, v: V) -> Result, FailedAllocationError> { Ok(self.insert(k, v)) } + + #[inline(always)] + pub fn begin_mutation(&mut self) {} + #[inline(always)] + pub fn end_mutation(&mut self) {} } #[derive(Clone)] diff --git a/components/hashglobe/src/lib.rs b/components/hashglobe/src/lib.rs index 60b5056ff44..480f3a540ea 100644 --- a/components/hashglobe/src/lib.rs +++ b/components/hashglobe/src/lib.rs @@ -11,6 +11,7 @@ extern crate heapsize; pub mod alloc; +pub mod diagnostic; pub mod hash_map; pub mod hash_set; mod shim; diff --git a/components/malloc_size_of/lib.rs b/components/malloc_size_of/lib.rs index 413942e4322..c722c176c5d 100644 --- a/components/malloc_size_of/lib.rs +++ b/components/malloc_size_of/lib.rs @@ -339,6 +339,25 @@ impl MallocSizeOf for hashglobe::hash_map::HashMap } } +impl MallocShallowSizeOf for hashglobe::diagnostic::DiagnosticHashMap + where K: Eq + Hash, + S: BuildHasher +{ + fn shallow_size_of(&self, ops: &mut MallocSizeOfOps) -> usize { + self.inner().shallow_size_of(ops) + } +} + +impl MallocSizeOf for hashglobe::diagnostic::DiagnosticHashMap + where K: Eq + Hash + MallocSizeOf, + V: MallocSizeOf, + S: BuildHasher, +{ + fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize { + self.inner().size_of(ops) + } +} + // XXX: we don't want MallocSizeOf to be defined for Rc and Arc. If negative // trait bounds are ever allowed, this code should be uncommented. // (We do have a compile-fail test for this: diff --git a/components/style/context.rs b/components/style/context.rs index 5ed3f06c6bc..a7fe03ad70c 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -322,8 +322,6 @@ pub struct TraversalStatistics { pub selectors: u32, /// The number of revalidation selectors. pub revalidation_selectors: u32, - /// The number of state/attr dependencies in the dependency set. - pub dependency_selectors: u32, /// The number of declarations in the stylist. pub declarations: u32, /// The number of times the stylist was rebuilt. @@ -344,7 +342,6 @@ impl<'a> ops::Add for &'a TraversalStatistics { "traversal_time_ms should be set at the end by the caller"); debug_assert!(self.selectors == 0, "set at the end"); debug_assert!(self.revalidation_selectors == 0, "set at the end"); - debug_assert!(self.dependency_selectors == 0, "set at the end"); debug_assert!(self.declarations == 0, "set at the end"); debug_assert!(self.stylist_rebuilds == 0, "set at the end"); TraversalStatistics { @@ -355,7 +352,6 @@ impl<'a> ops::Add for &'a TraversalStatistics { styles_reused: self.styles_reused + other.styles_reused, selectors: 0, revalidation_selectors: 0, - dependency_selectors: 0, declarations: 0, stylist_rebuilds: 0, traversal_time_ms: 0.0, @@ -383,7 +379,6 @@ impl fmt::Display for TraversalStatistics { writeln!(f, "[PERF],styles_reused,{}", self.styles_reused)?; writeln!(f, "[PERF],selectors,{}", self.selectors)?; writeln!(f, "[PERF],revalidation_selectors,{}", self.revalidation_selectors)?; - writeln!(f, "[PERF],dependency_selectors,{}", self.dependency_selectors)?; writeln!(f, "[PERF],declarations,{}", self.declarations)?; writeln!(f, "[PERF],stylist_rebuilds,{}", self.stylist_rebuilds)?; writeln!(f, "[PERF],traversal_time_ms,{}", self.traversal_time_ms)?; @@ -405,7 +400,6 @@ 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.num_invalidations() as u32; self.declarations = stylist.num_declarations() as u32; self.stylist_rebuilds = stylist.num_rebuilds() as u32; } diff --git a/components/style/custom_properties.rs b/components/style/custom_properties.rs index 8351248a388..9da84c3cc16 100644 --- a/components/style/custom_properties.rs +++ b/components/style/custom_properties.rs @@ -11,7 +11,7 @@ use cssparser::{Delimiter, Parser, ParserInput, SourcePosition, Token, TokenSeri use parser::ParserContext; use precomputed_hash::PrecomputedHash; use properties::{CSSWideKeyword, DeclaredValue}; -use selector_map::{PrecomputedHashSet, PrecomputedHashMap}; +use selector_map::{PrecomputedHashSet, PrecomputedDiagnosticHashMap}; use selectors::parser::SelectorParseError; use servo_arc::Arc; use std::ascii::AsciiExt; @@ -105,7 +105,7 @@ where /// Key index. index: Vec, /// Key-value map. - values: PrecomputedHashMap, + values: PrecomputedDiagnosticHashMap, } impl OrderedMap @@ -116,7 +116,7 @@ where pub fn new() -> Self { OrderedMap { index: Vec::new(), - values: PrecomputedHashMap::default(), + values: PrecomputedDiagnosticHashMap::default(), } } @@ -125,7 +125,9 @@ where if !self.values.contains_key(&key) { self.index.push(key.clone()); } - self.values.insert(key, value); + self.values.begin_mutation(); + self.values.try_insert(key, value).unwrap(); + self.values.end_mutation(); } /// Get a value given its key. @@ -165,7 +167,10 @@ where None => return None, }; self.index.remove(index); - self.values.remove(key) + self.values.begin_mutation(); + let result = self.values.remove(key); + self.values.end_mutation(); + result } } @@ -196,7 +201,7 @@ where }; self.pos += 1; - let value = &self.inner.values[key]; + let value = &self.inner.values.get(key).unwrap(); Some((key, value)) } diff --git a/components/style/hash.rs b/components/style/hash.rs index 305357a85b2..c99ca3e9e65 100644 --- a/components/style/hash.rs +++ b/components/style/hash.rs @@ -13,11 +13,16 @@ use fnv; pub use hashglobe::hash_map::HashMap; #[cfg(feature = "gecko")] pub use hashglobe::hash_set::HashSet; - +#[cfg(feature = "gecko")] +pub use hashglobe::diagnostic::DiagnosticHashMap; #[cfg(feature = "servo")] pub use hashglobe::fake::{HashMap, HashSet}; +/// Alias to use regular HashMaps everywhere in Servo. +#[cfg(feature = "servo")] +pub type DiagnosticHashMap = HashMap; + /// Appropriate reexports of hash_map types pub mod map { #[cfg(feature = "gecko")] diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index 301cd2e25a9..2e4816aad34 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -178,18 +178,6 @@ impl InvalidationMap { } } - /// Returns the number of dependencies stored in the invalidation map. - pub fn len(&self) -> usize { - self.state_affecting_selectors.len() + - self.other_attribute_affecting_selectors.len() + - self.id_to_selector.iter().fold(0, |accum, (_, ref v)| { - accum + v.len() - }) + - self.class_to_selector.iter().fold(0, |accum, (_, ref v)| { - accum + v.len() - }) - } - /// Adds a selector to this `InvalidationMap`. Returns Err(..) to /// signify OOM. pub fn note_selector( @@ -252,8 +240,7 @@ impl InvalidationMap { for class in compound_visitor.classes { self.class_to_selector - .entry(class, quirks_mode) - .or_insert_with(SmallVec::new) + .try_get_or_insert_with(class, quirks_mode, SmallVec::new)? .try_push(Dependency { selector: selector.clone(), selector_offset: sequence_start, @@ -262,8 +249,7 @@ impl InvalidationMap { for id in compound_visitor.ids { self.id_to_selector - .entry(id, quirks_mode) - .or_insert_with(SmallVec::new) + .try_get_or_insert_with(id, quirks_mode, SmallVec::new)? .try_push(Dependency { selector: selector.clone(), selector_offset: sequence_start, @@ -299,6 +285,22 @@ impl InvalidationMap { Ok(()) } + + /// Allows mutation of this InvalidationMap. + pub fn begin_mutation(&mut self) { + self.class_to_selector.begin_mutation(); + self.id_to_selector.begin_mutation(); + self.state_affecting_selectors.begin_mutation(); + self.other_attribute_affecting_selectors.begin_mutation(); + } + + /// Disallows mutation of this InvalidationMap. + pub fn end_mutation(&mut self) { + self.class_to_selector.end_mutation(); + self.id_to_selector.end_mutation(); + self.state_affecting_selectors.end_mutation(); + self.other_attribute_affecting_selectors.end_mutation(); + } } /// A struct that collects invalidations for a given compound selector. diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index 3b14431c06f..f44eea6fddd 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -10,8 +10,7 @@ use applicable_declarations::ApplicableDeclarationBlock; use context::QuirksMode; use dom::TElement; use fallible::FallibleVec; -use hash::{HashMap, HashSet}; -use hash::map as hash_map; +use hash::{HashMap, HashSet, DiagnosticHashMap}; use hashglobe::FailedAllocationError; use pdqsort::sort_by; use precomputed_hash::PrecomputedHash; @@ -38,6 +37,9 @@ impl Default for PrecomputedHasher { /// A simple alias for a hashmap using PrecomputedHasher. pub type PrecomputedHashMap = HashMap>; +/// A simple alias for a hashmap using PrecomputedHasher. +pub type PrecomputedDiagnosticHashMap = DiagnosticHashMap>; + /// A simple alias for a hashset using PrecomputedHasher. pub type PrecomputedHashSet = HashSet>; @@ -102,7 +104,7 @@ pub struct SelectorMap { /// A hash from a class name to rules which contain that class selector. pub class_hash: MaybeCaseInsensitiveHashMap>, /// A hash from local name to rules which contain that local name selector. - pub local_name_hash: PrecomputedHashMap>, + pub local_name_hash: PrecomputedDiagnosticHashMap>, /// Rules that don't have ID, class, or element selectors. pub other: SmallVec<[T; 1]>, /// The number of entries in this map. @@ -123,7 +125,7 @@ impl SelectorMap { SelectorMap { id_hash: MaybeCaseInsensitiveHashMap::new(), class_hash: MaybeCaseInsensitiveHashMap::new(), - local_name_hash: HashMap::default(), + local_name_hash: DiagnosticHashMap::default(), other: SmallVec::new(), count: 0, } @@ -147,6 +149,20 @@ impl SelectorMap { pub fn len(&self) -> usize { self.count } + + /// Allows mutation of this SelectorMap. + pub fn begin_mutation(&mut self) { + self.id_hash.begin_mutation(); + self.class_hash.begin_mutation(); + self.local_name_hash.begin_mutation(); + } + + /// Disallows mutation of this SelectorMap. + pub fn end_mutation(&mut self) { + self.id_hash.end_mutation(); + self.class_hash.end_mutation(); + self.local_name_hash.end_mutation(); + } } impl SelectorMap { @@ -251,12 +267,10 @@ impl SelectorMap { let vector = match find_bucket(entry.selector()) { Bucket::ID(id) => { - self.id_hash.try_entry(id.clone(), quirks_mode)? - .or_insert_with(SmallVec::new) + self.id_hash.try_get_or_insert_with(id.clone(), quirks_mode, SmallVec::new)? } Bucket::Class(class) => { - self.class_hash.try_entry(class.clone(), quirks_mode)? - .or_insert_with(SmallVec::new) + self.class_hash.try_get_or_insert_with(class.clone(), quirks_mode, SmallVec::new)? } Bucket::LocalName { name, lower_name } => { // If the local name in the selector isn't lowercase, insert it @@ -272,12 +286,10 @@ impl SelectorMap { // subsequent selector matching work will filter them out. if name != lower_name { self.local_name_hash - .try_entry(lower_name.clone())? - .or_insert_with(SmallVec::new) + .try_get_or_insert_with(lower_name.clone(), SmallVec::new)? .try_push(entry.clone())?; } - self.local_name_hash.try_entry(name.clone())? - .or_insert_with(SmallVec::new) + self.local_name_hash.try_get_or_insert_with(name.clone(), SmallVec::new)? } Bucket::Universal => { &mut self.other @@ -463,7 +475,7 @@ fn find_bucket<'a>(mut iter: SelectorIter<'a, SelectorImpl>) -> Bucket<'a> { #[derive(Debug)] #[cfg_attr(feature = "gecko", derive(MallocSizeOf))] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] -pub struct MaybeCaseInsensitiveHashMap(PrecomputedHashMap); +pub struct MaybeCaseInsensitiveHashMap(PrecomputedDiagnosticHashMap); // FIXME(Manishearth) the 'static bound can be removed when // our HashMap fork (hashglobe) is able to use NonZero, @@ -471,32 +483,20 @@ pub struct MaybeCaseInsensitiveHashMap MaybeCaseInsensitiveHashMap { /// Empty map pub fn new() -> Self { - MaybeCaseInsensitiveHashMap(PrecomputedHashMap::default()) + MaybeCaseInsensitiveHashMap(PrecomputedDiagnosticHashMap::default()) } - /// HashMap::entry - pub fn entry(&mut self, mut key: Atom, quirks_mode: QuirksMode) -> hash_map::Entry { - if quirks_mode == QuirksMode::Quirks { - key = key.to_ascii_lowercase() - } - self.0.entry(key) - } - - /// HashMap::try_entry - pub fn try_entry( + /// DiagnosticHashMap::try_get_or_insert_with + pub fn try_get_or_insert_with V>( &mut self, mut key: Atom, - quirks_mode: QuirksMode - ) -> Result, FailedAllocationError> { + quirks_mode: QuirksMode, + default: F, + ) -> Result<&mut V, FailedAllocationError> { if quirks_mode == QuirksMode::Quirks { key = key.to_ascii_lowercase() } - self.0.try_entry(key) - } - - /// HashMap::iter - pub fn iter(&self) -> hash_map::Iter { - self.0.iter() + self.0.try_get_or_insert_with(key, default) } /// HashMap::clear @@ -512,5 +512,15 @@ impl MaybeCaseInsensitiveHashMap { self.0.get(key) } } + + /// DiagnosticHashMap::begin_mutation + pub fn begin_mutation(&mut self) { + self.0.begin_mutation(); + } + + /// DiagnosticHashMap::end_mutation + pub fn end_mutation(&mut self) { + self.0.end_mutation(); + } } diff --git a/components/style/selector_parser.rs b/components/style/selector_parser.rs index f55161a9cc4..316f360daf8 100644 --- a/components/style/selector_parser.rs +++ b/components/style/selector_parser.rs @@ -159,6 +159,15 @@ impl PerPseudoElementMap { *self = Self::default(); } + /// Invokes a callback on each non-None entry. + pub fn for_each(&mut self, mut f: F) { + for entry in self.entries.iter_mut() { + if entry.is_some() { + f(entry.as_mut().unwrap()); + } + } + } + /// Set an entry value. /// /// Returns an error if the element is not a simple pseudo. diff --git a/components/style/stylist.rs b/components/style/stylist.rs index a8bc12085c1..63725dfd722 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -478,12 +478,6 @@ impl Stylist { .map(|(d, _)| d.selectors_for_cache_revalidation.len()).sum() } - /// 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 @@ -1870,6 +1864,32 @@ impl CascadeData { } } + #[cfg(feature = "gecko")] + fn begin_mutation(&mut self, rebuild_kind: &SheetRebuildKind) { + self.element_map.begin_mutation(); + self.pseudos_map.for_each(|m| m.begin_mutation()); + if rebuild_kind.should_rebuild_invalidation() { + self.invalidation_map.begin_mutation(); + self.selectors_for_cache_revalidation.begin_mutation(); + } + } + + #[cfg(feature = "servo")] + fn begin_mutation(&mut self, _: &SheetRebuildKind) {} + + #[cfg(feature = "gecko")] + fn end_mutation(&mut self, rebuild_kind: &SheetRebuildKind) { + self.element_map.end_mutation(); + self.pseudos_map.for_each(|m| m.end_mutation()); + if rebuild_kind.should_rebuild_invalidation() { + self.invalidation_map.end_mutation(); + self.selectors_for_cache_revalidation.end_mutation(); + } + } + + #[cfg(feature = "servo")] + fn end_mutation(&mut self, _: &SheetRebuildKind) {} + /// Collects all the applicable media query results into `results`. /// /// This duplicates part of the logic in `add_stylesheet`, which is @@ -1933,6 +1953,7 @@ impl CascadeData { self.effective_media_query_results.saw_effective(stylesheet); } + self.begin_mutation(&rebuild_kind); for rule in stylesheet.effective_rules(device, guard) { match *rule { CssRule::Style(ref locked) => { @@ -1969,8 +1990,11 @@ impl CascadeData { None => &mut self.element_map, Some(pseudo) => { self.pseudos_map - .get_or_insert_with(&pseudo.canonical(), || Box::new(SelectorMap::new())) - .expect("Unexpected tree pseudo-element?") + .get_or_insert_with(&pseudo.canonical(), || { + let mut map = Box::new(SelectorMap::new()); + map.begin_mutation(); + map + }).expect("Unexpected tree pseudo-element?") } }; @@ -2064,6 +2088,7 @@ impl CascadeData { _ => {} } } + self.end_mutation(&rebuild_kind); Ok(()) }