From 6515f78f6dee9b5825c7b5b9c0590ba3fb86a461 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Mon, 10 Apr 2017 12:22:33 +0800 Subject: [PATCH 1/7] Check for empty when determining what selectors to use for revalidation. --- components/style/matching.rs | 12 +++++------- components/style/restyle_hints.rs | 27 ++++++++++++++++++--------- components/style/stylist.rs | 28 +++++++++++++++------------- 3 files changed, 38 insertions(+), 29 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 2f2468413f5..820a8e2f061 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -154,9 +154,7 @@ fn element_matches_candidate(element: &E, miss!(PresHints) } - if !match_same_sibling_affecting_rules(element, - candidate_element, - shared_context) { + if !revalidate(element, candidate_element, shared_context) { miss!(SiblingRules) } @@ -212,10 +210,10 @@ fn match_same_style_affecting_attributes_rules(element: &E, } #[inline] -fn match_same_sibling_affecting_rules(element: &E, - candidate: &E, - ctx: &SharedStyleContext) -> bool { - ctx.stylist.match_same_sibling_affecting_rules(element, candidate) +fn revalidate(element: &E, + candidate: &E, + ctx: &SharedStyleContext) -> bool { + ctx.stylist.revalidate_entry_for_cache(element, candidate) } static STYLE_SHARING_CANDIDATE_CACHE_SIZE: usize = 8; diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index dd04079c783..9fe5e8a439e 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -400,8 +400,15 @@ fn is_attr_selector(sel: &SimpleSelector) -> bool { } } -fn is_sibling_affecting_selector(sel: &SimpleSelector) -> bool { +/// Whether a selector containing this simple selector needs to be explicitly +/// matched against both the style sharing cache entry and the candidate. +/// +/// +/// We use this for selectors that can have different matching behavior between +/// siblings that are otherwise identical as far as the cache is concerned. +fn needs_cache_revalidation(sel: &SimpleSelector) -> bool { match *sel { + SimpleSelector::Empty | SimpleSelector::FirstChild | SimpleSelector::LastChild | SimpleSelector::OnlyChild | @@ -412,6 +419,7 @@ fn is_sibling_affecting_selector(sel: &SimpleSelector) -> bool { SimpleSelector::FirstOfType | SimpleSelector::LastOfType | SimpleSelector::OnlyOfType => true, + SimpleSelector::NonTSPseudoClass(ref p) => p.state_flag().is_empty(), _ => false, } } @@ -481,7 +489,7 @@ struct Dependency { /// selector. pub struct SelectorDependencyVisitor<'a> { dependency_set: &'a mut DependencySet, - affects_siblings: bool, + needs_cache_revalidation: bool, affected_by_attribute: bool, } @@ -490,14 +498,15 @@ impl<'a> SelectorDependencyVisitor<'a> { pub fn new(dependency_set: &'a mut DependencySet) -> Self { SelectorDependencyVisitor { dependency_set: dependency_set, - affects_siblings: false, + needs_cache_revalidation: false, affected_by_attribute: false, } } - /// Returns whether this visitor has known of a sibling-dependent selector. - pub fn affects_siblings(&self) -> bool { - self.affects_siblings + /// Returns whether this visitor has encountered a simple selector that needs + /// cache revalidation. + pub fn needs_cache_revalidation(&self) -> bool { + self.needs_cache_revalidation } /// Returns whether this visitor has known of a attribute-dependent @@ -518,8 +527,8 @@ impl<'a> SelectorVisitor for SelectorDependencyVisitor<'a> { let mut sensitivities = Sensitivities::new(); for s in &selector.compound_selector { sensitivities.states.insert(selector_to_state(s)); - if !self.affects_siblings { - self.affects_siblings = is_sibling_affecting_selector(s); + if !self.needs_cache_revalidation { + self.needs_cache_revalidation = needs_cache_revalidation(s); } if !sensitivities.attrs { sensitivities.attrs = is_attr_selector(s); @@ -529,7 +538,7 @@ impl<'a> SelectorVisitor for SelectorDependencyVisitor<'a> { let hint = combinator_to_restyle_hint(combinator); self.affected_by_attribute |= sensitivities.attrs; - self.affects_siblings |= hint.intersects(RESTYLE_LATER_SIBLINGS); + self.needs_cache_revalidation |= hint.intersects(RESTYLE_LATER_SIBLINGS); if !sensitivities.is_empty() { self.dependency_set.add_dependency(Dependency { diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 4731ec84cb9..64f9df5fc44 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -111,9 +111,10 @@ pub struct Stylist { /// Selector dependencies used to compute restyle hints. state_deps: DependencySet, - /// Selectors in the page affecting siblings + /// Selectors that require explicit cache revalidation (i.e. which depend + /// on state that is not otherwise visible to the cache, like child index). #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] - sibling_affecting_selectors: Vec>, + selectors_for_cache_revalidation: Vec>, /// Selectors in the page matching elements with non-common style-affecting /// attributes. @@ -170,7 +171,7 @@ impl Stylist { rule_tree: RuleTree::new(), state_deps: DependencySet::new(), - sibling_affecting_selectors: vec![], + selectors_for_cache_revalidation: vec![], style_affecting_attributes_selectors: vec![] }; @@ -225,7 +226,7 @@ impl Stylist { self.state_deps.clear(); self.animations.clear(); - self.sibling_affecting_selectors.clear(); + self.selectors_for_cache_revalidation.clear(); self.style_affecting_attributes_selectors.clear(); extra_data.clear_font_faces(); @@ -246,8 +247,8 @@ impl Stylist { } debug!("Stylist stats:"); - debug!(" - Got {} sibling-affecting selectors", - self.sibling_affecting_selectors.len()); + debug!(" - Got {} selectors for cache revalidation", + self.selectors_for_cache_revalidation.len()); debug!(" - Got {} non-common-style-attribute-affecting selectors", self.style_affecting_attributes_selectors.len()); debug!(" - Got {} deps for style-hint calculation", @@ -304,8 +305,8 @@ impl Stylist { SelectorDependencyVisitor::new(&mut self.state_deps); selector.visit(&mut visitor); - if visitor.affects_siblings() { - self.sibling_affecting_selectors.push(selector.clone()); + if visitor.needs_cache_revalidation() { + self.selectors_for_cache_revalidation.push(selector.clone()); } if visitor.affected_by_attribute() { @@ -813,12 +814,13 @@ impl Stylist { self.rule_tree.root() } - /// Returns whether two elements match the same sibling-affecting rules. + /// Returns whether two elements match the same set of revalidation- + /// requiring rules. /// /// This is also for the style sharing candidate cache. - pub fn match_same_sibling_affecting_rules(&self, - element: &E, - candidate: &E) -> bool + pub fn revalidate_entry_for_cache(&self, + element: &E, + candidate: &E) -> bool where E: TElement, { use selectors::matching::StyleRelations; @@ -828,7 +830,7 @@ impl Stylist { // // TODO(emilio): Use the bloom filter, since they contain the element's // ancestor chain and it's correct for the candidate too. - for ref selector in self.sibling_affecting_selectors.iter() { + for ref selector in self.selectors_for_cache_revalidation.iter() { let element_matches = matches_complex_selector(&selector.complex_selector, element, None, &mut StyleRelations::empty(), From d51d95d0c7714d443536ee098e5e9022f4fbe810 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 11 Apr 2017 14:06:11 +0800 Subject: [PATCH 2/7] Merge style_affecting_attributes_selectors with selectors_for_cache_revalidation. --- components/style/matching.rs | 28 +++------------- components/style/restyle_hints.rs | 10 +----- components/style/stylist.rs | 53 ++----------------------------- 3 files changed, 8 insertions(+), 83 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 820a8e2f061..571a8135276 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -95,12 +95,9 @@ pub enum CacheMiss { Class, /// The presentation hints didn't match. PresHints, - /// The element and the candidate didn't match the same set of - /// sibling-affecting rules. - SiblingRules, - /// The element and the candidate didn't match the same set of style - /// affecting attribute selectors. - AttrRules, + /// The element and the candidate didn't match the same set of revalidation + /// selectors. + Revalidation, } fn element_matches_candidate(element: &E, @@ -155,13 +152,7 @@ fn element_matches_candidate(element: &E, } if !revalidate(element, candidate_element, shared_context) { - miss!(SiblingRules) - } - - if !match_same_style_affecting_attributes_rules(element, - candidate_element, - shared_context) { - miss!(AttrRules) + miss!(Revalidation) } let data = candidate_element.borrow_data().unwrap(); @@ -201,14 +192,6 @@ fn have_same_class(element: &E, element_class_attributes == *candidate.class_attributes.as_ref().unwrap() } -// TODO: These re-match the candidate every time, which is suboptimal. -#[inline] -fn match_same_style_affecting_attributes_rules(element: &E, - candidate: &E, - ctx: &SharedStyleContext) -> bool { - ctx.stylist.match_same_style_affecting_attributes_rules(element, candidate) -} - #[inline] fn revalidate(element: &E, candidate: &E, @@ -1031,8 +1014,7 @@ pub trait MatchMethods : TElement { // Too expensive failure, give up, we don't want another // one of these. CacheMiss::PresHints | - CacheMiss::SiblingRules | - CacheMiss::AttrRules => break, + CacheMiss::Revalidation => break, _ => {} } } diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 9fe5e8a439e..594246f4838 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -490,7 +490,6 @@ struct Dependency { pub struct SelectorDependencyVisitor<'a> { dependency_set: &'a mut DependencySet, needs_cache_revalidation: bool, - affected_by_attribute: bool, } impl<'a> SelectorDependencyVisitor<'a> { @@ -499,7 +498,6 @@ impl<'a> SelectorDependencyVisitor<'a> { SelectorDependencyVisitor { dependency_set: dependency_set, needs_cache_revalidation: false, - affected_by_attribute: false, } } @@ -508,12 +506,6 @@ impl<'a> SelectorDependencyVisitor<'a> { pub fn needs_cache_revalidation(&self) -> bool { self.needs_cache_revalidation } - - /// Returns whether this visitor has known of a attribute-dependent - /// selector. - pub fn affected_by_attribute(&self) -> bool { - self.affected_by_attribute - } } impl<'a> SelectorVisitor for SelectorDependencyVisitor<'a> { @@ -537,7 +529,7 @@ impl<'a> SelectorVisitor for SelectorDependencyVisitor<'a> { let hint = combinator_to_restyle_hint(combinator); - self.affected_by_attribute |= sensitivities.attrs; + self.needs_cache_revalidation |= sensitivities.attrs; self.needs_cache_revalidation |= hint.intersects(RESTYLE_LATER_SIBLINGS); if !sensitivities.is_empty() { diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 64f9df5fc44..f108d75fd76 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -112,14 +112,10 @@ pub struct Stylist { state_deps: DependencySet, /// Selectors that require explicit cache revalidation (i.e. which depend - /// on state that is not otherwise visible to the cache, like child index). + /// 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: Vec>, - - /// Selectors in the page matching elements with non-common style-affecting - /// attributes. - #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] - style_affecting_attributes_selectors: Vec>, } /// This struct holds data which user of Stylist may want to extract @@ -172,7 +168,6 @@ impl Stylist { state_deps: DependencySet::new(), selectors_for_cache_revalidation: vec![], - style_affecting_attributes_selectors: vec![] }; SelectorImpl::each_eagerly_cascaded_pseudo_element(|pseudo| { @@ -227,7 +222,6 @@ impl Stylist { self.animations.clear(); self.selectors_for_cache_revalidation.clear(); - self.style_affecting_attributes_selectors.clear(); extra_data.clear_font_faces(); @@ -249,8 +243,6 @@ impl Stylist { debug!("Stylist stats:"); debug!(" - Got {} selectors for cache revalidation", self.selectors_for_cache_revalidation.len()); - debug!(" - Got {} non-common-style-attribute-affecting selectors", - self.style_affecting_attributes_selectors.len()); debug!(" - Got {} deps for style-hint calculation", self.state_deps.len()); @@ -308,10 +300,6 @@ impl Stylist { if visitor.needs_cache_revalidation() { self.selectors_for_cache_revalidation.push(selector.clone()); } - - if visitor.affected_by_attribute() { - self.style_affecting_attributes_selectors.push(selector.clone()); - } } } CssRule::Import(ref import) => { @@ -771,43 +759,6 @@ impl Stylist { &self.animations } - /// Whether two elements match the same not-common style-affecting attribute - /// rules. - /// - /// This is used to test elements and candidates in the style-sharing - /// candidate cache. - pub fn match_same_style_affecting_attributes_rules(&self, - element: &E, - candidate: &E) -> bool - where E: TElement, - { - use selectors::matching::StyleRelations; - use selectors::matching::matches_complex_selector; - // TODO(emilio): we can probably do better, the candidate should already - // know what rules it matches. Also, we should only match until we find - // a descendant combinator, the rest should be ok, since the parent is - // the same. - // - // TODO(emilio): Use the bloom filter, since they contain the element's - // ancestor chain and it's correct for the candidate too. - for ref selector in self.style_affecting_attributes_selectors.iter() { - let element_matches = - matches_complex_selector(&selector.complex_selector, element, - None, &mut StyleRelations::empty(), - &mut |_, _| {}); - let candidate_matches = - matches_complex_selector(&selector.complex_selector, candidate, - None, &mut StyleRelations::empty(), - &mut |_, _| {}); - - if element_matches != candidate_matches { - return false; - } - } - - true - } - /// Returns the rule root node. #[inline] pub fn rule_tree_root(&self) -> StrongRuleNode { From 1663bf6e5dbf73ffdca727371193231f1a25d1a3 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 11 Apr 2017 14:23:35 +0800 Subject: [PATCH 3/7] Cache the results of cache entry revalidation and use the bloom filter. --- Cargo.lock | 1 + components/style/Cargo.toml | 1 + components/style/context.rs | 5 ++ components/style/lib.rs | 1 + components/style/matching.rs | 93 ++++++++++++++++++++++++++++------- components/style/stylist.rs | 44 +++++++---------- components/style/traversal.rs | 4 +- 7 files changed, 100 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 877f67e4dc4..97999fcbb15 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2733,6 +2733,7 @@ dependencies = [ "app_units 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "atomic_refcell 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "bindgen 0.22.0 (registry+https://github.com/rust-lang/crates.io-index)", + "bit-vec 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)", "bitflags 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)", "cfg-if 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/components/style/Cargo.toml b/components/style/Cargo.toml index 10ed72aaea3..6ea87409078 100644 --- a/components/style/Cargo.toml +++ b/components/style/Cargo.toml @@ -25,6 +25,7 @@ testing = [] app_units = "0.4" atomic_refcell = "0.1" bitflags = "0.7" +bit-vec = "0.4.3" byteorder = "1.0" cfg-if = "0.1.0" cssparser = "0.12.1" diff --git a/components/style/context.rs b/components/style/context.rs index 583b5bda187..17a49ad6040 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -7,6 +7,7 @@ use animation::{Animation, PropertyAnimation}; use app_units::Au; +use bit_vec::BitVec; use bloom::StyleBloom; use data::ElementData; use dom::{OpaqueNode, TNode, TElement, SendElement}; @@ -111,6 +112,9 @@ pub struct CurrentElementInfo { element: OpaqueNode, /// Whether the element is being styled for the first time. is_initial_style: bool, + /// Lazy cache of the result of matching the current element against the + /// revalidation selectors. + pub revalidation_match_results: Option, /// A Vec of possibly expired animations. Used only by Servo. #[allow(dead_code)] pub possibly_expired_animations: Vec, @@ -317,6 +321,7 @@ impl ThreadLocalStyleContext { self.current_element_info = Some(CurrentElementInfo { element: element.as_node().opaque(), is_initial_style: !data.has_styles(), + revalidation_match_results: None, possibly_expired_animations: Vec::new(), }); } diff --git a/components/style/lib.rs b/components/style/lib.rs index 171b8081fc2..41d03644acf 100644 --- a/components/style/lib.rs +++ b/components/style/lib.rs @@ -39,6 +39,7 @@ extern crate app_units; extern crate atomic_refcell; +extern crate bit_vec; #[macro_use] extern crate bitflags; #[allow(unused_extern_crates)] extern crate byteorder; diff --git a/components/style/matching.rs b/components/style/matching.rs index 571a8135276..7375f2f3a2a 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -10,9 +10,10 @@ use Atom; use animation::{self, Animation, PropertyAnimation}; use atomic_refcell::AtomicRefMut; +use bit_vec::BitVec; use cache::{LRUCache, LRUCacheMutIterator}; use cascade_info::CascadeInfo; -use context::{SequentialTask, SharedStyleContext, StyleContext}; +use context::{CurrentElementInfo, SequentialTask, SharedStyleContext, StyleContext}; use data::{ComputedStyle, ElementData, ElementStyles, RestyleData}; use dom::{AnimationRules, SendElement, TElement, TNode}; use font_metrics::FontMetricsProvider; @@ -53,6 +54,8 @@ struct StyleSharingCandidate { element: SendElement, /// The cached class names. class_attributes: Option>, + /// The cached result of matching this entry against the revalidation selectors. + revalidation_match_results: Option, } impl PartialEq> for StyleSharingCandidate { @@ -103,7 +106,9 @@ pub enum CacheMiss { fn element_matches_candidate(element: &E, candidate: &mut StyleSharingCandidate, candidate_element: &E, - shared_context: &SharedStyleContext) + shared: &SharedStyleContext, + bloom: &BloomFilter, + info: &mut CurrentElementInfo) -> Result { macro_rules! miss { ($miss: ident) => { @@ -151,7 +156,7 @@ fn element_matches_candidate(element: &E, miss!(PresHints) } - if !revalidate(element, candidate_element, shared_context) { + if !revalidate(element, candidate, candidate_element, shared, bloom, info) { miss!(Revalidation) } @@ -194,9 +199,37 @@ fn have_same_class(element: &E, #[inline] fn revalidate(element: &E, - candidate: &E, - ctx: &SharedStyleContext) -> bool { - ctx.stylist.revalidate_entry_for_cache(element, candidate) + candidate: &mut StyleSharingCandidate, + candidate_element: &E, + shared: &SharedStyleContext, + bloom: &BloomFilter, + info: &mut CurrentElementInfo) + -> bool { + // NB: We could avoid matching ancestor selectors entirely (rather than + // just depending on the bloom filter), at the expense of some complexity. + // Gecko bug 1354965 tracks this. + // + // We could also be even more careful about only matching the minimal number + // of revalidation selectors until we find a mismatch. Gecko bug 1355668 + // tracks this. + // + // These potential optimizations may not be worth the complexity. + let stylist = &shared.stylist; + + if info.revalidation_match_results.is_none() { + info.revalidation_match_results = + Some(stylist.match_revalidation_selectors(element, bloom)); + } + + if candidate.revalidation_match_results.is_none() { + candidate.revalidation_match_results = + Some(stylist.match_revalidation_selectors(candidate_element, bloom)); + } + + let for_element = info.revalidation_match_results.as_ref().unwrap(); + let for_candidate = candidate.revalidation_match_results.as_ref().unwrap(); + debug_assert!(for_element.len() == for_candidate.len()); + for_element == for_candidate } static STYLE_SHARING_CANDIDATE_CACHE_SIZE: usize = 8; @@ -219,7 +252,8 @@ impl StyleSharingCandidateCache { pub fn insert_if_possible(&mut self, element: &E, style: &Arc, - relations: StyleRelations) { + relations: StyleRelations, + revalidation_match_results: Option) { let parent = match element.parent_element() { Some(element) => element, None => { @@ -252,6 +286,7 @@ impl StyleSharingCandidateCache { self.cache.insert(StyleSharingCandidate { element: unsafe { SendElement::new(*element) }, class_attributes: None, + revalidation_match_results: revalidation_match_results, }); } @@ -632,11 +667,14 @@ trait PrivateMatchMethods: TElement { } fn share_style_with_candidate_if_possible(&self, - shared_context: &SharedStyleContext, - candidate: &mut StyleSharingCandidate) + candidate: &mut StyleSharingCandidate, + shared: &SharedStyleContext, + bloom: &BloomFilter, + info: &mut CurrentElementInfo) -> Result { let candidate_element = *candidate.element; - element_matches_candidate(self, candidate, &candidate_element, shared_context) + element_matches_candidate(self, candidate, &candidate_element, + shared, bloom, info) } } @@ -699,11 +737,23 @@ pub trait MatchMethods : TElement { // If the style is shareable, add it to the LRU cache. if sharing == StyleSharingBehavior::Allow && relations_are_shareable(&primary_relations) { + // If we previously tried to match this element against the cache, + // the revalidation match results will already be cached. Otherwise + // we'll have None, and compute them later on-demand. + // + // If we do have the results, grab them here to satisfy the borrow + // checker. + let revalidation_match_results = context.thread_local + .current_element_info + .as_mut().unwrap() + .revalidation_match_results + .take(); context.thread_local .style_sharing_candidate_cache .insert_if_possible(self, data.styles().primary.values(), - primary_relations); + primary_relations, + revalidation_match_results); } } @@ -951,9 +1001,7 @@ pub trait MatchMethods : TElement { /// live nodes in it, and we have no way to guarantee that at the type /// system level yet. unsafe fn share_style_if_possible(&self, - style_sharing_candidate_cache: - &mut StyleSharingCandidateCache, - shared_context: &SharedStyleContext, + context: &mut StyleContext, data: &mut AtomicRefMut) -> StyleSharingResult { if is_share_style_cache_disabled() { @@ -972,11 +1020,17 @@ pub trait MatchMethods : TElement { return StyleSharingResult::CannotShare } + let cache = &mut context.thread_local.style_sharing_candidate_cache; + let current_element_info = + &mut context.thread_local.current_element_info.as_mut().unwrap(); + let bloom = context.thread_local.bloom_filter.filter(); let mut should_clear_cache = false; - for (i, candidate) in style_sharing_candidate_cache.iter_mut().enumerate() { + for (i, candidate) in cache.iter_mut().enumerate() { let sharing_result = - self.share_style_with_candidate_if_possible(shared_context, - candidate); + self.share_style_with_candidate_if_possible(candidate, + &context.shared, + bloom, + current_element_info); match sharing_result { Ok(shared_style) => { // Yay, cache hit. Share the style. @@ -986,7 +1040,8 @@ pub trait MatchMethods : TElement { let old_values = data.get_styles_mut() .and_then(|s| s.primary.values.take()); if let Some(old) = old_values { - self.accumulate_damage(shared_context, data.restyle_mut(), &old, + self.accumulate_damage(&context.shared, + data.restyle_mut(), &old, shared_style.values(), None); } @@ -1021,7 +1076,7 @@ pub trait MatchMethods : TElement { } } if should_clear_cache { - style_sharing_candidate_cache.clear(); + cache.clear(); } StyleSharingResult::CannotShare diff --git a/components/style/stylist.rs b/components/style/stylist.rs index f108d75fd76..e0c8af2ba8b 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -7,6 +7,7 @@ #![deny(missing_docs)] use {Atom, LocalName}; +use bit_vec::BitVec; use data::ComputedStyle; use dom::{AnimationRules, PresentationalHintsSynthetizer, TElement}; use error_reporting::StdoutErrorReporter; @@ -765,41 +766,30 @@ impl Stylist { self.rule_tree.root() } - /// Returns whether two elements match the same set of revalidation- - /// requiring rules. - /// - /// This is also for the style sharing candidate cache. - pub fn revalidate_entry_for_cache(&self, - element: &E, - candidate: &E) -> bool + /// Computes the match results of a given element against the set of + /// revalidation selectors. + pub fn match_revalidation_selectors(&self, + element: &E, + bloom: &BloomFilter) + -> BitVec where E: TElement, { use selectors::matching::StyleRelations; use selectors::matching::matches_complex_selector; - // TODO(emilio): we can probably do better, the candidate should already - // know what rules it matches. - // - // TODO(emilio): Use the bloom filter, since they contain the element's - // ancestor chain and it's correct for the candidate too. - for ref selector in self.selectors_for_cache_revalidation.iter() { - let element_matches = - matches_complex_selector(&selector.complex_selector, element, - None, &mut StyleRelations::empty(), - &mut |_, _| {}); - let candidate_matches = - matches_complex_selector(&selector.complex_selector, candidate, - None, &mut StyleRelations::empty(), - &mut |_, _| {}); + let len = self.selectors_for_cache_revalidation.len(); + let mut results = BitVec::from_elem(len, false); - if element_matches != candidate_matches { - debug!("match_same_sibling_affecting_rules: Failure due to {:?}", - selector.complex_selector); - return false; - } + for (i, ref selector) in self.selectors_for_cache_revalidation + .iter().enumerate() { + results.set(i, matches_complex_selector(&selector.complex_selector, + element, + Some(bloom), + &mut StyleRelations::empty(), + &mut |_, _| {})); } - true + results } /// Given an element, and a snapshot that represents a previous state of the diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 67828e6a7ec..d0910de2c6d 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -680,15 +680,13 @@ fn compute_style(_traversal: &D, use matching::StyleSharingResult::*; context.thread_local.statistics.elements_styled += 1; - let shared_context = context.shared; let kind = data.restyle_kind(); // First, try the style sharing cache. If we get a match we can skip the rest // of the work. if let MatchAndCascade = kind { let sharing_result = unsafe { - let cache = &mut context.thread_local.style_sharing_candidate_cache; - element.share_style_if_possible(cache, shared_context, &mut data) + element.share_style_if_possible(context, &mut data) }; if let StyleWasShared(index) = sharing_result { context.thread_local.statistics.styles_shared += 1; From d50946933d9974facff18612d2d586800fb3cd9e Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 11 Apr 2017 15:54:24 +0800 Subject: [PATCH 4/7] Set selector flags when matching elements against revalidation selectors. --- components/style/matching.rs | 42 +++++++++++++++++++++++++++++------- components/style/stylist.rs | 12 ++++++----- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 7375f2f3a2a..aa8492a26bf 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -108,7 +108,8 @@ fn element_matches_candidate(element: &E, candidate_element: &E, shared: &SharedStyleContext, bloom: &BloomFilter, - info: &mut CurrentElementInfo) + info: &mut CurrentElementInfo, + tasks: &mut Vec>) -> Result { macro_rules! miss { ($miss: ident) => { @@ -156,7 +157,8 @@ fn element_matches_candidate(element: &E, miss!(PresHints) } - if !revalidate(element, candidate, candidate_element, shared, bloom, info) { + if !revalidate(element, candidate, candidate_element, + shared, bloom, info, tasks) { miss!(Revalidation) } @@ -203,7 +205,8 @@ fn revalidate(element: &E, candidate_element: &E, shared: &SharedStyleContext, bloom: &BloomFilter, - info: &mut CurrentElementInfo) + info: &mut CurrentElementInfo, + tasks: &mut Vec>) -> bool { // NB: We could avoid matching ancestor selectors entirely (rather than // just depending on the bloom filter), at the expense of some complexity. @@ -217,13 +220,33 @@ fn revalidate(element: &E, let stylist = &shared.stylist; if info.revalidation_match_results.is_none() { + // It's important to set the selector flags. Otherwise, if we succeed in + // sharing the style, we may not set the slow selector flags for the + // right elements (which may not necessarily be |element|), causing missed + // restyles after future DOM mutations. + // + // Gecko's test_bug534804.html exercises this. A minimal testcase is: + // + // + // + // + // + // + // The style sharing cache will get a hit for the second span. When the + // child span is subsequently removed from the DOM, missing selector + // flags would cause us to miss the restyle on the second span. + let mut set_selector_flags = |el: &E, flags: ElementSelectorFlags| { + element.apply_selector_flags(tasks, el, flags); + }; info.revalidation_match_results = - Some(stylist.match_revalidation_selectors(element, bloom)); + Some(stylist.match_revalidation_selectors(element, bloom, + &mut set_selector_flags)); } if candidate.revalidation_match_results.is_none() { candidate.revalidation_match_results = - Some(stylist.match_revalidation_selectors(candidate_element, bloom)); + Some(stylist.match_revalidation_selectors(candidate_element, bloom, + &mut |_, _| {})); } let for_element = info.revalidation_match_results.as_ref().unwrap(); @@ -670,11 +693,12 @@ trait PrivateMatchMethods: TElement { candidate: &mut StyleSharingCandidate, shared: &SharedStyleContext, bloom: &BloomFilter, - info: &mut CurrentElementInfo) + info: &mut CurrentElementInfo, + tasks: &mut Vec>) -> Result { let candidate_element = *candidate.element; element_matches_candidate(self, candidate, &candidate_element, - shared, bloom, info) + shared, bloom, info, tasks) } } @@ -1024,13 +1048,15 @@ pub trait MatchMethods : TElement { let current_element_info = &mut context.thread_local.current_element_info.as_mut().unwrap(); let bloom = context.thread_local.bloom_filter.filter(); + let tasks = &mut context.thread_local.tasks; let mut should_clear_cache = false; for (i, candidate) in cache.iter_mut().enumerate() { let sharing_result = self.share_style_with_candidate_if_possible(candidate, &context.shared, bloom, - current_element_info); + current_element_info, + tasks); match sharing_result { Ok(shared_style) => { // Yay, cache hit. Share the style. diff --git a/components/style/stylist.rs b/components/style/stylist.rs index e0c8af2ba8b..3d29ac68c2f 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -768,11 +768,13 @@ impl Stylist { /// Computes the match results of a given element against the set of /// revalidation selectors. - pub fn match_revalidation_selectors(&self, - element: &E, - bloom: &BloomFilter) - -> BitVec + pub fn match_revalidation_selectors(&self, + element: &E, + bloom: &BloomFilter, + flags_setter: &mut F) + -> BitVec where E: TElement, + F: FnMut(&E, ElementSelectorFlags) { use selectors::matching::StyleRelations; use selectors::matching::matches_complex_selector; @@ -786,7 +788,7 @@ impl Stylist { element, Some(bloom), &mut StyleRelations::empty(), - &mut |_, _| {})); + flags_setter)); } results From cceaf7c619127808ee20c49e5ac1e7ce21c44be0 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 11 Apr 2017 18:42:36 +0800 Subject: [PATCH 5/7] Logging fixes. I had these lying around in the other bug. --- components/style/cache.rs | 5 +++++ components/style/matching.rs | 12 ++++++++++++ 2 files changed, 17 insertions(+) diff --git a/components/style/cache.rs b/components/style/cache.rs index a2432ee9fa7..2b12c8acf8f 100644 --- a/components/style/cache.rs +++ b/components/style/cache.rs @@ -31,6 +31,11 @@ impl LRUCache { } } + /// Returns the number of elements in the cache. + pub fn num_entries(&self) -> usize { + self.entries.len() + } + #[inline] /// Touch a given position, and put it in the last item on the list. pub fn touch(&mut self, pos: usize) { diff --git a/components/style/matching.rs b/components/style/matching.rs index aa8492a26bf..e7433fdc002 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -265,6 +265,11 @@ impl StyleSharingCandidateCache { } } + /// Returns the number of entries in the cache. + pub fn num_entries(&self) -> usize { + self.cache.num_entries() + } + fn iter_mut(&mut self) -> LRUCacheMutIterator> { self.cache.iter_mut() } @@ -1029,18 +1034,22 @@ pub trait MatchMethods : TElement { data: &mut AtomicRefMut) -> StyleSharingResult { if is_share_style_cache_disabled() { + debug!("{:?} Cannot share style: style sharing cache disabled", self); return StyleSharingResult::CannotShare } if self.parent_element().is_none() { + debug!("{:?} Cannot share style: element has style attribute", self); return StyleSharingResult::CannotShare } if self.style_attribute().is_some() { + debug!("{:?} Cannot share style: element has style attribute", self); return StyleSharingResult::CannotShare } if self.has_attr(&ns!(), &local_name!("id")) { + debug!("{:?} Cannot share style: element has id", self); return StyleSharingResult::CannotShare } @@ -1101,6 +1110,9 @@ pub trait MatchMethods : TElement { } } } + + debug!("{:?} Cannot share style: {} cache entries", self, cache.num_entries()); + if should_clear_cache { cache.clear(); } From fbdfbb1dfa7e88f4904b57f8a726c1e756f5c4bf Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Sun, 9 Apr 2017 18:29:17 +0800 Subject: [PATCH 6/7] Do a better job of detecting where there are transitions. The current code thinks that every element has transitions. --- components/style/matching.rs | 2 +- components/style/properties/gecko.mako.rs | 13 +++++++++++++ components/style/properties/properties.mako.rs | 6 ++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index e7433fdc002..0e03d1e2ea5 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -298,7 +298,7 @@ impl StyleSharingCandidateCache { } let box_style = style.get_box(); - if box_style.transition_property_count() > 0 { + if box_style.specifies_transitions() { debug!("Failing to insert to the cache: transitions"); return; } diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 63f4a5abbfa..c2eb70ea979 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -1890,6 +1890,19 @@ fn static_assert() { self.gecko.mTransitions[0].mProperty = nsCSSPropertyID_eCSSPropertyExtra_no_properties; } } + + /// Returns whether there are any transitions specified. + pub fn specifies_transitions(&self) -> bool { + use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_all_properties; + if self.gecko.mTransitionPropertyCount == 1 && + self.gecko.mTransitions[0].mProperty == eCSSPropertyExtra_all_properties && + self.gecko.mTransitions[0].mDuration.max(0.0) + self.gecko.mTransitions[0].mDelay <= 0.0f32 { + return false; + } + + self.gecko.mTransitionPropertyCount > 0 + } + pub fn transition_property_at(&self, index: usize) -> longhands::transition_property::computed_value::SingleComputedValue { self.gecko.mTransitions[index].mProperty.into() diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index a483a82b862..28bfc925cb7 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1509,6 +1509,12 @@ pub mod style_structs { pub fn specifies_animations(&self) -> bool { self.animation_name_iter().any(|name| name.0 != atom!("")) } + + /// Returns whether there are any transitions specified. + #[cfg(feature = "servo")] + pub fn specifies_transitions(&self) -> bool { + self.transition_property_count() > 0 + } % endif } From 90697ab8e622f919a11bb8b731b8b5a83b611961 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 12 Apr 2017 11:24:33 +0800 Subject: [PATCH 7/7] Update test expectation. --- .../wpt/metadata-css/css21_dev/html4/lang-selector-001.htm.ini | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 tests/wpt/metadata-css/css21_dev/html4/lang-selector-001.htm.ini diff --git a/tests/wpt/metadata-css/css21_dev/html4/lang-selector-001.htm.ini b/tests/wpt/metadata-css/css21_dev/html4/lang-selector-001.htm.ini deleted file mode 100644 index 2f8519669f6..00000000000 --- a/tests/wpt/metadata-css/css21_dev/html4/lang-selector-001.htm.ini +++ /dev/null @@ -1,3 +0,0 @@ -[lang-selector-001.htm] - type: reftest - expected: FAIL