From b815cabaf7ce5be55321713ae16d5b3c9adba1c2 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 1 Jun 2017 09:17:43 -0700 Subject: [PATCH 01/14] Add a size_of test for Rule. MozReview-Commit-ID: J9y9cl0rpxX --- tests/unit/stylo/size_of.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/unit/stylo/size_of.rs b/tests/unit/stylo/size_of.rs index 7909a69680e..f35177bf058 100644 --- a/tests/unit/stylo/size_of.rs +++ b/tests/unit/stylo/size_of.rs @@ -19,6 +19,11 @@ fn size_of_selectors_dummy_types() { assert_eq!(align_of::(), align_of::()); } +// The size of this is critical to performance on the bloom-basic microbenchmark. +// When iterating over a large Rule array, we want to be able to fast-reject +// selectors (with the inline hashes) with as few cache misses as possible. +size_of_test!(test_size_of_rule, style::stylist::Rule, 64); + size_of_test!(test_size_of_property_declaration, style::properties::PropertyDeclaration, 32); // This is huge, but we allocate it on the stack and then never move it, From e3d3c5a7b29e27a11bf2e4f024110b3afaf9cf6d Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 1 Jun 2017 14:28:13 -0700 Subject: [PATCH 02/14] Hoist specificity_and_flags into ComplexSelector (soon to be Selector). In the upcoming patches we'll eliminate the existing Selector and SelectorInner, and ComplexSelector will become an Arc-managed |Selector|. So the tuple-indexed setup is just temporary. MozReview-Commit-ID: 4CcOYeHGUED --- components/selectors/parser.rs | 40 +++++++++++++++++----------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 197ce4f2629..296a57c6709 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -186,8 +186,8 @@ impl SelectorInner { } /// Creates a SelectorInner from a Vec of Components. Used in tests. - pub fn from_vec(vec: Vec>) -> Self { - let complex = ComplexSelector::from_vec(vec); + pub fn from_vec(vec: Vec>, specificity_and_flags: u32) -> Self { + let complex = ComplexSelector::from_vec(vec, specificity_and_flags); Self::new(complex) } } @@ -195,7 +195,6 @@ impl SelectorInner { #[derive(PartialEq, Eq, Clone)] pub struct Selector { pub inner: SelectorInner, - specificity_and_flags: u32, } impl ::std::borrow::Borrow> for Selector { @@ -208,14 +207,7 @@ const HAS_PSEUDO_BIT: u32 = 1 << 30; impl Selector { pub fn specificity(&self) -> u32 { - self.specificity_and_flags & !HAS_PSEUDO_BIT - } - - pub fn new_for_unit_testing(inner: SelectorInner, specificity: u32) -> Self { - Self { - inner: inner, - specificity_and_flags: specificity, - } + self.inner.complex.specificity() } pub fn pseudo_element(&self) -> Option<&Impl::PseudoElement> { @@ -234,7 +226,7 @@ impl Selector { } pub fn has_pseudo_element(&self) -> bool { - (self.specificity_and_flags & HAS_PSEUDO_BIT) != 0 + self.inner.complex.has_pseudo_element() } /// Whether this selector (pseudo-element part excluded) matches every element. @@ -371,9 +363,17 @@ pub fn namespace_empty_string() -> Impl::NamespaceUrl { /// canonical iteration order is right-to-left (selector matching order). The /// iterators abstract over these details. #[derive(Clone, Eq, PartialEq)] -pub struct ComplexSelector(ArcSlice>); +pub struct ComplexSelector(ArcSlice>, u32); impl ComplexSelector { + pub fn specificity(&self) -> u32 { + self.1 & !HAS_PSEUDO_BIT + } + + pub fn has_pseudo_element(&self) -> bool { + (self.1 & HAS_PSEUDO_BIT) != 0 + } + /// Returns an iterator over the next sequence of simple selectors. When /// a combinator is reached, the iterator will return None, and /// next_sequence() may be called to continue to the next sequence. @@ -407,7 +407,7 @@ impl ComplexSelector { pub fn slice_from(&self, index: usize) -> Self { // Note that we convert the slice_from to slice_to because selectors are // stored left-to-right but logical order is right-to-left. - ComplexSelector(self.0.clone().slice_to(self.0.len() - index)) + ComplexSelector(self.0.clone().slice_to(self.0.len() - index), self.1) } /// Returns a ComplexSelector identical to |self| but with the leftmost @@ -415,12 +415,12 @@ impl ComplexSelector { pub fn slice_to(&self, index: usize) -> Self { // Note that we convert the slice_to to slice_from because selectors are // stored left-to-right but logical order is right-to-left. - ComplexSelector(self.0.clone().slice_from(self.0.len() - index)) + ComplexSelector(self.0.clone().slice_from(self.0.len() - index), self.1) } /// Creates a ComplexSelector from a vec of Components. Used in tests. - pub fn from_vec(vec: Vec>) -> Self { - ComplexSelector(ArcSlice::new(vec.into_boxed_slice())) + pub fn from_vec(vec: Vec>, specificity_and_flags: u32) -> Self { + ComplexSelector(ArcSlice::new(vec.into_boxed_slice()), specificity_and_flags) } } @@ -965,13 +965,13 @@ fn complex_selector_specificity(selector: &ComplexSelector) fn parse_selector(parser: &P, input: &mut CssParser) -> Result, ()> where P: Parser, Impl: SelectorImpl { - let (complex, has_pseudo_element) = parse_complex_selector(parser, input)?; + let (mut complex, has_pseudo_element) = parse_complex_selector(parser, input)?; let mut specificity = specificity(&complex); if has_pseudo_element { specificity |= HAS_PSEUDO_BIT; } + complex.1 = specificity; Ok(Selector { - specificity_and_flags: specificity, inner: SelectorInner::new(complex), }) } @@ -1042,7 +1042,7 @@ fn parse_complex_selector( sequence.push(Component::Combinator(combinator)); } - let complex = ComplexSelector(ArcSlice::new(sequence.into_vec().into_boxed_slice())); + let complex = ComplexSelector(ArcSlice::new(sequence.into_vec().into_boxed_slice()), 0); Ok((complex, parsed_pseudo_element)) } From 713c9a63f63c03f9aefd54664a6c5106cee514d7 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 1 Jun 2017 13:56:21 -0700 Subject: [PATCH 03/14] Move the ancestor hashes out of Selector. MozReview-Commit-ID: 5mipXnjgSED --- components/script/dom/element.rs | 4 +- components/script/dom/node.rs | 4 +- components/selectors/matching.rs | 22 +++--- components/selectors/parser.rs | 81 +++++++++++++------- components/style/invalidation/stylesheets.rs | 4 +- components/style/restyle_hints.rs | 47 +++++++++--- components/style/selector_map.rs | 30 +++++--- components/style/stylist.rs | 40 ++++++---- 8 files changed, 153 insertions(+), 79 deletions(-) diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index be708113def..f875986edd2 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -2061,7 +2061,7 @@ impl ElementMethods for Element { Err(()) => Err(Error::Syntax), Ok(selectors) => { let mut ctx = MatchingContext::new(MatchingMode::Normal, None); - Ok(matches_selector_list(&selectors.0, &Root::from_ref(self), &mut ctx)) + Ok(matches_selector_list(&selectors, &Root::from_ref(self), &mut ctx)) } } } @@ -2080,7 +2080,7 @@ impl ElementMethods for Element { for element in root.inclusive_ancestors() { if let Some(element) = Root::downcast::(element) { let mut ctx = MatchingContext::new(MatchingMode::Normal, None); - if matches_selector_list(&selectors.0, &element, &mut ctx) { + if matches_selector_list(&selectors, &element, &mut ctx) { return Ok(Some(element)); } } diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index cc817587eda..8dde9cc7b01 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -345,7 +345,7 @@ impl<'a> Iterator for QuerySelectorIterator { type Item = Root; fn next(&mut self) -> Option> { - let selectors = &self.selectors.0; + let selectors = &self.selectors; // TODO(cgaebel): Is it worth it to build a bloom filter here // (instead of passing `None`)? Probably. @@ -722,7 +722,7 @@ impl Node { Ok(selectors) => { let mut ctx = MatchingContext::new(MatchingMode::Normal, None); Ok(self.traverse_preorder().filter_map(Root::downcast).find(|element| { - matches_selector_list(&selectors.0, element, &mut ctx) + matches_selector_list(&selectors, element, &mut ctx) })) } } diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index 444d24a7d1c..094da6b3ca7 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -4,8 +4,8 @@ use attr::{ParsedAttrSelectorOperation, AttrSelectorOperation, NamespaceConstraint}; use bloom::BloomFilter; -use parser::{Combinator, ComplexSelector, Component, LocalName}; -use parser::{Selector, SelectorInner, SelectorIter}; +use parser::{AncestorHashes, Combinator, ComplexSelector, Component, LocalName}; +use parser::{SelectorInner, SelectorIter, SelectorList}; use std::borrow::Borrow; use tree::Element; @@ -152,27 +152,29 @@ impl<'a> MatchingContext<'a> { } } -pub fn matches_selector_list(selector_list: &[Selector], +pub fn matches_selector_list(selector_list: &SelectorList, element: &E, context: &mut MatchingContext) -> bool where E: Element { - selector_list.iter().any(|selector| { - matches_selector(&selector.inner, + selector_list.0.iter().any(|selector_and_hashes| { + matches_selector(&selector_and_hashes.selector.inner, + &selector_and_hashes.hashes, element, context, &mut |_, _| {}) }) } -fn may_match(sel: &SelectorInner, +#[inline(always)] +fn may_match(hashes: &AncestorHashes, bf: &BloomFilter) -> bool where E: Element, { // Check against the list of precomputed hashes. - for hash in sel.ancestor_hashes.iter() { + for hash in hashes.0.iter() { // If we hit the 0 sentinel hash, that means the rest are zero as well. if *hash == 0 { break; @@ -330,8 +332,10 @@ enum SelectorMatchingResult { NotMatchedGlobally, } -/// Matches an inner selector. +/// Matches a selector, fast-rejecting against a bloom filter. +#[inline(always)] pub fn matches_selector(selector: &SelectorInner, + hashes: &AncestorHashes, element: &E, context: &mut MatchingContext, flags_setter: &mut F) @@ -341,7 +345,7 @@ pub fn matches_selector(selector: &SelectorInner, { // Use the bloom filter to fast-reject. if let Some(filter) = context.bloom_filter { - if !may_match::(&selector, filter) { + if !may_match::(hashes, filter) { return false; } } diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 296a57c6709..e083ecdab75 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -130,7 +130,27 @@ pub trait Parser { } #[derive(PartialEq, Eq, Clone, Debug)] -pub struct SelectorList(pub Vec>); +pub struct SelectorAndHashes { + pub selector: Selector, + pub hashes: AncestorHashes, +} + +impl SelectorAndHashes { + pub fn new(selector: Selector) -> Self { + let hashes = AncestorHashes::new(&selector); + Self::new_with_hashes(selector, hashes) + } + + pub fn new_with_hashes(selector: Selector, hashes: AncestorHashes) -> Self { + SelectorAndHashes { + selector: selector, + hashes: hashes, + } + } +} + +#[derive(PartialEq, Eq, Clone, Debug)] +pub struct SelectorList(pub Vec>); impl SelectorList { /// Parse a comma-separated list of Selectors. @@ -139,14 +159,41 @@ impl SelectorList { /// Return the Selectors or Err if there is an invalid selector. pub fn parse

(parser: &P, input: &mut CssParser) -> Result where P: Parser { - input.parse_comma_separated(|input| parse_selector(parser, input)) + input.parse_comma_separated(|input| parse_selector(parser, input).map(SelectorAndHashes::new)) .map(SelectorList) } } -/// Copied from Gecko, where it was noted to be unmeasured. +/// Copied from Gecko, who copied it from WebKit. Note that increasing the +/// number of hashes here will adversely affect the cache hit when fast- +/// rejecting long lists of Rules with inline hashes. const NUM_ANCESTOR_HASHES: usize = 4; +/// Ancestor hashes for the bloom filter. We precompute these and store them +/// inline with selectors to optimize cache performance during matching. +/// This matters a lot. +#[derive(Eq, PartialEq, Clone, Debug)] +pub struct AncestorHashes(pub [u32; NUM_ANCESTOR_HASHES]); + +impl AncestorHashes { + pub fn new(c: &Selector) -> Self { + let mut hashes = [0; NUM_ANCESTOR_HASHES]; + // Compute ancestor hashes for the bloom filter. + let mut hash_iter = c.inner.complex.iter_ancestors() + .map(|x| x.ancestor_hash()) + .filter(|x| x.is_some()) + .map(|x| x.unwrap()); + for i in 0..NUM_ANCESTOR_HASHES { + hashes[i] = match hash_iter.next() { + Some(x) => x, + None => break, + } + } + + AncestorHashes(hashes) + } +} + /// The cores parts of a selector used for matching. This exists to make that /// information accessibly separately from the specificity and pseudo-element /// information that lives on |Selector| proper. We may want to refactor things @@ -156,32 +203,12 @@ const NUM_ANCESTOR_HASHES: usize = 4; pub struct SelectorInner { /// The selector data. pub complex: ComplexSelector, - /// Ancestor hashes for the bloom filter. We precompute these and store - /// them inline to optimize cache performance during selector matching. - /// This matters a lot. - pub ancestor_hashes: [u32; NUM_ANCESTOR_HASHES], } impl SelectorInner { pub fn new(c: ComplexSelector) -> Self { - let mut hashes = [0; NUM_ANCESTOR_HASHES]; - { - // Compute ancestor hashes for the bloom filter. - let mut hash_iter = c.iter_ancestors() - .map(|x| x.ancestor_hash()) - .filter(|x| x.is_some()) - .map(|x| x.unwrap()); - for i in 0..NUM_ANCESTOR_HASHES { - hashes[i] = match hash_iter.next() { - Some(x) => x, - None => break, - } - } - } - SelectorInner { complex: c, - ancestor_hashes: hashes, } } @@ -590,7 +617,7 @@ pub enum Component { impl Component { /// Compute the ancestor hash to check against the bloom filter. - fn ancestor_hash(&self) -> Option { + pub fn ancestor_hash(&self) -> Option { match *self { Component::LocalName(LocalName { ref name, ref lower_name }) => { // Only insert the local-name into the filter if it's all lowercase. @@ -665,10 +692,10 @@ impl ToCss for SelectorList { let mut iter = self.0.iter(); let first = iter.next() .expect("Empty SelectorList, should contain at least one selector"); - first.to_css(dest)?; - for selector in iter { + first.selector.to_css(dest)?; + for selector_and_hashes in iter { dest.write_str(", ")?; - selector.to_css(dest)?; + selector_and_hashes.selector.to_css(dest)?; } Ok(()) } diff --git a/components/style/invalidation/stylesheets.rs b/components/style/invalidation/stylesheets.rs index 93f0d8783ad..adfb4b97a59 100644 --- a/components/style/invalidation/stylesheets.rs +++ b/components/style/invalidation/stylesheets.rs @@ -262,8 +262,8 @@ impl StylesheetInvalidationSet { match *rule { Style(ref lock) => { let style_rule = lock.read_with(guard); - for selector in &style_rule.selectors.0 { - self.collect_scopes(selector); + for selector_and_hashes in &style_rule.selectors.0 { + self.collect_scopes(&selector_and_hashes.selector); if self.fully_invalid { return; } diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index acca66d6fdb..615445bef2a 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -22,7 +22,8 @@ use selectors::Element; use selectors::attr::{AttrSelectorOperation, NamespaceConstraint}; use selectors::matching::{ElementSelectorFlags, MatchingContext, MatchingMode}; use selectors::matching::{RelevantLinkStatus, VisitedHandlingMode, matches_selector}; -use selectors::parser::{Combinator, Component, Selector, SelectorInner, SelectorMethods}; +use selectors::parser::{AncestorHashes, Combinator, Component}; +use selectors::parser::{Selector, SelectorAndHashes, SelectorInner, SelectorMethods}; use selectors::visitor::SelectorVisitor; use smallvec::SmallVec; use std::cell::Cell; @@ -867,7 +868,9 @@ impl Sensitivities { #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct Dependency { #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] - selector: SelectorInner, + selector: Selector, + #[cfg_attr(feature = "servo", ignore_heap_size_of = "No heap data")] + hashes: AncestorHashes, /// The hint associated with this dependency. pub hint: RestyleHint, /// The sensitivities associated with this dependency. @@ -875,9 +878,13 @@ pub struct Dependency { } impl SelectorMapEntry for Dependency { - fn selector(&self) -> &SelectorInner { + fn selector(&self) -> &Selector { &self.selector } + + fn hashes(&self) -> &AncestorHashes { + &self.hashes + } } /// The following visitor visits all the simple selectors for a given complex @@ -940,9 +947,9 @@ pub enum HintComputationContext<'a, E: 'a> impl DependencySet { /// Adds a selector to this `DependencySet`. - pub fn note_selector(&mut self, selector: &Selector) { + pub fn note_selector(&mut self, selector_and_hashes: &SelectorAndHashes) { let mut combinator = None; - let mut iter = selector.inner.complex.iter(); + let mut iter = selector_and_hashes.selector.inner.complex.iter(); let mut index = 0; let mut child_combinators_seen = 0; let mut saw_descendant_combinator = false; @@ -992,17 +999,25 @@ impl DependencySet { None => RestyleHint::for_self(), }; - let dep_selector = if sequence_start == 0 { + let (dep_selector, hashes) = if sequence_start == 0 { // Reuse the bloom hashes if this is the base selector. - selector.inner.clone() + (selector_and_hashes.selector.clone(), + selector_and_hashes.hashes.clone()) + } else { - SelectorInner::new(selector.inner.complex.slice_from(sequence_start)) + let inner = SelectorInner::new(selector_and_hashes.selector + .inner + .complex.slice_from(sequence_start)); + let selector = Selector { inner: inner }; + let hashes = AncestorHashes::new(&selector); + (selector, hashes) }; self.dependencies.insert(Dependency { sensitivities: visitor.sensitivities, hint: hint, selector: dep_selector, + hashes: hashes, }); } @@ -1130,14 +1145,18 @@ impl DependencySet { MatchingContext::new_for_visited(MatchingMode::Normal, None, VisitedHandlingMode::AllLinksUnvisited); let matched_then = - matches_selector(&dep.selector, &snapshot_el, + matches_selector(&dep.selector.inner, + &dep.hashes, + &snapshot_el, &mut then_context, &mut |_, _| {}); let mut now_context = MatchingContext::new_for_visited(MatchingMode::Normal, bloom_filter, VisitedHandlingMode::AllLinksUnvisited); let matches_now = - matches_selector(&dep.selector, el, + matches_selector(&dep.selector.inner, + &dep.hashes, + el, &mut now_context, &mut |_, _| {}); @@ -1162,12 +1181,16 @@ impl DependencySet { dep.sensitivities.states.intersects(IN_VISITED_OR_UNVISITED_STATE) { then_context.visited_handling = VisitedHandlingMode::RelevantLinkVisited; let matched_then = - matches_selector(&dep.selector, &snapshot_el, + matches_selector(&dep.selector.inner, + &dep.hashes, + &snapshot_el, &mut then_context, &mut |_, _| {}); now_context.visited_handling = VisitedHandlingMode::RelevantLinkVisited; let matches_now = - matches_selector(&dep.selector, el, + matches_selector(&dep.selector.inner, + &dep.hashes, + el, &mut now_context, &mut |_, _| {}); if matched_then != matches_now { diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index 846d7972811..4120f000704 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -12,7 +12,7 @@ use pdqsort::sort_by; use rule_tree::CascadeLevel; use selector_parser::SelectorImpl; use selectors::matching::{matches_selector, MatchingContext, ElementSelectorFlags}; -use selectors::parser::{Component, Combinator, SelectorInner}; +use selectors::parser::{AncestorHashes, Component, Combinator, Selector, SelectorAndHashes}; use selectors::parser::LocalName as LocalNameSelector; use smallvec::VecLike; use std::borrow::Borrow; @@ -22,13 +22,20 @@ use stylist::{ApplicableDeclarationBlock, Rule}; /// A trait to abstract over a given selector map entry. pub trait SelectorMapEntry : Sized + Clone { - /// Get the selector we should use to index in the selector map. - fn selector(&self) -> &SelectorInner; + /// Gets the selector we should use to index in the selector map. + fn selector(&self) -> &Selector; + + /// Gets the ancestor hashes associated with the selector. + fn hashes(&self) -> &AncestorHashes; } -impl SelectorMapEntry for SelectorInner { - fn selector(&self) -> &SelectorInner { - self +impl SelectorMapEntry for SelectorAndHashes { + fn selector(&self) -> &Selector { + &self.selector + } + + fn hashes(&self) -> &AncestorHashes { + &self.hashes } } @@ -225,6 +232,7 @@ impl SelectorMap { { for rule in rules { if matches_selector(&rule.selector.inner, + &rule.hashes, element, context, flags_setter) { @@ -390,12 +398,12 @@ impl SelectorMap { /// /// Effectively, pseudo-elements are ignored, given only state pseudo-classes /// may appear before them. -fn find_from_right(selector: &SelectorInner, +fn find_from_right(selector: &Selector, mut f: F) -> Option where F: FnMut(&Component) -> Option, { - let mut iter = selector.complex.iter(); + let mut iter = selector.inner.complex.iter(); for ss in &mut iter { if let Some(r) = f(ss) { return Some(r) @@ -414,7 +422,7 @@ fn find_from_right(selector: &SelectorInner, } /// Retrieve the first ID name in the selector, or None otherwise. -pub fn get_id_name(selector: &SelectorInner) +pub fn get_id_name(selector: &Selector) -> Option { find_from_right(selector, |ss| { // TODO(pradeep): Implement case-sensitivity based on the @@ -427,7 +435,7 @@ pub fn get_id_name(selector: &SelectorInner) } /// Retrieve the FIRST class name in the selector, or None otherwise. -pub fn get_class_name(selector: &SelectorInner) +pub fn get_class_name(selector: &Selector) -> Option { find_from_right(selector, |ss| { // TODO(pradeep): Implement case-sensitivity based on the @@ -440,7 +448,7 @@ pub fn get_class_name(selector: &SelectorInner) } /// Retrieve the name if it is a type selector, or None otherwise. -pub fn get_local_name(selector: &SelectorInner) +pub fn get_local_name(selector: &Selector) -> Option> { find_from_right(selector, |ss| { if let Component::LocalName(ref n) = *ss { diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 5df0bf2717b..010d37e1e53 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -28,7 +28,8 @@ use selectors::attr::NamespaceConstraint; use selectors::bloom::BloomFilter; use selectors::matching::{ElementSelectorFlags, matches_selector, MatchingContext, MatchingMode}; use selectors::matching::AFFECTED_BY_PRESENTATIONAL_HINTS; -use selectors::parser::{Combinator, Component, Selector, SelectorInner, SelectorIter, SelectorMethods}; +use selectors::parser::{AncestorHashes, Combinator, Component, Selector, SelectorAndHashes}; +use selectors::parser::{SelectorIter, SelectorMethods}; use selectors::visitor::SelectorVisitor; use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards}; use sink::Push; @@ -159,7 +160,7 @@ pub struct Stylist { /// 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>, + selectors_for_cache_revalidation: SelectorMap>, /// The total number of selectors. num_selectors: usize, @@ -454,10 +455,10 @@ impl Stylist { CssRule::Style(ref locked) => { let style_rule = locked.read_with(&guard); self.num_declarations += style_rule.block.read_with(&guard).len(); - for selector in &style_rule.selectors.0 { + for selector_and_hashes in &style_rule.selectors.0 { self.num_selectors += 1; - let map = if let Some(pseudo) = selector.pseudo_element() { + let map = if let Some(pseudo) = selector_and_hashes.selector.pseudo_element() { self.pseudos_map .entry(pseudo.canonical()) .or_insert_with(PerPseudoElementSelectorMap::new) @@ -466,20 +467,21 @@ impl Stylist { self.element_map.borrow_for_origin(&stylesheet.origin) }; - map.insert(Rule::new(selector.clone(), + map.insert(Rule::new(selector_and_hashes.selector.clone(), + selector_and_hashes.hashes.clone(), locked.clone(), self.rules_source_order)); - self.dependencies.note_selector(selector); - if needs_revalidation(selector) { - self.selectors_for_cache_revalidation.insert(selector.inner.clone()); + self.dependencies.note_selector(selector_and_hashes); + if needs_revalidation(&selector_and_hashes.selector) { + self.selectors_for_cache_revalidation.insert(selector_and_hashes.clone()); } - selector.visit(&mut AttributeAndStateDependencyVisitor { + selector_and_hashes.selector.visit(&mut AttributeAndStateDependencyVisitor { attribute_dependencies: &mut self.attribute_dependencies, style_attribute_dependency: &mut self.style_attribute_dependency, state_dependencies: &mut self.state_dependencies, }); - selector.visit(&mut MappedIdVisitor { + selector_and_hashes.selector.visit(&mut MappedIdVisitor { mapped_ids: &mut self.mapped_ids, }); } @@ -1130,8 +1132,9 @@ 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, &mut |selector| { - results.push(matches_selector(selector, + self.selectors_for_cache_revalidation.lookup(*element, &mut |selector_and_hashes| { + results.push(matches_selector(&selector_and_hashes.selector.inner, + &selector_and_hashes.hashes, element, &mut matching_context, flags_setter)); @@ -1410,6 +1413,9 @@ pub struct Rule { /// can ruin performance when there are a lot of rules. #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] pub selector: Selector, + /// The ancestor hashes associated with the selector. + #[cfg_attr(feature = "servo", ignore_heap_size_of = "No heap data")] + pub hashes: AncestorHashes, /// The actual style rule. #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] pub style_rule: Arc>, @@ -1418,8 +1424,12 @@ pub struct Rule { } impl SelectorMapEntry for Rule { - fn selector(&self) -> &SelectorInner { - &self.selector.inner + fn selector(&self) -> &Selector { + &self.selector + } + + fn hashes(&self) -> &AncestorHashes { + &self.hashes } } @@ -1444,12 +1454,14 @@ impl Rule { /// Creates a new Rule. pub fn new(selector: Selector, + hashes: AncestorHashes, style_rule: Arc>, source_order: usize) -> Self { Rule { selector: selector, + hashes: hashes, style_rule: style_rule, source_order: source_order, } From 5ddabef636fa91c2d0d0a9d9c90d2a7c71874ca6 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 1 Jun 2017 15:38:33 -0700 Subject: [PATCH 04/14] Collapse Selector, SelectorInner, and ComplexSelector into a single Selector. The refcounting is still internal. We'll fix that up next. MozReview-Commit-ID: CTxZNaR3Qgj --- components/selectors/matching.rs | 14 +- components/selectors/parser.rs | 169 ++++++------------- components/style/gecko/selector_parser.rs | 6 +- components/style/invalidation/stylesheets.rs | 2 +- components/style/restyle_hints.rs | 21 +-- components/style/selector_map.rs | 4 +- components/style/stylist.rs | 2 +- 7 files changed, 71 insertions(+), 147 deletions(-) diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index 094da6b3ca7..133e3232d04 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -4,8 +4,8 @@ use attr::{ParsedAttrSelectorOperation, AttrSelectorOperation, NamespaceConstraint}; use bloom::BloomFilter; -use parser::{AncestorHashes, Combinator, ComplexSelector, Component, LocalName}; -use parser::{SelectorInner, SelectorIter, SelectorList}; +use parser::{AncestorHashes, Combinator, Component, LocalName}; +use parser::{Selector, SelectorIter, SelectorList}; use std::borrow::Borrow; use tree::Element; @@ -159,7 +159,7 @@ pub fn matches_selector_list(selector_list: &SelectorList, where E: Element { selector_list.0.iter().any(|selector_and_hashes| { - matches_selector(&selector_and_hashes.selector.inner, + matches_selector(&selector_and_hashes.selector, &selector_and_hashes.hashes, element, context, @@ -334,7 +334,7 @@ enum SelectorMatchingResult { /// Matches a selector, fast-rejecting against a bloom filter. #[inline(always)] -pub fn matches_selector(selector: &SelectorInner, +pub fn matches_selector(selector: &Selector, hashes: &AncestorHashes, element: &E, context: &mut MatchingContext, @@ -350,13 +350,11 @@ pub fn matches_selector(selector: &SelectorInner, } } - matches_complex_selector(&selector.complex, element, context, flags_setter) + matches_complex_selector(selector, element, context, flags_setter) } /// Matches a complex selector. -/// -/// Use `matches_selector` if you need to skip pseudos. -pub fn matches_complex_selector(complex_selector: &ComplexSelector, +pub fn matches_complex_selector(complex_selector: &Selector, element: &E, context: &mut MatchingContext, flags_setter: &mut F) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index e083ecdab75..c442d24ab9d 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -176,10 +176,10 @@ const NUM_ANCESTOR_HASHES: usize = 4; pub struct AncestorHashes(pub [u32; NUM_ANCESTOR_HASHES]); impl AncestorHashes { - pub fn new(c: &Selector) -> Self { + pub fn new(s: &Selector) -> Self { let mut hashes = [0; NUM_ANCESTOR_HASHES]; // Compute ancestor hashes for the bloom filter. - let mut hash_iter = c.inner.complex.iter_ancestors() + let mut hash_iter = s.iter_ancestors() .map(|x| x.ancestor_hash()) .filter(|x| x.is_some()) .map(|x| x.unwrap()); @@ -194,81 +194,8 @@ impl AncestorHashes { } } -/// The cores parts of a selector used for matching. This exists to make that -/// information accessibly separately from the specificity and pseudo-element -/// information that lives on |Selector| proper. We may want to refactor things -/// and move that information elsewhere, at which point we could rename this -/// to |Selector|. -#[derive(PartialEq, Eq, Clone)] -pub struct SelectorInner { - /// The selector data. - pub complex: ComplexSelector, -} - -impl SelectorInner { - pub fn new(c: ComplexSelector) -> Self { - SelectorInner { - complex: c, - } - } - - /// Creates a SelectorInner from a Vec of Components. Used in tests. - pub fn from_vec(vec: Vec>, specificity_and_flags: u32) -> Self { - let complex = ComplexSelector::from_vec(vec, specificity_and_flags); - Self::new(complex) - } -} - -#[derive(PartialEq, Eq, Clone)] -pub struct Selector { - pub inner: SelectorInner, -} - -impl ::std::borrow::Borrow> for Selector { - fn borrow(&self) -> &SelectorInner { - &self.inner - } -} - const HAS_PSEUDO_BIT: u32 = 1 << 30; -impl Selector { - pub fn specificity(&self) -> u32 { - self.inner.complex.specificity() - } - - pub fn pseudo_element(&self) -> Option<&Impl::PseudoElement> { - if !self.has_pseudo_element() { - return None - } - - for component in self.inner.complex.iter() { - if let Component::PseudoElement(ref pseudo) = *component { - return Some(pseudo) - } - } - - debug_assert!(false, "has_pseudo_element lied!"); - None - } - - pub fn has_pseudo_element(&self) -> bool { - self.inner.complex.has_pseudo_element() - } - - /// Whether this selector (pseudo-element part excluded) matches every element. - /// - /// Used for "pre-computed" pseudo-elements in components/style/stylist.rs - pub fn is_universal(&self) -> bool { - self.inner.complex.iter_raw().all(|c| matches!(*c, - Component::ExplicitUniversalType | - Component::ExplicitAnyNamespace | - Component::Combinator(Combinator::PseudoElement) | - Component::PseudoElement(..) - )) - } -} - pub trait SelectorMethods { type Impl: SelectorImpl; @@ -279,16 +206,6 @@ pub trait SelectorMethods { impl SelectorMethods for Selector { type Impl = Impl; - fn visit(&self, visitor: &mut V) -> bool - where V: SelectorVisitor, - { - self.inner.complex.visit(visitor) - } -} - -impl SelectorMethods for ComplexSelector { - type Impl = Impl; - fn visit(&self, visitor: &mut V) -> bool where V: SelectorVisitor, { @@ -381,7 +298,7 @@ pub fn namespace_empty_string() -> Impl::NamespaceUrl { Impl::NamespaceUrl::default() } -/// A ComplexSelectors stores a sequence of simple selectors and combinators. The +/// A Selector stores a sequence of simple selectors and combinators. The /// iterator classes allow callers to iterate at either the raw sequence level or /// at the level of sequences of simple selectors separated by combinators. Most /// callers want the higher-level iterator. @@ -390,9 +307,9 @@ pub fn namespace_empty_string() -> Impl::NamespaceUrl { /// canonical iteration order is right-to-left (selector matching order). The /// iterators abstract over these details. #[derive(Clone, Eq, PartialEq)] -pub struct ComplexSelector(ArcSlice>, u32); +pub struct Selector(ArcSlice>, u32); -impl ComplexSelector { +impl Selector { pub fn specificity(&self) -> u32 { self.1 & !HAS_PSEUDO_BIT } @@ -401,6 +318,34 @@ impl ComplexSelector { (self.1 & HAS_PSEUDO_BIT) != 0 } + pub fn pseudo_element(&self) -> Option<&Impl::PseudoElement> { + if !self.has_pseudo_element() { + return None + } + + for component in self.iter() { + if let Component::PseudoElement(ref pseudo) = *component { + return Some(pseudo) + } + } + + debug_assert!(false, "has_pseudo_element lied!"); + None + } + + /// Whether this selector (pseudo-element part excluded) matches every element. + /// + /// Used for "pre-computed" pseudo-elements in components/style/stylist.rs + pub fn is_universal(&self) -> bool { + self.iter_raw().all(|c| matches!(*c, + Component::ExplicitUniversalType | + Component::ExplicitAnyNamespace | + Component::Combinator(Combinator::PseudoElement) | + Component::PseudoElement(..) + )) + } + + /// Returns an iterator over the next sequence of simple selectors. When /// a combinator is reached, the iterator will return None, and /// next_sequence() may be called to continue to the next sequence. @@ -429,25 +374,25 @@ impl ComplexSelector { AncestorIter::new(self.iter()) } - /// Returns a ComplexSelector identical to |self| but with the rightmost |index| - /// entries removed. + /// Returns a Selector identical to |self| but with the rightmost |index| entries + /// removed. pub fn slice_from(&self, index: usize) -> Self { // Note that we convert the slice_from to slice_to because selectors are // stored left-to-right but logical order is right-to-left. - ComplexSelector(self.0.clone().slice_to(self.0.len() - index), self.1) + Selector(self.0.clone().slice_to(self.0.len() - index), self.1) } - /// Returns a ComplexSelector identical to |self| but with the leftmost - /// |len() - index| entries removed. + /// Returns a Selector identical to |self| but with the leftmost |len() - index| + /// entries removed. pub fn slice_to(&self, index: usize) -> Self { // Note that we convert the slice_to to slice_from because selectors are // stored left-to-right but logical order is right-to-left. - ComplexSelector(self.0.clone().slice_from(self.0.len() - index), self.1) + Selector(self.0.clone().slice_from(self.0.len() - index), self.1) } - /// Creates a ComplexSelector from a vec of Components. Used in tests. + /// Creates a Selector from a vec of Components. Used in tests. pub fn from_vec(vec: Vec>, specificity_and_flags: u32) -> Self { - ComplexSelector(ArcSlice::new(vec.into_boxed_slice()), specificity_and_flags) + Selector(ArcSlice::new(vec.into_boxed_slice()), specificity_and_flags) } } @@ -671,12 +616,6 @@ impl Debug for Selector { } } -impl Debug for SelectorInner { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.complex.to_css(f) } -} -impl Debug for ComplexSelector { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.to_css(f) } -} impl Debug for Component { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.to_css(f) } } @@ -702,12 +641,6 @@ impl ToCss for SelectorList { } impl ToCss for Selector { - fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { - self.inner.complex.to_css(dest) - } -} - -impl ToCss for ComplexSelector { fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { for item in self.iter_raw_rev() { item.to_css(dest)?; @@ -916,13 +849,13 @@ impl From for u32 { } } -fn specificity(complex_selector: &ComplexSelector) -> u32 +fn specificity(complex_selector: &Selector) -> u32 where Impl: SelectorImpl { complex_selector_specificity(complex_selector).into() } -fn complex_selector_specificity(selector: &ComplexSelector) +fn complex_selector_specificity(selector: &Selector) -> Specificity where Impl: SelectorImpl { @@ -992,15 +925,13 @@ fn complex_selector_specificity(selector: &ComplexSelector) fn parse_selector(parser: &P, input: &mut CssParser) -> Result, ()> where P: Parser, Impl: SelectorImpl { - let (mut complex, has_pseudo_element) = parse_complex_selector(parser, input)?; - let mut specificity = specificity(&complex); + let (mut selector, has_pseudo_element) = parse_complex_selector(parser, input)?; + let mut specificity = specificity(&selector); if has_pseudo_element { specificity |= HAS_PSEUDO_BIT; } - complex.1 = specificity; - Ok(Selector { - inner: SelectorInner::new(complex), - }) + selector.1 = specificity; + Ok(selector) } /// We use a SmallVec for parsing to avoid extra reallocs compared to using a Vec @@ -1021,7 +952,7 @@ type ParseVec = SmallVec<[Component; 8]>; fn parse_complex_selector( parser: &P, input: &mut CssParser) - -> Result<(ComplexSelector, bool), ()> + -> Result<(Selector, bool), ()> where P: Parser, Impl: SelectorImpl { let mut sequence = ParseVec::new(); @@ -1069,11 +1000,11 @@ fn parse_complex_selector( sequence.push(Component::Combinator(combinator)); } - let complex = ComplexSelector(ArcSlice::new(sequence.into_vec().into_boxed_slice()), 0); + let complex = Selector(ArcSlice::new(sequence.into_vec().into_boxed_slice()), 0); Ok((complex, parsed_pseudo_element)) } -impl ComplexSelector { +impl Selector { /// Parse a complex selector, without any pseudo-element. pub fn parse

(parser: &P, input: &mut CssParser) -> Result where P: Parser diff --git a/components/style/gecko/selector_parser.rs b/components/style/gecko/selector_parser.rs index b6051f1f34c..90d503f58ab 100644 --- a/components/style/gecko/selector_parser.rs +++ b/components/style/gecko/selector_parser.rs @@ -8,7 +8,7 @@ use cssparser::{Parser, ToCss}; use element_state::ElementState; use gecko_bindings::structs::CSSPseudoClassType; use selector_parser::{SelectorParser, PseudoElementCascadeType}; -use selectors::parser::{ComplexSelector, SelectorMethods}; +use selectors::parser::{Selector, SelectorMethods}; use selectors::visitor::SelectorVisitor; use std::borrow::Cow; use std::fmt; @@ -47,7 +47,7 @@ macro_rules! pseudo_class_name { /// /// TODO(emilio): We disallow combinators and pseudos here, so we /// should use SimpleSelector instead - MozAny(Box<[ComplexSelector]>), + MozAny(Box<[Selector]>), } } } @@ -273,7 +273,7 @@ impl<'a> ::selectors::Parser for SelectorParser<'a> { }, )* "-moz-any" => { let selectors = parser.parse_comma_separated(|input| { - ComplexSelector::parse(self, input) + Selector::parse(self, input) })?; // Selectors inside `:-moz-any` may not include combinators. if selectors.iter().flat_map(|x| x.iter_raw()).any(|s| s.is_combinator()) { diff --git a/components/style/invalidation/stylesheets.rs b/components/style/invalidation/stylesheets.rs index adfb4b97a59..a222b611d44 100644 --- a/components/style/invalidation/stylesheets.rs +++ b/components/style/invalidation/stylesheets.rs @@ -218,7 +218,7 @@ impl StylesheetInvalidationSet { let mut scope: Option = None; let mut scan = true; - let mut iter = selector.inner.complex.iter(); + let mut iter = selector.iter(); loop { for component in &mut iter { diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 615445bef2a..1a65e821e86 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -23,7 +23,7 @@ use selectors::attr::{AttrSelectorOperation, NamespaceConstraint}; use selectors::matching::{ElementSelectorFlags, MatchingContext, MatchingMode}; use selectors::matching::{RelevantLinkStatus, VisitedHandlingMode, matches_selector}; use selectors::parser::{AncestorHashes, Combinator, Component}; -use selectors::parser::{Selector, SelectorAndHashes, SelectorInner, SelectorMethods}; +use selectors::parser::{Selector, SelectorAndHashes, SelectorMethods}; use selectors::visitor::SelectorVisitor; use smallvec::SmallVec; use std::cell::Cell; @@ -949,7 +949,7 @@ impl DependencySet { /// Adds a selector to this `DependencySet`. pub fn note_selector(&mut self, selector_and_hashes: &SelectorAndHashes) { let mut combinator = None; - let mut iter = selector_and_hashes.selector.inner.complex.iter(); + let mut iter = selector_and_hashes.selector.iter(); let mut index = 0; let mut child_combinators_seen = 0; let mut saw_descendant_combinator = false; @@ -1001,14 +1001,9 @@ impl DependencySet { let (dep_selector, hashes) = if sequence_start == 0 { // Reuse the bloom hashes if this is the base selector. - (selector_and_hashes.selector.clone(), - selector_and_hashes.hashes.clone()) - + (selector_and_hashes.selector.clone(), selector_and_hashes.hashes.clone()) } else { - let inner = SelectorInner::new(selector_and_hashes.selector - .inner - .complex.slice_from(sequence_start)); - let selector = Selector { inner: inner }; + let selector = selector_and_hashes.selector.slice_from(sequence_start); let hashes = AncestorHashes::new(&selector); (selector, hashes) }; @@ -1145,7 +1140,7 @@ impl DependencySet { MatchingContext::new_for_visited(MatchingMode::Normal, None, VisitedHandlingMode::AllLinksUnvisited); let matched_then = - matches_selector(&dep.selector.inner, + matches_selector(&dep.selector, &dep.hashes, &snapshot_el, &mut then_context, @@ -1154,7 +1149,7 @@ impl DependencySet { MatchingContext::new_for_visited(MatchingMode::Normal, bloom_filter, VisitedHandlingMode::AllLinksUnvisited); let matches_now = - matches_selector(&dep.selector.inner, + matches_selector(&dep.selector, &dep.hashes, el, &mut now_context, @@ -1181,14 +1176,14 @@ impl DependencySet { dep.sensitivities.states.intersects(IN_VISITED_OR_UNVISITED_STATE) { then_context.visited_handling = VisitedHandlingMode::RelevantLinkVisited; let matched_then = - matches_selector(&dep.selector.inner, + matches_selector(&dep.selector, &dep.hashes, &snapshot_el, &mut then_context, &mut |_, _| {}); now_context.visited_handling = VisitedHandlingMode::RelevantLinkVisited; let matches_now = - matches_selector(&dep.selector.inner, + matches_selector(&dep.selector, &dep.hashes, el, &mut now_context, diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index 4120f000704..3a295bb2588 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -231,7 +231,7 @@ impl SelectorMap { F: FnMut(&E, ElementSelectorFlags), { for rule in rules { - if matches_selector(&rule.selector.inner, + if matches_selector(&rule.selector, &rule.hashes, element, context, @@ -403,7 +403,7 @@ fn find_from_right(selector: &Selector, -> Option where F: FnMut(&Component) -> Option, { - let mut iter = selector.inner.complex.iter(); + let mut iter = selector.iter(); for ss in &mut iter { if let Some(r) = f(ss) { return Some(r) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 010d37e1e53..b90e6fc42ab 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1133,7 +1133,7 @@ impl Stylist { // this in the caller by asserting that the bitvecs are same-length. let mut results = BitVec::new(); self.selectors_for_cache_revalidation.lookup(*element, &mut |selector_and_hashes| { - results.push(matches_selector(&selector_and_hashes.selector.inner, + results.push(matches_selector(&selector_and_hashes.selector, &selector_and_hashes.hashes, element, &mut matching_context, From 1281fd935358aa5d85d46176734a3e4055091059 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 1 Jun 2017 16:47:41 -0700 Subject: [PATCH 05/14] Stop slicing selectors when noting dependencies, and match with an offset instead. MozReview-Commit-ID: KLqmdRKygO0 --- components/selectors/matching.rs | 20 +++++++++++++--- components/selectors/parser.rs | 38 ++++++++++++------------------- components/style/gecko/wrapper.rs | 2 +- components/style/restyle_hints.rs | 32 +++++++++++++++++--------- components/style/selector_map.rs | 34 +++++++++++++++------------ components/style/stylist.rs | 5 ++-- 6 files changed, 76 insertions(+), 55 deletions(-) diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index 133e3232d04..3a2ddfd998a 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -160,6 +160,7 @@ pub fn matches_selector_list(selector_list: &SelectorList, { selector_list.0.iter().any(|selector_and_hashes| { matches_selector(&selector_and_hashes.selector, + 0, &selector_and_hashes.hashes, element, context, @@ -333,8 +334,16 @@ enum SelectorMatchingResult { } /// Matches a selector, fast-rejecting against a bloom filter. +/// +/// We accept an offset to allow consumers to represent and match against partial +/// selectors (indexed from the right). We use this API design, rather than +/// having the callers pass a SelectorIter, because creating a SelectorIter +/// requires dereferencing the selector to get the length, which adds an +/// unncessary cache miss for cases when we can fast-reject with AncestorHashes +/// (which the caller can store inline with the selector pointer). #[inline(always)] pub fn matches_selector(selector: &Selector, + offset: usize, hashes: &AncestorHashes, element: &E, context: &mut MatchingContext, @@ -350,11 +359,12 @@ pub fn matches_selector(selector: &Selector, } } - matches_complex_selector(selector, element, context, flags_setter) + matches_complex_selector(selector, offset, element, context, flags_setter) } /// Matches a complex selector. pub fn matches_complex_selector(complex_selector: &Selector, + offset: usize, element: &E, context: &mut MatchingContext, flags_setter: &mut F) @@ -362,11 +372,15 @@ pub fn matches_complex_selector(complex_selector: &Selector, where E: Element, F: FnMut(&E, ElementSelectorFlags), { - let mut iter = complex_selector.iter(); + let mut iter = if offset == 0 { + complex_selector.iter() + } else { + complex_selector.iter_from(offset) + }; if cfg!(debug_assertions) { if context.matching_mode == MatchingMode::ForStatelessPseudoElement { - assert!(complex_selector.iter().any(|c| { + assert!(iter.clone().any(|c| { matches!(*c, Component::PseudoElement(..)) })); } diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index c442d24ab9d..4c40ab54bb4 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -177,9 +177,13 @@ pub struct AncestorHashes(pub [u32; NUM_ANCESTOR_HASHES]); impl AncestorHashes { pub fn new(s: &Selector) -> Self { + Self::from_iter(s.iter()) + } + + pub fn from_iter(iter: SelectorIter) -> Self { let mut hashes = [0; NUM_ANCESTOR_HASHES]; // Compute ancestor hashes for the bloom filter. - let mut hash_iter = s.iter_ancestors() + let mut hash_iter = AncestorIter::new(iter) .map(|x| x.ancestor_hash()) .filter(|x| x.is_some()) .map(|x| x.unwrap()); @@ -356,6 +360,16 @@ impl Selector { } } + pub fn iter_from(&self, offset: usize) -> SelectorIter { + // Note: selectors are stored left-to-right but logical order is right-to-left. + let slice = self.0.as_ref(); + let iter = slice[..(slice.len() - offset)].iter().rev(); + SelectorIter { + iter: iter, + next_combinator: None, + } + } + /// Returns an iterator over the entire sequence of simple selectors and combinators, /// from right to left. pub fn iter_raw(&self) -> Rev>> { @@ -368,28 +382,6 @@ impl Selector { self.0.iter() } - /// Returns an iterator over ancestor simple selectors. All combinators and - /// non-ancestor simple selectors will be skipped. - pub fn iter_ancestors(&self) -> AncestorIter { - AncestorIter::new(self.iter()) - } - - /// Returns a Selector identical to |self| but with the rightmost |index| entries - /// removed. - pub fn slice_from(&self, index: usize) -> Self { - // Note that we convert the slice_from to slice_to because selectors are - // stored left-to-right but logical order is right-to-left. - Selector(self.0.clone().slice_to(self.0.len() - index), self.1) - } - - /// Returns a Selector identical to |self| but with the leftmost |len() - index| - /// entries removed. - pub fn slice_to(&self, index: usize) -> Self { - // Note that we convert the slice_to to slice_from because selectors are - // stored left-to-right but logical order is right-to-left. - Selector(self.0.clone().slice_from(self.0.len() - index), self.1) - } - /// Creates a Selector from a vec of Components. Used in tests. pub fn from_vec(vec: Vec>, specificity_and_flags: u32) -> Self { Selector(ArcSlice::new(vec.into_boxed_slice()), specificity_and_flags) diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index d808f148d85..d9baeb79312 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1405,7 +1405,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { NonTSPseudoClass::MozPlaceholder => false, NonTSPseudoClass::MozAny(ref sels) => { sels.iter().any(|s| { - matches_complex_selector(s, self, context, flags_setter) + matches_complex_selector(s, 0, self, context, flags_setter) }) } NonTSPseudoClass::MozSystemMetric(ref s) | diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 1a65e821e86..9c62fbe01b5 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -23,7 +23,7 @@ use selectors::attr::{AttrSelectorOperation, NamespaceConstraint}; use selectors::matching::{ElementSelectorFlags, MatchingContext, MatchingMode}; use selectors::matching::{RelevantLinkStatus, VisitedHandlingMode, matches_selector}; use selectors::parser::{AncestorHashes, Combinator, Component}; -use selectors::parser::{Selector, SelectorAndHashes, SelectorMethods}; +use selectors::parser::{Selector, SelectorAndHashes, SelectorIter, SelectorMethods}; use selectors::visitor::SelectorVisitor; use smallvec::SmallVec; use std::cell::Cell; @@ -643,7 +643,7 @@ impl<'a, E> Element for ElementWrapper<'a, E> use selectors::matching::matches_complex_selector; if let NonTSPseudoClass::MozAny(ref selectors) = *pseudo_class { return selectors.iter().any(|s| { - matches_complex_selector(s, self, context, _setter) + matches_complex_selector(s, 0, self, context, _setter) }) } } @@ -867,8 +867,13 @@ impl Sensitivities { #[derive(Clone, Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct Dependency { + /// The dependency selector. #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] selector: Selector, + /// The offset into the selector that we should match on. + selector_offset: usize, + /// The ancestor hashes associated with the above selector at the given + /// offset. #[cfg_attr(feature = "servo", ignore_heap_size_of = "No heap data")] hashes: AncestorHashes, /// The hint associated with this dependency. @@ -878,8 +883,8 @@ pub struct Dependency { } impl SelectorMapEntry for Dependency { - fn selector(&self) -> &Selector { - &self.selector + fn selector(&self) -> SelectorIter { + self.selector.iter_from(self.selector_offset) } fn hashes(&self) -> &AncestorHashes { @@ -999,19 +1004,20 @@ impl DependencySet { None => RestyleHint::for_self(), }; - let (dep_selector, hashes) = if sequence_start == 0 { - // Reuse the bloom hashes if this is the base selector. - (selector_and_hashes.selector.clone(), selector_and_hashes.hashes.clone()) + // Reuse the bloom hashes if this is the base selector. Otherwise, + // rebuild them. + let hashes = if sequence_start == 0 { + selector_and_hashes.hashes.clone() } else { - let selector = selector_and_hashes.selector.slice_from(sequence_start); - let hashes = AncestorHashes::new(&selector); - (selector, hashes) + let seq_iter = selector_and_hashes.selector.iter_from(sequence_start); + AncestorHashes::from_iter(seq_iter) }; self.dependencies.insert(Dependency { sensitivities: visitor.sensitivities, hint: hint, - selector: dep_selector, + selector: selector_and_hashes.selector.clone(), + selector_offset: sequence_start, hashes: hashes, }); } @@ -1141,6 +1147,7 @@ impl DependencySet { VisitedHandlingMode::AllLinksUnvisited); let matched_then = matches_selector(&dep.selector, + dep.selector_offset, &dep.hashes, &snapshot_el, &mut then_context, @@ -1150,6 +1157,7 @@ impl DependencySet { VisitedHandlingMode::AllLinksUnvisited); let matches_now = matches_selector(&dep.selector, + dep.selector_offset, &dep.hashes, el, &mut now_context, @@ -1177,6 +1185,7 @@ impl DependencySet { then_context.visited_handling = VisitedHandlingMode::RelevantLinkVisited; let matched_then = matches_selector(&dep.selector, + dep.selector_offset, &dep.hashes, &snapshot_el, &mut then_context, @@ -1184,6 +1193,7 @@ impl DependencySet { now_context.visited_handling = VisitedHandlingMode::RelevantLinkVisited; let matches_now = matches_selector(&dep.selector, + dep.selector_offset, &dep.hashes, el, &mut now_context, diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index 3a295bb2588..a65b664ad38 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -12,7 +12,7 @@ use pdqsort::sort_by; use rule_tree::CascadeLevel; use selector_parser::SelectorImpl; use selectors::matching::{matches_selector, MatchingContext, ElementSelectorFlags}; -use selectors::parser::{AncestorHashes, Component, Combinator, Selector, SelectorAndHashes}; +use selectors::parser::{AncestorHashes, Component, Combinator, SelectorAndHashes, SelectorIter}; use selectors::parser::LocalName as LocalNameSelector; use smallvec::VecLike; use std::borrow::Borrow; @@ -23,15 +23,15 @@ use stylist::{ApplicableDeclarationBlock, Rule}; /// A trait to abstract over a given selector map entry. pub trait SelectorMapEntry : Sized + Clone { /// Gets the selector we should use to index in the selector map. - fn selector(&self) -> &Selector; + fn selector(&self) -> SelectorIter; /// Gets the ancestor hashes associated with the selector. fn hashes(&self) -> &AncestorHashes; } impl SelectorMapEntry for SelectorAndHashes { - fn selector(&self) -> &Selector { - &self.selector + fn selector(&self) -> SelectorIter { + self.selector.iter() } fn hashes(&self) -> &AncestorHashes { @@ -232,6 +232,7 @@ impl SelectorMap { { for rule in rules { if matches_selector(&rule.selector, + 0, &rule.hashes, element, context, @@ -398,12 +399,12 @@ impl SelectorMap { /// /// Effectively, pseudo-elements are ignored, given only state pseudo-classes /// may appear before them. -fn find_from_right(selector: &Selector, +#[inline(always)] +fn find_from_right(mut iter: SelectorIter, mut f: F) -> Option where F: FnMut(&Component) -> Option, { - let mut iter = selector.iter(); for ss in &mut iter { if let Some(r) = f(ss) { return Some(r) @@ -422,9 +423,10 @@ fn find_from_right(selector: &Selector, } /// Retrieve the first ID name in the selector, or None otherwise. -pub fn get_id_name(selector: &Selector) - -> Option { - find_from_right(selector, |ss| { +#[inline(always)] +pub fn get_id_name(iter: SelectorIter) + -> Option { + find_from_right(iter, |ss| { // TODO(pradeep): Implement case-sensitivity based on the // document type and quirks mode. if let Component::ID(ref id) = *ss { @@ -435,9 +437,10 @@ pub fn get_id_name(selector: &Selector) } /// Retrieve the FIRST class name in the selector, or None otherwise. -pub fn get_class_name(selector: &Selector) - -> Option { - find_from_right(selector, |ss| { +#[inline(always)] +pub fn get_class_name(iter: SelectorIter) + -> Option { + find_from_right(iter, |ss| { // TODO(pradeep): Implement case-sensitivity based on the // document type and quirks mode. if let Component::Class(ref class) = *ss { @@ -448,9 +451,10 @@ pub fn get_class_name(selector: &Selector) } /// Retrieve the name if it is a type selector, or None otherwise. -pub fn get_local_name(selector: &Selector) - -> Option> { - find_from_right(selector, |ss| { +#[inline(always)] +pub fn get_local_name(iter: SelectorIter) + -> Option> { + find_from_right(iter, |ss| { if let Component::LocalName(ref n) = *ss { return Some(LocalNameSelector { name: n.name.clone(), diff --git a/components/style/stylist.rs b/components/style/stylist.rs index b90e6fc42ab..43b29c0275d 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1134,6 +1134,7 @@ impl Stylist { let mut results = BitVec::new(); self.selectors_for_cache_revalidation.lookup(*element, &mut |selector_and_hashes| { results.push(matches_selector(&selector_and_hashes.selector, + 0, &selector_and_hashes.hashes, element, &mut matching_context, @@ -1424,8 +1425,8 @@ pub struct Rule { } impl SelectorMapEntry for Rule { - fn selector(&self) -> &Selector { - &self.selector + fn selector(&self) -> SelectorIter { + self.selector.iter() } fn hashes(&self) -> &AncestorHashes { From 992059c8560fddd92bb49d709f606ef72f0d71f0 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 1 Jun 2017 17:03:35 -0700 Subject: [PATCH 06/14] Move around specificity computation so that we know it by the time we mint the Selector. This is important to make the selector immutable, which needs to happen when we stick it in an Arc. MozReview-Commit-ID: BaMbOEbYC3D --- components/selectors/parser.rs | 38 +++++++++++++++++----------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 4c40ab54bb4..0049c4e5286 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -841,13 +841,13 @@ impl From for u32 { } } -fn specificity(complex_selector: &Selector) -> u32 +fn specificity(iter: SelectorIter) -> u32 where Impl: SelectorImpl { - complex_selector_specificity(complex_selector).into() + complex_selector_specificity(iter).into() } -fn complex_selector_specificity(selector: &Selector) +fn complex_selector_specificity(mut iter: SelectorIter) -> Specificity where Impl: SelectorImpl { @@ -896,9 +896,7 @@ fn complex_selector_specificity(selector: &Selector) } } - let mut specificity = Default::default(); - let mut iter = selector.iter(); loop { for simple_selector in &mut iter { simple_selector_specificity(&simple_selector, &mut specificity); @@ -917,12 +915,7 @@ fn complex_selector_specificity(selector: &Selector) fn parse_selector(parser: &P, input: &mut CssParser) -> Result, ()> where P: Parser, Impl: SelectorImpl { - let (mut selector, has_pseudo_element) = parse_complex_selector(parser, input)?; - let mut specificity = specificity(&selector); - if has_pseudo_element { - specificity |= HAS_PSEUDO_BIT; - } - selector.1 = specificity; + let selector = parse_complex_selector(parser, input)?; Ok(selector) } @@ -944,7 +937,7 @@ type ParseVec = SmallVec<[Component; 8]>; fn parse_complex_selector( parser: &P, input: &mut CssParser) - -> Result<(Selector, bool), ()> + -> Result, ()> where P: Parser, Impl: SelectorImpl { let mut sequence = ParseVec::new(); @@ -992,21 +985,28 @@ fn parse_complex_selector( sequence.push(Component::Combinator(combinator)); } - let complex = Selector(ArcSlice::new(sequence.into_vec().into_boxed_slice()), 0); - Ok((complex, parsed_pseudo_element)) + let mut specificity = specificity(SelectorIter { + iter: sequence.iter().rev(), + next_combinator: None, + }); + if parsed_pseudo_element { + specificity |= HAS_PSEUDO_BIT; + } + + let complex = Selector(ArcSlice::new(sequence.into_vec().into_boxed_slice()), specificity); + Ok(complex) } impl Selector { - /// Parse a complex selector, without any pseudo-element. + /// Parse a selector, without any pseudo-element. pub fn parse

(parser: &P, input: &mut CssParser) -> Result where P: Parser { - let (complex, has_pseudo_element) = - parse_complex_selector(parser, input)?; - if has_pseudo_element { + let selector = parse_complex_selector(parser, input)?; + if selector.has_pseudo_element() { return Err(()) } - Ok(complex) + Ok(selector) } } From fa9d2cb03618c6ea310eebab27856112cdef2dad Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 2 Jun 2017 17:40:00 -0700 Subject: [PATCH 07/14] Move stylearc into a separate crate. MozReview-Commit-ID: C3btN8Jw9sJ --- Cargo.lock | 9 +++++++++ components/servo_arc/Cargo.toml | 17 +++++++++++++++++ .../{style/stylearc.rs => servo_arc/lib.rs} | 4 +++- components/style/Cargo.toml | 1 + components/style/lib.rs | 7 +++++-- 5 files changed, 35 insertions(+), 3 deletions(-) create mode 100644 components/servo_arc/Cargo.toml rename components/{style/stylearc.rs => servo_arc/lib.rs} (99%) diff --git a/Cargo.lock b/Cargo.lock index 4e5e1c777a2..d340b4860e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2688,6 +2688,14 @@ dependencies = [ "url 1.4.1 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "servo_arc" +version = "0.0.1" +dependencies = [ + "heapsize 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", + "serde 0.9.15 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "servo_atoms" version = "0.0.1" @@ -2913,6 +2921,7 @@ dependencies = [ "selectors 0.19.0", "serde 0.9.15 (registry+https://github.com/rust-lang/crates.io-index)", "serde_derive 0.9.15 (registry+https://github.com/rust-lang/crates.io-index)", + "servo_arc 0.0.1", "servo_atoms 0.0.1", "servo_config 0.0.1", "servo_url 0.0.1", diff --git a/components/servo_arc/Cargo.toml b/components/servo_arc/Cargo.toml new file mode 100644 index 00000000000..578b845da53 --- /dev/null +++ b/components/servo_arc/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "servo_arc" +version = "0.0.1" +authors = ["The Servo Project Developers"] +license = "MPL-2.0" +publish = false + +[lib] +name = "servo_arc" +path = "lib.rs" + +[features] +servo = ["serde", "heapsize"] + +[dependencies] +heapsize = {version = "0.4.0", optional = true} +serde = {version = "0.9", optional = true} diff --git a/components/style/stylearc.rs b/components/servo_arc/lib.rs similarity index 99% rename from components/style/stylearc.rs rename to components/servo_arc/lib.rs index 9b355e7d211..8f7573cbf73 100644 --- a/components/style/stylearc.rs +++ b/components/servo_arc/lib.rs @@ -8,7 +8,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -//! Fork of Arc for the style system. This has the following advantages over std::Arc: +//! Fork of Arc for Servo. This has the following advantages over std::Arc: //! * We don't waste storage on the weak reference count. //! * We don't do extra RMU operations to handle the possibility of weak references. //! * We can experiment with arena allocation (todo). @@ -20,6 +20,8 @@ // duplicate those here. #![allow(missing_docs)] +#[cfg(feature = "servo")] extern crate serde; + #[cfg(feature = "servo")] use heapsize::HeapSizeOf; #[cfg(feature = "servo")] diff --git a/components/style/Cargo.toml b/components/style/Cargo.toml index 4b33f660a68..a8751e1646b 100644 --- a/components/style/Cargo.toml +++ b/components/style/Cargo.toml @@ -61,6 +61,7 @@ rayon = "0.7.1" selectors = { path = "../selectors" } serde = {version = "0.9", optional = true} serde_derive = {version = "0.9", optional = true} +servo_arc = { path = "../servo_arc" } servo_atoms = {path = "../atoms", optional = true} servo_config = {path = "../config", optional = true} smallvec = "0.4" diff --git a/components/style/lib.rs b/components/style/lib.rs index 4856610b9ed..0cde26b37f0 100644 --- a/components/style/lib.rs +++ b/components/style/lib.rs @@ -72,8 +72,8 @@ extern crate pdqsort; #[cfg(feature = "gecko")] extern crate precomputed_hash; extern crate rayon; extern crate selectors; -#[cfg(feature = "servo")] extern crate serde; #[cfg(feature = "servo")] #[macro_use] extern crate serde_derive; +pub extern crate servo_arc; #[cfg(feature = "servo")] #[macro_use] extern crate servo_atoms; #[cfg(feature = "servo")] extern crate servo_config; #[cfg(feature = "servo")] extern crate servo_url; @@ -129,7 +129,6 @@ pub mod sequential; pub mod sink; pub mod str; pub mod style_adjuster; -pub mod stylearc; pub mod stylesheet_set; pub mod stylesheets; pub mod thread_state; @@ -139,6 +138,10 @@ pub mod traversal; #[allow(non_camel_case_types)] pub mod values; +// Compat shim for the old name when it lived in the style crate. +// FIXME(bholley) Remove this. +pub use servo_arc as stylearc; + use std::fmt; use style_traits::ToCss; From 21448cf2668e1a0ed2ee8b5cfeace7afd682e088 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 2 Jun 2017 11:16:55 -0700 Subject: [PATCH 08/14] Add support for dynamically-sized types in servo_arc. MozReview-Commit-ID: H15cG5RBtZH --- components/servo_arc/lib.rs | 97 +++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs index 8f7573cbf73..41ebc7f1bf1 100644 --- a/components/servo_arc/lib.rs +++ b/components/servo_arc/lib.rs @@ -13,6 +13,7 @@ //! * We don't do extra RMU operations to handle the possibility of weak references. //! * We can experiment with arena allocation (todo). //! * We can add methods to support our custom use cases [1]. +//! * We have support for dynamically-sized types (see from_header_and_iter). //! //! [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1360883 @@ -32,8 +33,11 @@ use std::cmp::Ordering; use std::convert::From; use std::fmt; use std::hash::{Hash, Hasher}; +use std::iter::{ExactSizeIterator, Iterator}; use std::mem; use std::ops::{Deref, DerefMut}; +use std::ptr; +use std::slice; use std::sync::atomic; use std::sync::atomic::Ordering::{Acquire, Relaxed, Release}; @@ -414,3 +418,96 @@ impl Serialize for Arc (**self).serialize(serializer) } } + +/// Structure to allow Arc-managing some fixed-sized data and a variably-sized +/// slice in a single allocation. +#[derive(Debug, Eq, PartialEq, PartialOrd)] +pub struct HeaderSlice { + /// The fixed-sized data. + pub header: H, + + /// The dynamically-sized data. + pub slice: T, +} + +#[inline(always)] +fn divide_rounding_up(dividend: usize, divisor: usize) -> usize { + (dividend + divisor - 1) / divisor +} + +impl Arc> { + /// Creates an Arc for a HeaderSlice using the given header struct and + /// iterator to generate the slice. The resulting Arc will be fat. + #[inline] + pub fn from_header_and_iter(header: H, mut items: I) -> Self + where I: Iterator + ExactSizeIterator + { + use ::std::mem::size_of; + assert!(size_of::() != 0, "Need to think about ZST"); + + // Compute the required size for the allocation. + let num_items = items.len(); + let size = { + // First, determine the alignment of a hypothetical pointer to a + // HeaderSlice. + let fake_slice_ptr_align: usize = mem::align_of::>>(); + + // Next, synthesize a totally garbage (but properly aligned) pointer + // to a sequence of T. + let fake_slice_ptr = fake_slice_ptr_align as *const T; + + // Convert that sequence to a fat pointer. The address component of + // the fat pointer will be garbage, but the length will be correct. + let fake_slice = unsafe { slice::from_raw_parts(fake_slice_ptr, num_items) }; + + // Pretend the garbage address points to our allocation target (with + // a trailing sequence of T), rather than just a sequence of T. + let fake_ptr = fake_slice as *const [T] as *const ArcInner>; + let fake_ref: &ArcInner> = unsafe { &*fake_ptr }; + + // Use size_of_val, which will combine static information about the + // type with the length from the fat pointer. The garbage address + // will not be used. + mem::size_of_val(fake_ref) + }; + + let ptr: *mut ArcInner>; + unsafe { + // Allocate the buffer. We use Vec because the underlying allocation + // machinery isn't available in stable Rust. + // + // To avoid alignment issues, we allocate words rather than bytes, + // rounding up to the nearest word size. + let words_to_allocate = divide_rounding_up(size, size_of::()); + let mut vec = Vec::::with_capacity(words_to_allocate); + vec.set_len(words_to_allocate); + let buffer = Box::into_raw(vec.into_boxed_slice()) as *mut usize as *mut u8; + + // Synthesize the fat pointer. We do this by claiming we have a direct + // pointer to a [T], and then changing the type of the borrow. The key + // point here is that the length portion of the fat pointer applies + // only to the number of elements in the dynamically-sized portion of + // the type, so the value will be the same whether it points to a [T] + // or something else with a [T] as its last member. + let fake_slice: &mut [T] = slice::from_raw_parts_mut(buffer as *mut T, num_items); + ptr = fake_slice as *mut [T] as *mut ArcInner>; + + // Write the data. + ptr::write(&mut ((*ptr).count), atomic::AtomicUsize::new(1)); + ptr::write(&mut ((*ptr).data.header), header); + let mut current: *mut T = &mut (*ptr).data.slice[0]; + for _ in 0..num_items { + ptr::write(current, items.next().expect("ExactSizeIterator over-reported length")); + current = current.offset(1); + } + assert!(items.next().is_none(), "ExactSizeIterator under-reported length"); + + // We should have consumed the buffer exactly. + debug_assert!(current as *mut u8 == buffer.offset(size as isize)); + } + + // Return the fat Arc. + assert_eq!(size_of::(), size_of::() * 2, "The Arc will be fat"); + Arc { ptr: ptr } + } +} From 852efb825f3811dcfc8b8b54be10e00398ad17d3 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 2 Jun 2017 17:59:35 -0700 Subject: [PATCH 09/14] Use dynamically-sized Arcs for Selector. MozReview-Commit-ID: hq0jYrx8Sg --- Cargo.lock | 1 + components/selectors/Cargo.toml | 1 + components/selectors/arcslice.rs | 326 ------------------------------- components/selectors/lib.rs | 2 +- components/selectors/parser.rs | 37 ++-- tests/unit/stylo/size_of.rs | 2 +- 6 files changed, 29 insertions(+), 340 deletions(-) delete mode 100644 components/selectors/arcslice.rs diff --git a/Cargo.lock b/Cargo.lock index d340b4860e2..8ed951c806f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2523,6 +2523,7 @@ dependencies = [ "phf 0.7.21 (registry+https://github.com/rust-lang/crates.io-index)", "phf_codegen 0.7.21 (registry+https://github.com/rust-lang/crates.io-index)", "precomputed-hash 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "servo_arc 0.0.1", "size_of_test 0.0.1", "smallvec 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", ] diff --git a/components/selectors/Cargo.toml b/components/selectors/Cargo.toml index effb612bb43..15aba3d1811 100644 --- a/components/selectors/Cargo.toml +++ b/components/selectors/Cargo.toml @@ -28,6 +28,7 @@ cssparser = "0.13.7" fnv = "1.0" phf = "0.7.18" precomputed-hash = "0.1" +servo_arc = { path = "../servo_arc" } smallvec = "0.4" [dev-dependencies] diff --git a/components/selectors/arcslice.rs b/components/selectors/arcslice.rs deleted file mode 100644 index e5722ba6505..00000000000 --- a/components/selectors/arcslice.rs +++ /dev/null @@ -1,326 +0,0 @@ -/* Licensed under the Apache License, Version 2.0 or the MIT license - * , at your - * option. This file may not be copied, modified, or distributed - * except according to those terms. - * - * See the COPYRIGHT file at the top-level directory of this distribution */ -//! A thread-safe reference-counted slice type. -//! -//! Forked from https://github.com/huonw/shared_slice , which doesn't work on -//! rust stable. - -use std::{cmp, fmt, ops}; -use std::hash::{Hash, Hasher}; -use std::sync::{Arc, Weak}; - - -/// A reference-counted slice type. -pub struct ArcSlice { - data: *const [T], - counts: Arc>, -} - -unsafe impl Send for ArcSlice {} -unsafe impl Sync for ArcSlice {} - -/// A non-owning reference-counted slice type. -/// -/// This is to `ArcSlice` as `std::sync::Weak` is to `std::sync::Arc`, and -/// allows one to have cyclic references without stopping memory from -/// being deallocated. -pub struct WeakSlice { - data: *const [T], - counts: Weak>, -} -unsafe impl Send for WeakSlice {} -unsafe impl Sync for WeakSlice {} - -impl ArcSlice { - /// Construct a new `ArcSlice` containing the elements of `slice`. - /// - /// This reuses the allocation of `slice`. - pub fn new(slice: Box<[T]>) -> ArcSlice { - ArcSlice { - data: &*slice, - counts: Arc::new(slice), - } - } - - /// Downgrade self into a weak slice. - pub fn downgrade(&self) -> WeakSlice { - WeakSlice { - data: self.data, - counts: Arc::downgrade(&self.counts) - } - } - - /// Construct a new `ArcSlice` that only points to elements at - /// indices `lo` (inclusive) through `hi` (exclusive). - /// - /// This consumes `self` to avoid unnecessary reference-count - /// modifications. Use `.clone()` if it is necessary to refer to - /// `self` after calling this. - /// - /// # Panics - /// - /// Panics if `lo > hi` or if either are strictly greater than - /// `self.len()`. - pub fn slice(mut self, lo: usize, hi: usize) -> ArcSlice { - self.data = &self[lo..hi]; - self - } - /// Construct a new `ArcSlice` that only points to elements at - /// indices up to `hi` (exclusive). - /// - /// This consumes `self` to avoid unnecessary reference-count - /// modifications. Use `.clone()` if it is necessary to refer to - /// `self` after calling this. - /// - /// # Panics - /// - /// Panics if `hi > self.len()`. - pub fn slice_to(self, hi: usize) -> ArcSlice { - self.slice(0, hi) - } - /// Construct a new `ArcSlice` that only points to elements at - /// indices starting at `lo` (inclusive). - /// - /// This consumes `self` to avoid unnecessary reference-count - /// modifications. Use `.clone()` if it is necessary to refer to - /// `self` after calling this. - /// - /// # Panics - /// - /// Panics if `lo > self.len()`. - pub fn slice_from(self, lo: usize) -> ArcSlice { - let hi = self.len(); - self.slice(lo, hi) - } -} - -impl Clone for ArcSlice { - fn clone(&self) -> ArcSlice { - ArcSlice { - data: self.data, - counts: self.counts.clone() - } - } -} - -impl ops::Deref for ArcSlice { - type Target = [T]; - fn deref<'a>(&'a self) -> &'a [T] { - unsafe { &*self.data } - } -} - -impl AsRef<[T]> for ArcSlice { - fn as_ref(&self) -> &[T] { &**self } -} - -impl PartialEq for ArcSlice { - fn eq(&self, other: &ArcSlice) -> bool { **self == **other } - fn ne(&self, other: &ArcSlice) -> bool { **self != **other } -} -impl Eq for ArcSlice {} - -impl PartialOrd for ArcSlice { - fn partial_cmp(&self, other: &ArcSlice) -> Option { - (**self).partial_cmp(&**other) - } - fn lt(&self, other: &ArcSlice) -> bool { **self < **other } - fn le(&self, other: &ArcSlice) -> bool { **self <= **other } - fn gt(&self, other: &ArcSlice) -> bool { **self > **other } - fn ge(&self, other: &ArcSlice) -> bool { **self >= **other } -} -impl Ord for ArcSlice { - fn cmp(&self, other: &ArcSlice) -> cmp::Ordering { (**self).cmp(&**other) } -} - -impl Hash for ArcSlice { - fn hash(&self, state: &mut H) { - Hash::hash(&**self, state) - } -} - -impl fmt::Debug for ArcSlice { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - fmt::Debug::fmt(&**self, f) - } -} - -impl WeakSlice { - /// Attempt to upgrade `self` to a strongly-counted `ArcSlice`. - /// - /// Returns `None` if this is not possible (the data has already - /// been freed). - pub fn upgrade(&self) -> Option> { - self.counts.upgrade().map(|counts| { - ArcSlice { - data: self.data, - counts: counts - } - }) - } -} - -#[cfg(test)] -mod tests { - use std::cell::Cell; - use std::cmp::Ordering; - use std::sync::{Arc, Mutex}; - use super::{ArcSlice, WeakSlice}; - #[test] - fn clone() { - let x = ArcSlice::new(Box::new([Cell::new(false)])); - let y = x.clone(); - - assert_eq!(x[0].get(), false); - assert_eq!(y[0].get(), false); - - x[0].set(true); - assert_eq!(x[0].get(), true); - assert_eq!(y[0].get(), true); - } - - #[test] - fn test_upgrade_downgrade() { - let x = ArcSlice::new(Box::new([1])); - let y: WeakSlice<_> = x.downgrade(); - - assert_eq!(y.upgrade(), Some(x.clone())); - - drop(x); - - assert!(y.upgrade().is_none()) - } - - #[test] - fn test_total_cmp() { - let x = ArcSlice::new(Box::new([1, 2, 3])); - let y = ArcSlice::new(Box::new([1, 2, 3])); - let z = ArcSlice::new(Box::new([1, 2, 4])); - - assert_eq!(x, x); - assert_eq!(x, y); - assert!(x != z); - assert!(y != z); - - assert!(x < z); - assert!(x <= z); - assert!(!(x > z)); - assert!(!(x >= z)); - - assert!(!(z < x)); - assert!(!(z <= x)); - assert!(z > x); - assert!(z >= x); - - assert_eq!(x.partial_cmp(&x), Some(Ordering::Equal)); - assert_eq!(x.partial_cmp(&y), Some(Ordering::Equal)); - assert_eq!(x.partial_cmp(&z), Some(Ordering::Less)); - assert_eq!(z.partial_cmp(&y), Some(Ordering::Greater)); - - assert_eq!(x.cmp(&x), Ordering::Equal); - assert_eq!(x.cmp(&y), Ordering::Equal); - assert_eq!(x.cmp(&z), Ordering::Less); - assert_eq!(z.cmp(&y), Ordering::Greater); - } - - #[test] - fn test_partial_cmp() { - use std::f64; - let x = ArcSlice::new(Box::new([1.0, f64::NAN])); - let y = ArcSlice::new(Box::new([1.0, f64::NAN])); - let z = ArcSlice::new(Box::new([2.0, f64::NAN])); - let w = ArcSlice::new(Box::new([f64::NAN, 1.0])); - assert!(!(x == y)); - assert!(x != y); - - assert!(!(x < y)); - assert!(!(x <= y)); - assert!(!(x > y)); - assert!(!(x >= y)); - - assert!(x < z); - assert!(x <= z); - assert!(!(x > z)); - assert!(!(x >= z)); - - assert!(!(z < w)); - assert!(!(z <= w)); - assert!(!(z > w)); - assert!(!(z >= w)); - - assert_eq!(x.partial_cmp(&x), None); - assert_eq!(x.partial_cmp(&y), None); - assert_eq!(x.partial_cmp(&z), Some(Ordering::Less)); - assert_eq!(z.partial_cmp(&x), Some(Ordering::Greater)); - - assert_eq!(x.partial_cmp(&w), None); - assert_eq!(y.partial_cmp(&w), None); - assert_eq!(z.partial_cmp(&w), None); - assert_eq!(w.partial_cmp(&w), None); - } - - #[test] - fn test_show() { - let x = ArcSlice::new(Box::new([1, 2])); - assert_eq!(format!("{:?}", x), "[1, 2]"); - - let y: ArcSlice = ArcSlice::new(Box::new([])); - assert_eq!(format!("{:?}", y), "[]"); - } - - #[test] - fn test_slice() { - let x = ArcSlice::new(Box::new([1, 2, 3])); - let real = [1, 2, 3]; - for i in 0..3 + 1 { - for j in i..3 + 1 { - let slice: ArcSlice<_> = x.clone().slice(i, j); - assert_eq!(&*slice, &real[i..j]); - } - assert_eq!(&*x.clone().slice_to(i), &real[..i]); - assert_eq!(&*x.clone().slice_from(i), &real[i..]); - } - } - - - #[test] - fn test_send_sync() { - fn assert_send() {} - fn assert_sync() {} - - assert_send::>(); - assert_sync::>(); - assert_send::>(); - assert_sync::>(); - } - - #[test] - fn test_drop() { - let drop_flag = Arc::new(Mutex::new(0)); - struct Foo(Arc>); - - impl Drop for Foo { - fn drop(&mut self) { - let mut n = self.0.lock().unwrap(); - *n += 1; - } - } - - let whole = ArcSlice::new(Box::new([Foo(drop_flag.clone()), Foo(drop_flag.clone())])); - - drop(whole); - assert_eq!(*drop_flag.lock().unwrap(), 2); - - *drop_flag.lock().unwrap() = 0; - - let whole = ArcSlice::new(Box::new([Foo(drop_flag.clone()), Foo(drop_flag.clone())])); - let part = whole.slice(1, 2); - drop(part); - assert_eq!(*drop_flag.lock().unwrap(), 2); - } -} diff --git a/components/selectors/lib.rs b/components/selectors/lib.rs index 3e413c0c08b..a6f4acd3d84 100644 --- a/components/selectors/lib.rs +++ b/components/selectors/lib.rs @@ -9,9 +9,9 @@ extern crate fnv; extern crate phf; extern crate precomputed_hash; #[cfg(test)] #[macro_use] extern crate size_of_test; +extern crate servo_arc; extern crate smallvec; -pub mod arcslice; pub mod attr; pub mod bloom; pub mod matching; diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 0049c4e5286..3aee478311c 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -2,11 +2,11 @@ * 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/. */ -use arcslice::ArcSlice; use attr::{AttrSelectorWithNamespace, ParsedAttrSelectorOperation, AttrSelectorOperator}; use attr::{ParsedCaseSensitivity, SELECTOR_WHITESPACE, NamespaceConstraint}; use cssparser::{Token, Parser as CssParser, parse_nth, ToCss, serialize_identifier, CssStringWriter}; use precomputed_hash::PrecomputedHash; +use servo_arc::{Arc, HeaderSlice}; use smallvec::SmallVec; use std::ascii::AsciiExt; use std::borrow::{Borrow, Cow}; @@ -302,6 +302,19 @@ pub fn namespace_empty_string() -> Impl::NamespaceUrl { Impl::NamespaceUrl::default() } +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +struct SpecificityAndFlags(u32); + +impl SpecificityAndFlags { + fn specificity(&self) -> u32 { + self.0 & !HAS_PSEUDO_BIT + } + + fn has_pseudo_element(&self) -> bool { + (self.0 & HAS_PSEUDO_BIT) != 0 + } +} + /// A Selector stores a sequence of simple selectors and combinators. The /// iterator classes allow callers to iterate at either the raw sequence level or /// at the level of sequences of simple selectors separated by combinators. Most @@ -311,15 +324,15 @@ pub fn namespace_empty_string() -> Impl::NamespaceUrl { /// canonical iteration order is right-to-left (selector matching order). The /// iterators abstract over these details. #[derive(Clone, Eq, PartialEq)] -pub struct Selector(ArcSlice>, u32); +pub struct Selector(Arc]>>); impl Selector { pub fn specificity(&self) -> u32 { - self.1 & !HAS_PSEUDO_BIT + self.0.header.specificity() } pub fn has_pseudo_element(&self) -> bool { - (self.1 & HAS_PSEUDO_BIT) != 0 + self.0.header.has_pseudo_element() } pub fn pseudo_element(&self) -> Option<&Impl::PseudoElement> { @@ -362,8 +375,7 @@ impl Selector { pub fn iter_from(&self, offset: usize) -> SelectorIter { // Note: selectors are stored left-to-right but logical order is right-to-left. - let slice = self.0.as_ref(); - let iter = slice[..(slice.len() - offset)].iter().rev(); + let iter = self.0.slice[..(self.0.slice.len() - offset)].iter().rev(); SelectorIter { iter: iter, next_combinator: None, @@ -379,12 +391,13 @@ impl Selector { /// Returns an iterator over the entire sequence of simple selectors and combinators, /// from left to right. pub fn iter_raw_rev(&self) -> slice::Iter> { - self.0.iter() + self.0.slice.iter() } /// Creates a Selector from a vec of Components. Used in tests. pub fn from_vec(vec: Vec>, specificity_and_flags: u32) -> Self { - Selector(ArcSlice::new(vec.into_boxed_slice()), specificity_and_flags) + let spec = SpecificityAndFlags(specificity_and_flags); + Selector(Arc::from_header_and_iter(spec, vec.into_iter())) } } @@ -985,15 +998,15 @@ fn parse_complex_selector( sequence.push(Component::Combinator(combinator)); } - let mut specificity = specificity(SelectorIter { + let mut spec = SpecificityAndFlags(specificity(SelectorIter { iter: sequence.iter().rev(), next_combinator: None, - }); + })); if parsed_pseudo_element { - specificity |= HAS_PSEUDO_BIT; + spec.0 |= HAS_PSEUDO_BIT; } - let complex = Selector(ArcSlice::new(sequence.into_vec().into_boxed_slice()), specificity); + let complex = Selector(Arc::from_header_and_iter(spec, sequence.into_iter())); Ok(complex) } diff --git a/tests/unit/stylo/size_of.rs b/tests/unit/stylo/size_of.rs index f35177bf058..0cdf3067210 100644 --- a/tests/unit/stylo/size_of.rs +++ b/tests/unit/stylo/size_of.rs @@ -22,7 +22,7 @@ fn size_of_selectors_dummy_types() { // The size of this is critical to performance on the bloom-basic microbenchmark. // When iterating over a large Rule array, we want to be able to fast-reject // selectors (with the inline hashes) with as few cache misses as possible. -size_of_test!(test_size_of_rule, style::stylist::Rule, 64); +size_of_test!(test_size_of_rule, style::stylist::Rule, 48); size_of_test!(test_size_of_property_declaration, style::properties::PropertyDeclaration, 32); From e8a56e969f38b48f535a97cfad57008f57c0f4aa Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 2 Jun 2017 20:02:10 -0700 Subject: [PATCH 10/14] Make ParseVec bigger. MozReview-Commit-ID: DszMC031Xlj --- components/selectors/parser.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 3aee478311c..22293688b3a 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -932,15 +932,11 @@ fn parse_selector(parser: &P, input: &mut CssParser) -> Result 8 entries, we save two reallocations. -type ParseVec = SmallVec<[Component; 8]>; +/// We make this large because the result of parsing a selector is fed into a new +/// Arc-ed allocation, so any spilled vec would be a wasted allocation. Also, +/// Components are large enough that we don't have much cache locality benefit +/// from reserving stack space for fewer of them. +type ParseVec = SmallVec<[Component; 32]>; /// Parses a complex selector, including any pseudo-element. /// From ce8d2a920857fd85cce3480ffb1b44da1bda8010 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Sun, 4 Jun 2017 19:14:14 -0700 Subject: [PATCH 11/14] Implement ThinArc. MozReview-Commit-ID: LB3bs9ed2WC --- Cargo.lock | 1 + components/servo_arc/Cargo.toml | 1 + components/servo_arc/lib.rs | 164 ++++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 8ed951c806f..b5981a9cf6b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2694,6 +2694,7 @@ name = "servo_arc" version = "0.0.1" dependencies = [ "heapsize 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", + "nodrop 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)", "serde 0.9.15 (registry+https://github.com/rust-lang/crates.io-index)", ] diff --git a/components/servo_arc/Cargo.toml b/components/servo_arc/Cargo.toml index 578b845da53..ffb743bd20a 100644 --- a/components/servo_arc/Cargo.toml +++ b/components/servo_arc/Cargo.toml @@ -15,3 +15,4 @@ servo = ["serde", "heapsize"] [dependencies] heapsize = {version = "0.4.0", optional = true} serde = {version = "0.9", optional = true} +nodrop = {version = "0.1.8"} diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs index 41ebc7f1bf1..5d91c9bc44f 100644 --- a/components/servo_arc/lib.rs +++ b/components/servo_arc/lib.rs @@ -14,6 +14,7 @@ //! * We can experiment with arena allocation (todo). //! * We can add methods to support our custom use cases [1]. //! * We have support for dynamically-sized types (see from_header_and_iter). +//! * We have support for thin arcs to unsized types (see ThinArc). //! //! [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1360883 @@ -22,9 +23,11 @@ #![allow(missing_docs)] #[cfg(feature = "servo")] extern crate serde; +extern crate nodrop; #[cfg(feature = "servo")] use heapsize::HeapSizeOf; +use nodrop::NoDrop; #[cfg(feature = "servo")] use serde::{Deserialize, Serialize}; use std::{isize, usize}; @@ -511,3 +514,164 @@ impl Arc> { Arc { ptr: ptr } } } + +/// Header data with an inline length. Consumers that use HeaderWithLength as the +/// Header type in HeaderSlice can take advantage of ThinArc. +#[derive(Debug, Eq, PartialEq, PartialOrd)] +pub struct HeaderWithLength { + /// The fixed-sized data. + pub header: H, + + /// The slice length. + length: usize, +} + +impl HeaderWithLength { + /// Creates a new HeaderWithLength. + pub fn new(header: H, length: usize) -> Self { + HeaderWithLength { + header: header, + length: length, + } + } +} + +pub struct ThinArc { + ptr: *mut ArcInner, [T; 1]>>, +} + +unsafe impl Send for ThinArc {} +unsafe impl Sync for ThinArc {} + +// Synthesize a fat pointer from a thin pointer. +// +// See the comment around the analogous operation in from_header_and_iter. +fn thin_to_thick(thin: *mut ArcInner, [T; 1]>>) + -> *mut ArcInner, [T]>> +{ + let len = unsafe { (*thin).data.header.length }; + let fake_slice: *mut [T] = unsafe { + slice::from_raw_parts_mut(thin as *mut T, len) + }; + + fake_slice as *mut ArcInner, [T]>> +} + +impl ThinArc { + /// Temporarily converts |self| into a bonafide Arc and exposes it to the + /// provided callback. The refcount is not modified. + #[inline(always)] + pub fn with_arc(&self, f: F) -> U + where F: FnOnce(&Arc, [T]>>) -> U + { + // Synthesize transient Arc, which never touches the refcount of the ArcInner. + let transient = NoDrop::new(Arc { + ptr: thin_to_thick(self.ptr) + }); + + // Expose the transient Arc to the callback, which may clone it if it wants. + let result = f(&transient); + + // Forget the transient Arc to leave the refcount untouched. + mem::forget(transient); + + // Forward the result. + result + } +} + +impl Deref for ThinArc { + type Target = HeaderSlice, [T]>; + fn deref(&self) -> &Self::Target { + unsafe { &(*thin_to_thick(self.ptr)).data } + } +} + +impl Clone for ThinArc { + fn clone(&self) -> Self { + ThinArc::with_arc(self, |a| Arc::into_thin(a.clone())) + } +} + +impl Drop for ThinArc { + fn drop(&mut self) { + let _ = Arc::from_thin(ThinArc { ptr: self.ptr }); + } +} + +impl Arc, [T]>> { + /// Converts an Arc into a ThinArc. This consumes the Arc, so the refcount + /// is not modified. + pub fn into_thin(a: Self) -> ThinArc { + assert!(a.header.length == a.slice.len(), + "Length needs to be correct for ThinArc to work"); + let fat_ptr: *mut ArcInner, [T]>> = a.ptr; + mem::forget(a); + let thin_ptr = fat_ptr as *mut [usize] as *mut usize; + ThinArc { + ptr: thin_ptr as *mut ArcInner, [T; 1]>> + } + } + + /// Converts a ThinArc into an Arc. This consumes the ThinArc, so the refcount + /// is not modified. + pub fn from_thin(a: ThinArc) -> Self { + let ptr = thin_to_thick(a.ptr); + mem::forget(a); + Arc { + ptr: ptr + } + } +} + +impl PartialEq for ThinArc { + fn eq(&self, other: &ThinArc) -> bool { + ThinArc::with_arc(self, |a| { + ThinArc::with_arc(other, |b| { + *a == *b + }) + }) + } +} + +impl Eq for ThinArc {} + +#[cfg(test)] +mod tests { + use std::clone::Clone; + use std::ops::Drop; + use std::sync::atomic; + use std::sync::atomic::Ordering::{Acquire, SeqCst}; + use super::{Arc, HeaderWithLength, ThinArc}; + + #[derive(PartialEq)] + struct Canary(*mut atomic::AtomicUsize); + + impl Drop for Canary { + fn drop(&mut self) { + unsafe { + match *self { + Canary(c) => { + (*c).fetch_add(1, SeqCst); + } + } + } + } + } + + #[test] + fn slices_and_thin() { + let mut canary = atomic::AtomicUsize::new(0); + let c = Canary(&mut canary as *mut atomic::AtomicUsize); + let v = vec![5, 6]; + let header = HeaderWithLength::new(c, v.len()); + { + let x = Arc::into_thin(Arc::from_header_and_iter(header, v.into_iter())); + let y = ThinArc::with_arc(&x, |q| q.clone()); + let _ = y.clone(); + let _ = x == x; + Arc::from_thin(x.clone()); + } + assert!(canary.load(Acquire) == 1); + } +} From 27568de567b4e16cc6f570715f6d65d812100e32 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Sun, 4 Jun 2017 19:14:24 -0700 Subject: [PATCH 12/14] Use ThinArc in Selector. MozReview-Commit-ID: Axvq0rbqA7Y --- components/selectors/parser.rs | 15 ++++++++------- tests/unit/stylo/size_of.rs | 2 +- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 22293688b3a..ece14613a75 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -6,7 +6,7 @@ use attr::{AttrSelectorWithNamespace, ParsedAttrSelectorOperation, AttrSelectorO use attr::{ParsedCaseSensitivity, SELECTOR_WHITESPACE, NamespaceConstraint}; use cssparser::{Token, Parser as CssParser, parse_nth, ToCss, serialize_identifier, CssStringWriter}; use precomputed_hash::PrecomputedHash; -use servo_arc::{Arc, HeaderSlice}; +use servo_arc::{Arc, HeaderWithLength, ThinArc}; use smallvec::SmallVec; use std::ascii::AsciiExt; use std::borrow::{Borrow, Cow}; @@ -324,15 +324,15 @@ impl SpecificityAndFlags { /// canonical iteration order is right-to-left (selector matching order). The /// iterators abstract over these details. #[derive(Clone, Eq, PartialEq)] -pub struct Selector(Arc]>>); +pub struct Selector(ThinArc>); impl Selector { pub fn specificity(&self) -> u32 { - self.0.header.specificity() + self.0.header.header.specificity() } pub fn has_pseudo_element(&self) -> bool { - self.0.header.has_pseudo_element() + self.0.header.header.has_pseudo_element() } pub fn pseudo_element(&self) -> Option<&Impl::PseudoElement> { @@ -396,8 +396,8 @@ impl Selector { /// Creates a Selector from a vec of Components. Used in tests. pub fn from_vec(vec: Vec>, specificity_and_flags: u32) -> Self { - let spec = SpecificityAndFlags(specificity_and_flags); - Selector(Arc::from_header_and_iter(spec, vec.into_iter())) + let header = HeaderWithLength::new(SpecificityAndFlags(specificity_and_flags), vec.len()); + Selector(Arc::into_thin(Arc::from_header_and_iter(header, vec.into_iter()))) } } @@ -1002,7 +1002,8 @@ fn parse_complex_selector( spec.0 |= HAS_PSEUDO_BIT; } - let complex = Selector(Arc::from_header_and_iter(spec, sequence.into_iter())); + let header = HeaderWithLength::new(spec, sequence.len()); + let complex = Selector(Arc::into_thin(Arc::from_header_and_iter(header, sequence.into_iter()))); Ok(complex) } diff --git a/tests/unit/stylo/size_of.rs b/tests/unit/stylo/size_of.rs index 0cdf3067210..3344a65c075 100644 --- a/tests/unit/stylo/size_of.rs +++ b/tests/unit/stylo/size_of.rs @@ -22,7 +22,7 @@ fn size_of_selectors_dummy_types() { // The size of this is critical to performance on the bloom-basic microbenchmark. // When iterating over a large Rule array, we want to be able to fast-reject // selectors (with the inline hashes) with as few cache misses as possible. -size_of_test!(test_size_of_rule, style::stylist::Rule, 48); +size_of_test!(test_size_of_rule, style::stylist::Rule, 40); size_of_test!(test_size_of_property_declaration, style::properties::PropertyDeclaration, 32); From 442211723cecfa895d061b3507cf3f447bdb895e Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 5 Jun 2017 09:58:37 -0700 Subject: [PATCH 13/14] Eliminate redundant parse_selector. MozReview-Commit-ID: 6eBVgT9ri5Q --- components/selectors/parser.rs | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index ece14613a75..423d092978a 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -921,29 +921,17 @@ fn complex_selector_specificity(mut iter: SelectorIter) specificity } -/// Build up a Selector. -/// selector : simple_selector_sequence [ combinator simple_selector_sequence ]* ; -/// -/// `Err` means invalid selector. -fn parse_selector(parser: &P, input: &mut CssParser) -> Result, ()> - where P: Parser, Impl: SelectorImpl -{ - let selector = parse_complex_selector(parser, input)?; - Ok(selector) -} - /// We make this large because the result of parsing a selector is fed into a new /// Arc-ed allocation, so any spilled vec would be a wasted allocation. Also, /// Components are large enough that we don't have much cache locality benefit /// from reserving stack space for fewer of them. type ParseVec = SmallVec<[Component; 32]>; -/// Parses a complex selector, including any pseudo-element. +/// Build up a Selector. +/// selector : simple_selector_sequence [ combinator simple_selector_sequence ]* ; /// -/// For now, it always forces the pseudo-element to be at the end of the -/// selector, and the boolean represents whether the last thing parsed was a -/// pseudo-element. -fn parse_complex_selector( +/// `Err` means invalid selector. +fn parse_selector( parser: &P, input: &mut CssParser) -> Result, ()> @@ -1012,7 +1000,7 @@ impl Selector { pub fn parse

(parser: &P, input: &mut CssParser) -> Result where P: Parser { - let selector = parse_complex_selector(parser, input)?; + let selector = parse_selector(parser, input)?; if selector.has_pseudo_element() { return Err(()) } From f105d3438dc3a97bf7f34f28adcd216c67adb262 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 5 Jun 2017 18:07:37 -0700 Subject: [PATCH 14/14] Fix up unit tests. MozReview-Commit-ID: 9eyOhoQ02qX --- components/selectors/parser.rs | 441 ++++++++++++-------------- components/selectors/size_of_tests.rs | 4 +- tests/unit/style/stylesheets.rs | 85 +++-- tests/unit/style/stylist.rs | 16 +- 4 files changed, 246 insertions(+), 300 deletions(-) diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 423d092978a..a7f3d259fbd 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -162,6 +162,11 @@ impl SelectorList { input.parse_comma_separated(|input| parse_selector(parser, input).map(SelectorAndHashes::new)) .map(SelectorList) } + + /// Creates a SelectorList from a Vec of selectors. Used in tests. + pub fn from_vec(v: Vec>) -> Self { + SelectorList(v.into_iter().map(SelectorAndHashes::new).collect()) + } } /// Copied from Gecko, who copied it from WebKit. Note that increasing the @@ -1709,7 +1714,7 @@ pub mod tests { let result = SelectorList::parse(parser, &mut CssParser::new(input)); if let Ok(ref selectors) = result { assert_eq!(selectors.0.len(), 1); - assert_eq!(selectors.0[0].to_css_string(), input); + assert_eq!(selectors.0[0].selector.to_css_string(), input); } result } @@ -1732,185 +1737,164 @@ pub mod tests { assert_eq!(parse(""), Err(())) ; assert_eq!(parse(":lang(4)"), Err(())) ; assert_eq!(parse(":lang(en US)"), Err(())) ; - assert_eq!(parse("EeÉ"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + assert_eq!(parse("EeÉ"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::LocalName(LocalName { name: DummyAtom::from("EeÉ"), - lower_name: DummyAtom::from("eeÉ") })), - ), - specificity_and_flags: specificity(0, 0, 1), - })))); - assert_eq!(parse("|e"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + lower_name: DummyAtom::from("eeÉ") }) + ), specificity(0, 0, 1)) + )))); + assert_eq!(parse("|e"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::ExplicitNoNamespace, Component::LocalName(LocalName { name: DummyAtom::from("e"), lower_name: DummyAtom::from("e") - }), - )), - specificity_and_flags: specificity(0, 0, 1), - })))); + })), specificity(0, 0, 1)) + )))); // https://github.com/servo/servo/issues/16020 - assert_eq!(parse("*|e"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + assert_eq!(parse("*|e"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::ExplicitAnyNamespace, Component::LocalName(LocalName { name: DummyAtom::from("e"), lower_name: DummyAtom::from("e") - }), - )), - specificity_and_flags: specificity(0, 0, 1), - })))); - assert_eq!(parse("*"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + }) + ), specificity(0, 0, 1)) + )))); + assert_eq!(parse("*"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::ExplicitUniversalType, - )), - specificity_and_flags: specificity(0, 0, 0), - })))); - assert_eq!(parse("|*"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + ), specificity(0, 0, 0)) + )))); + assert_eq!(parse("|*"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::ExplicitNoNamespace, Component::ExplicitUniversalType, - )), - specificity_and_flags: specificity(0, 0, 0), - })))); - assert_eq!(parse("*|*"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + ), specificity(0, 0, 0)) + )))); + assert_eq!(parse("*|*"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::ExplicitAnyNamespace, Component::ExplicitUniversalType, - )), - specificity_and_flags: specificity(0, 0, 0), - })))); - assert_eq!(parse(".foo:lang(en-US)"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec![ + ), specificity(0, 0, 0)) + )))); + assert_eq!(parse(".foo:lang(en-US)"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::Class(DummyAtom::from("foo")), Component::NonTSPseudoClass(PseudoClass::Lang("en-US".to_owned())) - ]), - specificity_and_flags: specificity(0, 2, 0), - })))); - assert_eq!(parse("#bar"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!(Component::ID(DummyAtom::from("bar")))), - specificity_and_flags: specificity(1, 0, 0), - })))); - assert_eq!(parse("e.foo#bar"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!(Component::LocalName(LocalName { - name: DummyAtom::from("e"), - lower_name: DummyAtom::from("e") }), - Component::Class(DummyAtom::from("foo")), - Component::ID(DummyAtom::from("bar")))), - specificity_and_flags: specificity(1, 1, 1), - })))); - assert_eq!(parse("e.foo #bar"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( - Component::LocalName(LocalName { - name: DummyAtom::from("e"), - lower_name: DummyAtom::from("e") - }), - Component::Class(DummyAtom::from("foo")), - Component::Combinator(Combinator::Descendant), - Component::ID(DummyAtom::from("bar")), - )), - specificity_and_flags: specificity(1, 1, 1), - })))); + ), specificity(0, 2, 0)) + )))); + assert_eq!(parse("#bar"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::ID(DummyAtom::from("bar")) + ), specificity(1, 0, 0)) + )))); + assert_eq!(parse("e.foo#bar"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::LocalName(LocalName { + name: DummyAtom::from("e"), + lower_name: DummyAtom::from("e") + }), + Component::Class(DummyAtom::from("foo")), + Component::ID(DummyAtom::from("bar")) + ), specificity(1, 1, 1)) + )))); + assert_eq!(parse("e.foo #bar"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::LocalName(LocalName { + name: DummyAtom::from("e"), + lower_name: DummyAtom::from("e") + }), + Component::Class(DummyAtom::from("foo")), + Component::Combinator(Combinator::Descendant), + Component::ID(DummyAtom::from("bar")), + ), specificity(1, 1, 1)) + )))); // Default namespace does not apply to attribute selectors // https://github.com/mozilla/servo/pull/1652 let mut parser = DummyParser::default(); - assert_eq!(parse_ns("[Foo]", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec![ + assert_eq!(parse_ns("[Foo]", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::AttributeInNoNamespaceExists { local_name: DummyAtom::from("Foo"), local_name_lower: DummyAtom::from("foo"), } - ]), - specificity_and_flags: specificity(0, 1, 0), - })))); + ), specificity(0, 1, 0)) + )))); assert_eq!(parse_ns("svg|circle", &parser), Err(())); parser.ns_prefixes.insert(DummyAtom("svg".into()), DummyAtom(SVG.into())); - assert_eq!(parse_ns("svg|circle", &parser), Ok(SelectorList(vec![Selector { - inner: SelectorInner::from_vec( - vec![ - Component::Namespace(DummyAtom("svg".into()), SVG.into()), - Component::LocalName(LocalName { - name: DummyAtom::from("circle"), - lower_name: DummyAtom::from("circle"), - }) - ]), - specificity_and_flags: specificity(0, 0, 1), - }]))); - assert_eq!(parse_ns("svg|*", &parser), Ok(SelectorList(vec![Selector { - inner: SelectorInner::from_vec( - vec![ - Component::Namespace(DummyAtom("svg".into()), SVG.into()), - Component::ExplicitUniversalType, - ]), - specificity_and_flags: specificity(0, 0, 0), - }]))); + assert_eq!(parse_ns("svg|circle", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::Namespace(DummyAtom("svg".into()), SVG.into()), + Component::LocalName(LocalName { + name: DummyAtom::from("circle"), + lower_name: DummyAtom::from("circle"), + }) + ), specificity(0, 0, 1)) + )))); + assert_eq!(parse_ns("svg|*", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::Namespace(DummyAtom("svg".into()), SVG.into()), + Component::ExplicitUniversalType, + ), specificity(0, 0, 0)) + )))); // Default namespace does not apply to attribute selectors // https://github.com/mozilla/servo/pull/1652 // but it does apply to implicit type selectors // https://github.com/servo/rust-selectors/pull/82 parser.default_ns = Some(MATHML.into()); - assert_eq!(parse_ns("[Foo]", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec( - vec![ - Component::DefaultNamespace(MATHML.into()), - Component::AttributeInNoNamespaceExists { - local_name: DummyAtom::from("Foo"), - local_name_lower: DummyAtom::from("foo"), - }, - ]), - specificity_and_flags: specificity(0, 1, 0), - })))); + assert_eq!(parse_ns("[Foo]", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::DefaultNamespace(MATHML.into()), + Component::AttributeInNoNamespaceExists { + local_name: DummyAtom::from("Foo"), + local_name_lower: DummyAtom::from("foo"), + }, + ), specificity(0, 1, 0)) + )))); // Default namespace does apply to type selectors - assert_eq!(parse_ns("e", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec( - vec!( - Component::DefaultNamespace(MATHML.into()), - Component::LocalName(LocalName { - name: DummyAtom::from("e"), - lower_name: DummyAtom::from("e") }), - )), - specificity_and_flags: specificity(0, 0, 1), - })))); - assert_eq!(parse_ns("*", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec( - vec!( - Component::DefaultNamespace(MATHML.into()), - Component::ExplicitUniversalType, - )), - specificity_and_flags: specificity(0, 0, 0), - })))); - assert_eq!(parse_ns("*|*", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec( - vec!( - Component::ExplicitAnyNamespace, - Component::ExplicitUniversalType, - )), - specificity_and_flags: specificity(0, 0, 0), - })))); + assert_eq!(parse_ns("e", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::DefaultNamespace(MATHML.into()), + Component::LocalName(LocalName { + name: DummyAtom::from("e"), + lower_name: DummyAtom::from("e") }), + ), specificity(0, 0, 1)) + )))); + assert_eq!(parse_ns("*", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::DefaultNamespace(MATHML.into()), + Component::ExplicitUniversalType, + ), specificity(0, 0, 0)) + )))); + assert_eq!(parse_ns("*|*", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::ExplicitAnyNamespace, + Component::ExplicitUniversalType, + ), specificity(0, 0, 0)) + )))); // Default namespace applies to universal and type selectors inside :not and :matches, // but not otherwise. - assert_eq!(parse_ns(":not(.cl)", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + assert_eq!(parse_ns(":not(.cl)", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::DefaultNamespace(MATHML.into()), Component::Negation(vec![ Component::Class(DummyAtom::from("cl")) ].into_boxed_slice()), - )), - specificity_and_flags: specificity(0, 1, 0), - })))); - assert_eq!(parse_ns(":not(*)", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + ), specificity(0, 1, 0)) + )))); + assert_eq!(parse_ns(":not(*)", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::DefaultNamespace(MATHML.into()), Component::Negation(vec![ Component::DefaultNamespace(MATHML.into()), Component::ExplicitUniversalType, ].into_boxed_slice()), - )), - specificity_and_flags: specificity(0, 0, 0), - })))); - assert_eq!(parse_ns(":not(e)", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!( + ), specificity(0, 0, 0)) + )))); + assert_eq!(parse_ns(":not(e)", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::DefaultNamespace(MATHML.into()), Component::Negation(vec![ Component::DefaultNamespace(MATHML.into()), @@ -1919,51 +1903,39 @@ pub mod tests { lower_name: DummyAtom::from("e") }), ].into_boxed_slice()) - )), - specificity_and_flags: specificity(0, 0, 1), - })))); - assert_eq!(parse("[attr |= \"foo\"]"), Ok(SelectorList(vec![Selector { - inner: SelectorInner::from_vec( - vec![ - Component::AttributeInNoNamespace { - local_name: DummyAtom::from("attr"), - local_name_lower: DummyAtom::from("attr"), - operator: AttrSelectorOperator::DashMatch, - value: DummyAtom::from("foo"), - never_matches: false, - case_sensitivity: ParsedCaseSensitivity::CaseSensitive, - } - ]), - specificity_and_flags: specificity(0, 1, 0), - }]))); + ), specificity(0, 0, 1)) + )))); + assert_eq!(parse("[attr |= \"foo\"]"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::AttributeInNoNamespace { + local_name: DummyAtom::from("attr"), + local_name_lower: DummyAtom::from("attr"), + operator: AttrSelectorOperator::DashMatch, + value: DummyAtom::from("foo"), + never_matches: false, + case_sensitivity: ParsedCaseSensitivity::CaseSensitive, + } + ), specificity(0, 1, 0)) + )))); // https://github.com/mozilla/servo/issues/1723 - assert_eq!(parse("::before"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec( - vec![ - Component::PseudoElement(PseudoElement::Before), - ] - ), - specificity_and_flags: specificity(0, 0, 1) | HAS_PSEUDO_BIT, - })))); - assert_eq!(parse("::before:hover"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec( - vec![ - Component::PseudoElement(PseudoElement::Before), - Component::NonTSPseudoClass(PseudoClass::Hover), - ] - ), - specificity_and_flags: specificity(0, 1, 1) | HAS_PSEUDO_BIT, - })))); - assert_eq!(parse("::before:hover:hover"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec( - vec![ - Component::PseudoElement(PseudoElement::Before), - Component::NonTSPseudoClass(PseudoClass::Hover), - Component::NonTSPseudoClass(PseudoClass::Hover), - ] - ), - specificity_and_flags: specificity(0, 2, 1) | HAS_PSEUDO_BIT, - })))); + assert_eq!(parse("::before"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::PseudoElement(PseudoElement::Before), + ), specificity(0, 0, 1) | HAS_PSEUDO_BIT) + )))); + assert_eq!(parse("::before:hover"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::PseudoElement(PseudoElement::Before), + Component::NonTSPseudoClass(PseudoClass::Hover), + ), specificity(0, 1, 1) | HAS_PSEUDO_BIT) + )))); + assert_eq!(parse("::before:hover:hover"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::PseudoElement(PseudoElement::Before), + Component::NonTSPseudoClass(PseudoClass::Hover), + Component::NonTSPseudoClass(PseudoClass::Hover), + ), specificity(0, 2, 1) | HAS_PSEUDO_BIT) + )))); assert_eq!(parse("::before:hover:active"), Err(())); assert_eq!(parse("::before:hover .foo"), Err(())); assert_eq!(parse("::before .foo"), Err(())); @@ -1972,41 +1944,35 @@ pub mod tests { // https://github.com/servo/servo/issues/15335 assert_eq!(parse(":: before"), Err(())); - assert_eq!(parse("div ::after"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec( - vec![ - Component::LocalName(LocalName { - name: DummyAtom::from("div"), - lower_name: DummyAtom::from("div") }), - Component::Combinator(Combinator::Descendant), - Component::Combinator(Combinator::PseudoElement), - Component::PseudoElement(PseudoElement::After), - ]), - specificity_and_flags: specificity(0, 0, 2) | HAS_PSEUDO_BIT, - })))); - assert_eq!(parse("#d1 > .ok"), Ok(SelectorList(vec![Selector { - inner: SelectorInner::from_vec( - vec![ - Component::ID(DummyAtom::from("d1")), - Component::Combinator(Combinator::Child), - Component::Class(DummyAtom::from("ok")), - ]), - specificity_and_flags: (1 << 20) + (1 << 10) + (0 << 0), - }]))); + assert_eq!(parse("div ::after"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::LocalName(LocalName { + name: DummyAtom::from("div"), + lower_name: DummyAtom::from("div") }), + Component::Combinator(Combinator::Descendant), + Component::Combinator(Combinator::PseudoElement), + Component::PseudoElement(PseudoElement::After), + ), specificity(0, 0, 2) | HAS_PSEUDO_BIT) + )))); + assert_eq!(parse("#d1 > .ok"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::ID(DummyAtom::from("d1")), + Component::Combinator(Combinator::Child), + Component::Class(DummyAtom::from("ok")), + ), (1 << 20) + (1 << 10) + (0 << 0)) + )))); parser.default_ns = None; assert_eq!(parse(":not(#provel.old)"), Err(())); assert_eq!(parse(":not(#provel > old)"), Err(())); assert!(parse("table[rules]:not([rules = \"none\"]):not([rules = \"\"])").is_ok()); - assert_eq!(parse(":not(#provel)"), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!(Component::Negation( - vec![ + assert_eq!(parse(":not(#provel)"), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!(Component::Negation(vec!( Component::ID(DummyAtom::from("provel")), - ].into_boxed_slice() - ))), - specificity_and_flags: specificity(1, 0, 0), - })))); - assert_eq!(parse_ns(":not(svg|circle)", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!(Component::Negation( + ).into_boxed_slice() + )), specificity(1, 0, 0)) + )))); + assert_eq!(parse_ns(":not(svg|circle)", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!(Component::Negation( vec![ Component::Namespace(DummyAtom("svg".into()), SVG.into()), Component::LocalName(LocalName { @@ -2014,52 +1980,47 @@ pub mod tests { lower_name: DummyAtom::from("circle") }), ].into_boxed_slice() - ))), - specificity_and_flags: specificity(0, 0, 1), - })))); + )), specificity(0, 0, 1)) + )))); // https://github.com/servo/servo/issues/16017 - assert_eq!(parse_ns(":not(*)", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!(Component::Negation( + assert_eq!(parse_ns(":not(*)", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!(Component::Negation( vec![ Component::ExplicitUniversalType, ].into_boxed_slice() - ))), - specificity_and_flags: specificity(0, 0, 0), - })))); - assert_eq!(parse_ns(":not(|*)", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!(Component::Negation( + )), specificity(0, 0, 0)) + )))); + assert_eq!(parse_ns(":not(|*)", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!(Component::Negation( vec![ Component::ExplicitNoNamespace, Component::ExplicitUniversalType, ].into_boxed_slice() - ))), - specificity_and_flags: specificity(0, 0, 0), - })))); - assert_eq!(parse_ns(":not(*|*)", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!(Component::Negation( + )), specificity(0, 0, 0)) + )))); + assert_eq!(parse_ns(":not(*|*)", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!(Component::Negation( vec![ Component::ExplicitAnyNamespace, Component::ExplicitUniversalType, ].into_boxed_slice() - ))), - specificity_and_flags: specificity(0, 0, 0), - })))); - assert_eq!(parse_ns(":not(svg|*)", &parser), Ok(SelectorList(vec!(Selector { - inner: SelectorInner::from_vec(vec!(Component::Negation( + )), specificity(0, 0, 0)) + )))); + assert_eq!(parse_ns(":not(svg|*)", &parser), Ok(SelectorList::from_vec(vec!( + Selector::from_vec(vec!(Component::Negation( vec![ Component::Namespace(DummyAtom("svg".into()), SVG.into()), Component::ExplicitUniversalType, ].into_boxed_slice() - ))), - specificity_and_flags: specificity(0, 0, 0), - })))); + )), specificity(0, 0, 0)) + )))); } #[test] fn test_pseudo_iter() { - let selector = &parse("q::before").unwrap().0[0]; + let selector = &parse("q::before").unwrap().0[0].selector; assert!(!selector.is_universal()); - let mut iter = selector.inner.complex.iter(); + let mut iter = selector.iter(); assert_eq!(iter.next(), Some(&Component::PseudoElement(PseudoElement::Before))); assert_eq!(iter.next(), None); let combinator = iter.next_sequence(); @@ -2071,15 +2032,15 @@ pub mod tests { #[test] fn test_universal() { - let selector = &parse("*|*::before").unwrap().0[0]; + let selector = &parse("*|*::before").unwrap().0[0].selector; assert!(selector.is_universal()); } #[test] fn test_empty_pseudo_iter() { - let selector = &parse("::before").unwrap().0[0]; + let selector = &parse("::before").unwrap().0[0].selector; assert!(selector.is_universal()); - let mut iter = selector.inner.complex.iter(); + let mut iter = selector.iter(); assert_eq!(iter.next(), Some(&Component::PseudoElement(PseudoElement::Before))); assert_eq!(iter.next(), None); assert_eq!(iter.next_sequence(), None); @@ -2103,11 +2064,11 @@ pub mod tests { #[test] fn visitor() { let mut test_visitor = TestVisitor { seen: vec![], }; - parse(":not(:hover) ~ label").unwrap().0[0].visit(&mut test_visitor); + parse(":not(:hover) ~ label").unwrap().0[0].selector.visit(&mut test_visitor); assert!(test_visitor.seen.contains(&":hover".into())); let mut test_visitor = TestVisitor { seen: vec![], }; - parse("::before:hover").unwrap().0[0].visit(&mut test_visitor); + parse("::before:hover").unwrap().0[0].selector.visit(&mut test_visitor); assert!(test_visitor.seen.contains(&":hover".into())); } } diff --git a/components/selectors/size_of_tests.rs b/components/selectors/size_of_tests.rs index 5fec3085fcb..d9ed1651cf7 100644 --- a/components/selectors/size_of_tests.rs +++ b/components/selectors/size_of_tests.rs @@ -11,10 +11,8 @@ use precomputed_hash::PrecomputedHash; use std::fmt; use visitor::SelectorVisitor; -size_of_test!(size_of_selector, Selector, 48); +size_of_test!(size_of_selector, Selector, 8); size_of_test!(size_of_pseudo_element, gecko_like_types::PseudoElement, 1); -size_of_test!(size_of_selector_inner, SelectorInner, 40); -size_of_test!(size_of_complex_selector, ComplexSelector, 24); size_of_test!(size_of_component, Component, 32); size_of_test!(size_of_pseudo_class, PseudoClass, 24); diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs index ce465b0f1ac..686a3979a9a 100644 --- a/tests/unit/style/stylesheets.rs +++ b/tests/unit/style/stylesheets.rs @@ -90,26 +90,23 @@ fn test_parse_stylesheet() { }, }))), CssRule::Style(Arc::new(stylesheet.shared_lock.wrap(StyleRule { - selectors: SelectorList(vec![ - Selector::new_for_unit_testing( - SelectorInner::from_vec(vec![ - Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), - Component::LocalName(LocalName { - name: local_name!("input"), - lower_name: local_name!("input"), - }), - Component::AttributeInNoNamespace { - local_name: local_name!("type"), - local_name_lower: local_name!("type"), - operator: AttrSelectorOperator::Equal, - value: "hidden".to_owned(), - case_sensitivity: ParsedCaseSensitivity::AsciiCaseInsensitive, - never_matches: false, - } - ]), - (0 << 20) + (1 << 10) + (1 << 0) - ), - ]), + selectors: SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), + Component::LocalName(LocalName { + name: local_name!("input"), + lower_name: local_name!("input"), + }), + Component::AttributeInNoNamespace { + local_name: local_name!("type"), + local_name_lower: local_name!("type"), + operator: AttrSelectorOperator::Equal, + value: "hidden".to_owned(), + case_sensitivity: ParsedCaseSensitivity::AsciiCaseInsensitive, + never_matches: false, + } + ), (0 << 20) + (1 << 10) + (1 << 0)) + )), block: Arc::new(stylesheet.shared_lock.wrap(block_from(vec![ (PropertyDeclaration::Display(longhands::display::SpecifiedValue::none), Importance::Important), @@ -123,28 +120,23 @@ fn test_parse_stylesheet() { }, }))), CssRule::Style(Arc::new(stylesheet.shared_lock.wrap(StyleRule { - selectors: SelectorList(vec![ - Selector::new_for_unit_testing( - SelectorInner::from_vec(vec![ + selectors: SelectorList::from_vec(vec!( + Selector::from_vec(vec!( Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), Component::LocalName(LocalName { name: local_name!("html"), lower_name: local_name!("html"), }), - ]), - (0 << 20) + (0 << 10) + (1 << 0) + ), (0 << 20) + (0 << 10) + (1 << 0)), + Selector::from_vec(vec!( + Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), + Component::LocalName(LocalName { + name: local_name!("body"), + lower_name: local_name!("body"), + }) + ), (0 << 20) + (0 << 10) + (1 << 0) ), - Selector::new_for_unit_testing( - SelectorInner::from_vec(vec![ - Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), - Component::LocalName(LocalName { - name: local_name!("body"), - lower_name: local_name!("body"), - }), - ]), - (0 << 20) + (0 << 10) + (1 << 0) - ), - ]), + )), block: Arc::new(stylesheet.shared_lock.wrap(block_from(vec![ (PropertyDeclaration::Display(longhands::display::SpecifiedValue::block), Importance::Normal), @@ -155,18 +147,15 @@ fn test_parse_stylesheet() { }, }))), CssRule::Style(Arc::new(stylesheet.shared_lock.wrap(StyleRule { - selectors: SelectorList(vec![ - Selector::new_for_unit_testing( - SelectorInner::from_vec(vec![ - Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), - Component::ID(Atom::from("d1")), - Component::Combinator(Combinator::Child), - Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), - Component::Class(Atom::from("ok")), - ]), - (1 << 20) + (1 << 10) + (0 << 0) - ), - ]), + selectors: SelectorList::from_vec(vec!( + Selector::from_vec(vec!( + Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), + Component::ID(Atom::from("d1")), + Component::Combinator(Combinator::Child), + Component::DefaultNamespace(NsAtom::from("http://www.w3.org/1999/xhtml")), + Component::Class(Atom::from("ok")) + ), (1 << 20) + (1 << 10) + (0 << 0)) + )), block: Arc::new(stylesheet.shared_lock.wrap(block_from(vec![ (PropertyDeclaration::BackgroundColor( longhands::background_color::SpecifiedValue { diff --git a/tests/unit/style/stylist.rs b/tests/unit/style/stylist.rs index afd967338f9..17faaddd04a 100644 --- a/tests/unit/style/stylist.rs +++ b/tests/unit/style/stylist.rs @@ -45,7 +45,7 @@ fn get_mock_rules(css_selectors: &[&str]) -> (Vec>, SharedRwLock) { let guard = shared_lock.read(); let rule = locked.read_with(&guard); rule.selectors.0.iter().map(|s| { - Rule::new(s.clone(), locked.clone(), i) + Rule::new(s.selector.clone(), s.hashes.clone(), locked.clone(), i) }).collect() }).collect(), shared_lock) } @@ -68,7 +68,7 @@ fn parse_selectors(selectors: &[&str]) -> Vec> { .map(|x| SelectorParser::parse_author_origin_no_namespace(x).unwrap().0 .into_iter() .nth(0) - .unwrap()) + .unwrap().selector) .collect() } @@ -126,7 +126,6 @@ fn test_revalidation_selectors() { "p:first-child span", ]).into_iter() .filter(|s| needs_revalidation(&s)) - .map(|s| s.inner.complex) .collect::>(); let reference = parse_selectors(&[ @@ -167,7 +166,6 @@ fn test_revalidation_selectors() { // Selectors in the ancestor chain (needed for cousin sharing). "p:first-child span", ]).into_iter() - .map(|s| s.inner.complex) .collect::>(); assert_eq!(test.len(), reference.len()); @@ -189,22 +187,22 @@ fn test_rule_ordering_same_specificity() { #[test] fn test_get_id_name() { let (rules_list, _) = get_mock_rules(&[".intro", "#top"]); - assert_eq!(selector_map::get_id_name(&rules_list[0][0].selector.inner), None); - assert_eq!(selector_map::get_id_name(&rules_list[1][0].selector.inner), Some(Atom::from("top"))); + assert_eq!(selector_map::get_id_name(rules_list[0][0].selector.iter()), None); + assert_eq!(selector_map::get_id_name(rules_list[1][0].selector.iter()), Some(Atom::from("top"))); } #[test] fn test_get_class_name() { let (rules_list, _) = get_mock_rules(&[".intro.foo", "#top"]); - assert_eq!(selector_map::get_class_name(&rules_list[0][0].selector.inner), Some(Atom::from("foo"))); - assert_eq!(selector_map::get_class_name(&rules_list[1][0].selector.inner), None); + assert_eq!(selector_map::get_class_name(rules_list[0][0].selector.iter()), Some(Atom::from("foo"))); + assert_eq!(selector_map::get_class_name(rules_list[1][0].selector.iter()), None); } #[test] fn test_get_local_name() { let (rules_list, _) = get_mock_rules(&["img.foo", "#top", "IMG", "ImG"]); let check = |i: usize, names: Option<(&str, &str)>| { - assert!(selector_map::get_local_name(&rules_list[i][0].selector.inner) + assert!(selector_map::get_local_name(rules_list[i][0].selector.iter()) == names.map(|(name, lower_name)| LocalNameSelector { name: LocalName::from(name), lower_name: LocalName::from(lower_name) }))