From 3ebd48039ed810899322f54ff1d95051b1481681 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 29 May 2017 21:31:04 +0200 Subject: [PATCH 1/8] style: Allow StyleSource to be compared. --- components/style/rule_tree/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index 22a862cac5f..2557b4c77e5 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -61,6 +61,12 @@ pub enum StyleSource { Declarations(Arc>), } +impl PartialEq for StyleSource { + fn eq(&self, other: &Self) -> bool { + self.ptr_equals(other) + } +} + impl StyleSource { #[inline] fn ptr_equals(&self, other: &Self) -> bool { From 544e0f4cd6c45135c55770039b0b92054b92121c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 29 May 2017 21:38:21 +0200 Subject: [PATCH 2/8] style: Allow ApplicableDeclarationBlocks to be compared. We'll need this to cache pres hints, which generate these. --- components/style/stylist.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index d40a1b8de75..1689db9bd8d 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1350,7 +1350,7 @@ impl Rule { /// This represents the declarations in a given declaration block for a given /// importance. #[cfg_attr(feature = "servo", derive(HeapSizeOf))] -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub struct ApplicableDeclarationBlock { /// The style source, either a style rule, or a property declaration block. #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] From fb6339bbf9d9930e9db830feb50afac0b34070e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 29 May 2017 21:39:41 +0200 Subject: [PATCH 3/8] style: rewrap some comments to 80 columns for consistency with the rest of the code. --- components/style/context.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index 179e8250068..98122aa197d 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -154,13 +154,14 @@ impl<'a> SharedStyleContext<'a> { } } -/// Information about the current element being processed. We group this together -/// into a single struct within ThreadLocalStyleContext so that we can instantiate -/// and destroy it easily at the beginning and end of element processing. +/// Information about the current element being processed. We group this +/// together into a single struct within ThreadLocalStyleContext so that we can +/// instantiate and destroy it easily at the beginning and end of element +/// processing. pub struct CurrentElementInfo { - /// The element being processed. Currently we use an OpaqueNode since we only - /// use this for identity checks, but we could use SendElement if there were - /// a good reason to. + /// The element being processed. Currently we use an OpaqueNode since we + /// only use this for identity checks, but we could use SendElement if there + /// were a good reason to. element: OpaqueNode, /// Whether the element is being styled for the first time. is_initial_style: bool, @@ -172,8 +173,9 @@ pub struct CurrentElementInfo { pub possibly_expired_animations: Vec, } -/// Statistics gathered during the traversal. We gather statistics on each thread -/// and then combine them after the threads join via the Add implementation below. +/// Statistics gathered during the traversal. We gather statistics on each +/// thread and then combine them after the threads join via the Add +/// implementation below. #[derive(Default)] pub struct TraversalStatistics { /// The total number of elements traversed. From abcc9b301c37ada8bde4e333b7a07c2ee6324704 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 29 May 2017 21:56:21 +0200 Subject: [PATCH 4/8] style: Add a CachedStyleSharingData to hold the candidate class list and revalidation results. --- components/style/sharing/checks.rs | 17 +----- components/style/sharing/mod.rs | 85 ++++++++++++++++++++++++++---- 2 files changed, 78 insertions(+), 24 deletions(-) diff --git a/components/style/sharing/checks.rs b/components/style/sharing/checks.rs index 4ac537e0009..84ec5c5f0a5 100644 --- a/components/style/sharing/checks.rs +++ b/components/style/sharing/checks.rs @@ -72,13 +72,7 @@ pub fn have_same_class(element: E, let mut element_class_attributes = vec![]; element.each_class(|c| element_class_attributes.push(c.clone())); - if candidate.class_attributes.is_none() { - let mut attrs = vec![]; - candidate.element.each_class(|c| attrs.push(c.clone())); - candidate.class_attributes = Some(attrs) - } - - element_class_attributes == *candidate.class_attributes.as_ref().unwrap() + element_class_attributes == candidate.class_list() } /// Compare element and candidate state, but ignore visitedness. Styles don't @@ -137,15 +131,8 @@ pub fn revalidate(element: E, &mut set_selector_flags)); } - if candidate.revalidation_match_results.is_none() { - let results = - stylist.match_revalidation_selectors(&*candidate.element, bloom, - &mut |_, _| {}); - candidate.revalidation_match_results = Some(results); - } - let for_element = info.revalidation_match_results.as_ref().unwrap(); - let for_candidate = candidate.revalidation_match_results.as_ref().unwrap(); + let for_candidate = candidate.revalidation_match_results(stylist, bloom); // This assert "ensures", to some extent, that the two candidates have // matched the same rulehash buckets, and as such, that the bits we're diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 67fde2e047b..145ede1a23b 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -14,8 +14,10 @@ use dom::{TElement, SendElement}; use matching::{ChildCascadeRequirement, MatchMethods}; use properties::ComputedValues; use selectors::bloom::BloomFilter; -use selectors::matching::StyleRelations; +use selectors::matching::{ElementSelectorFlags, StyleRelations}; use sink::ForgetfulSink; +use smallvec::SmallVec; +use stylist::Stylist; mod checks; @@ -32,23 +34,86 @@ pub enum StyleSharingBehavior { Disallow, } +/// Some data we want to avoid recomputing all the time while trying to share +/// style. +#[derive(Debug)] +pub struct CachedStyleSharingData { + /// The class list of this element. + /// + /// TODO(emilio): See if it's worth to sort them, or doing something else in + /// a similar fashion as what Boris is doing for the ID attribute. + class_list: Option>, + + /// The cached result of matching this entry against the revalidation + /// selectors. + revalidation_match_results: Option, +} + +impl CachedStyleSharingData { + /// Get or compute the class-list associated with this element. + pub fn class_list(&mut self, element: E) -> &[Atom] + where E: TElement, + { + if self.class_list.is_none() { + let mut class_list = SmallVec::new(); + element.each_class(|c| class_list.push(c.clone())); + self.class_list = Some(class_list); + } + &*self.class_list.as_ref().unwrap() + } + + /// Computes the revalidation results if needed, and returns it. + fn revalidation_match_results( + &mut self, + element: E, + stylist: &Stylist, + bloom: &BloomFilter, + flags_setter: &mut F + ) -> &BitVec + where E: TElement, + F: FnMut(&E, ElementSelectorFlags), + { + if self.revalidation_match_results.is_none() { + self.revalidation_match_results = + Some(stylist.match_revalidation_selectors(&element, bloom, + flags_setter)); + } + + self.revalidation_match_results.as_ref().unwrap() + } +} + /// Information regarding a style sharing candidate, that is, an entry in the /// style sharing cache. /// /// Note that this information is stored in TLS and cleared after the traversal, /// and once here, the style information of the element is immutable, so it's /// safe to access. -/// -/// TODO: We can stick a lot more info here. #[derive(Debug)] pub struct StyleSharingCandidate { /// The element. We use SendElement here so that the cache may live in /// ScopedTLS. element: SendElement, - /// The cached class names. - class_attributes: Option>, - /// The cached result of matching this entry against the revalidation selectors. - revalidation_match_results: Option, + cache: CachedStyleSharingData, +} + +impl StyleSharingCandidate { + /// Get the classlist of this candidate. + fn class_list(&mut self) -> &[Atom] { + self.cache.class_list(*self.element) + } + + /// Get the classlist of this candidate. + fn revalidation_match_results( + &mut self, + stylist: &Stylist, + bloom: &BloomFilter, + ) -> &BitVec { + self.cache.revalidation_match_results(*self.element, + stylist, + bloom, + &mut |_, _| {}) + } } impl PartialEq> for StyleSharingCandidate { @@ -177,8 +242,10 @@ impl StyleSharingCandidateCache { self.cache.insert(StyleSharingCandidate { element: unsafe { SendElement::new(*element) }, - class_attributes: None, - revalidation_match_results: revalidation_match_results, + cache: CachedStyleSharingData { + class_list: None, + revalidation_match_results: revalidation_match_results, + } }); } From 03952a0c27311e956748234389657ef637b22e2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 29 May 2017 22:54:17 +0200 Subject: [PATCH 5/8] style: Also cache the class list in the CurrentElementInfo. This patch also removes all notion of style sharing from matching.rs, which is nice. --- components/style/context.rs | 10 +- components/style/matching.rs | 28 +---- components/style/sharing/checks.rs | 44 ++----- components/style/sharing/mod.rs | 184 ++++++++++++++++++++++------- components/style/traversal.rs | 8 +- 5 files changed, 160 insertions(+), 114 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index 98122aa197d..ee50e953980 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -7,7 +7,6 @@ #[cfg(feature = "servo")] use animation::Animation; use animation::PropertyAnimation; use app_units::Au; -use bit_vec::BitVec; use bloom::StyleBloom; use cache::LRUCache; use data::ElementData; @@ -23,7 +22,7 @@ use selector_parser::SnapshotMap; use selectors::matching::ElementSelectorFlags; #[cfg(feature = "servo")] use servo_config::opts; use shared_lock::StylesheetGuards; -use sharing::StyleSharingCandidateCache; +use sharing::{CachedStyleSharingData, StyleSharingCandidateCache}; #[cfg(feature = "servo")] use std::collections::HashMap; #[cfg(feature = "gecko")] use std::env; use std::fmt; @@ -165,9 +164,8 @@ 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, + /// Lazy cache of the different data used for style sharing. + pub cached_style_sharing_data: CachedStyleSharingData, /// A Vec of possibly expired animations. Used only by Servo. #[allow(dead_code)] pub possibly_expired_animations: Vec, @@ -465,7 +463,7 @@ impl ThreadLocalStyleContext { self.current_element_info = Some(CurrentElementInfo { element: element.as_node().opaque(), is_initial_style: !data.has_styles(), - revalidation_match_results: None, + cached_style_sharing_data: CachedStyleSharingData::new(), possibly_expired_animations: Vec::new(), }); } diff --git a/components/style/matching.rs b/components/style/matching.rs index 25349b5457f..c741a949df2 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -23,7 +23,7 @@ use rule_tree::{CascadeLevel, StrongRuleNode}; use selector_parser::{PseudoElement, RestyleDamage, SelectorImpl}; use selectors::matching::{ElementSelectorFlags, MatchingContext, MatchingMode, StyleRelations}; use selectors::matching::{VisitedHandlingMode, AFFECTED_BY_PSEUDO_ELEMENTS}; -use sharing::{StyleSharingBehavior, StyleSharingResult}; +use sharing::StyleSharingBehavior; use stylearc::Arc; use stylist::{ApplicableDeclarationList, RuleInclusion}; @@ -858,7 +858,7 @@ pub trait MatchMethods : TElement { let revalidation_match_results = context.thread_local .current_element_info .as_mut().unwrap() - .revalidation_match_results + .cached_style_sharing_data .take(); context.thread_local .style_sharing_candidate_cache @@ -1341,30 +1341,6 @@ pub trait MatchMethods : TElement { false } - /// Attempts to share a style with another node. This method is unsafe - /// because it depends on the `style_sharing_candidate_cache` having only - /// 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, - context: &mut StyleContext, - data: &mut ElementData) - -> StyleSharingResult { - let shared_context = &context.shared; - let current_element_info = - context.thread_local.current_element_info.as_mut().unwrap(); - let selector_flags_map = &mut context.thread_local.selector_flags; - let bloom_filter = context.thread_local.bloom_filter.filter(); - - context.thread_local - .style_sharing_candidate_cache - .share_style_if_possible(shared_context, - current_element_info, - selector_flags_map, - bloom_filter, - *self, - data) - } - /// Given the old and new style of this element, and whether it's a /// pseudo-element, compute the restyle damage used to determine which /// kind of layout or painting operations we'll need. diff --git a/components/style/sharing/checks.rs b/components/style/sharing/checks.rs index 84ec5c5f0a5..6fad78fcb7e 100644 --- a/components/style/sharing/checks.rs +++ b/components/style/sharing/checks.rs @@ -6,13 +6,12 @@ //! quickly whether it's worth to share style, and whether two different //! elements can indeed share the same style. -use context::{CurrentElementInfo, SelectorFlagsMap, SharedStyleContext}; +use context::{SelectorFlagsMap, SharedStyleContext}; use dom::TElement; use element_state::*; -use matching::MatchMethods; use selectors::bloom::BloomFilter; -use selectors::matching::{ElementSelectorFlags, StyleRelations}; -use sharing::StyleSharingCandidate; +use selectors::matching::StyleRelations; +use sharing::{StyleSharingCandidate, StyleSharingTarget}; use sink::ForgetfulSink; use stylearc::Arc; @@ -63,16 +62,12 @@ pub fn has_presentational_hints(element: E) -> bool /// Whether a given element has the same class attribute than a given candidate. /// /// We don't try to share style across elements with different class attributes. -pub fn have_same_class(element: E, +pub fn have_same_class(target: &mut StyleSharingTarget, candidate: &mut StyleSharingCandidate) -> bool where E: TElement, { - // XXX Efficiency here, I'm only validating ideas. - let mut element_class_attributes = vec![]; - element.each_class(|c| element_class_attributes.push(c.clone())); - - element_class_attributes == candidate.class_list() + target.class_list() == candidate.class_list() } /// Compare element and candidate state, but ignore visitedness. Styles don't @@ -96,42 +91,19 @@ pub fn have_same_state_ignoring_visitedness(element: E, /// :first-child, etc, or on attributes that we don't check off-hand (pretty /// much every attribute selector except `id` and `class`. #[inline] -pub fn revalidate(element: E, +pub fn revalidate(target: &mut StyleSharingTarget, candidate: &mut StyleSharingCandidate, shared_context: &SharedStyleContext, bloom: &BloomFilter, - info: &mut CurrentElementInfo, selector_flags_map: &mut SelectorFlagsMap) -> bool where E: TElement, { let stylist = &shared_context.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(selector_flags_map, el, flags); - }; - info.revalidation_match_results = - Some(stylist.match_revalidation_selectors(&element, bloom, - &mut set_selector_flags)); - } + let for_element = + target.revalidation_match_results(stylist, bloom, selector_flags_map); - let for_element = info.revalidation_match_results.as_ref().unwrap(); let for_candidate = candidate.revalidation_match_results(stylist, bloom); // This assert "ensures", to some extent, that the two candidates have diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 145ede1a23b..c850ee24605 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -8,7 +8,7 @@ use Atom; use bit_vec::BitVec; use cache::{LRUCache, LRUCacheMutIterator}; -use context::{CurrentElementInfo, SelectorFlagsMap, SharedStyleContext}; +use context::{SelectorFlagsMap, SharedStyleContext, StyleContext}; use data::{ComputedStyle, ElementData, ElementStyles}; use dom::{TElement, SendElement}; use matching::{ChildCascadeRequirement, MatchMethods}; @@ -17,6 +17,7 @@ use selectors::bloom::BloomFilter; use selectors::matching::{ElementSelectorFlags, StyleRelations}; use sink::ForgetfulSink; use smallvec::SmallVec; +use std::ops::Deref; use stylist::Stylist; mod checks; @@ -50,6 +51,23 @@ pub struct CachedStyleSharingData { } impl CachedStyleSharingData { + /// Trivially construct an empty `CachedStyleSharingData` with nothing on + /// it. + pub fn new() -> Self { + Self { + class_list: None, + revalidation_match_results: None, + } + } + + /// Move the cached data to a new instance, and return it. + pub fn take(&mut self) -> Self { + Self { + class_list: self.class_list.take(), + revalidation_match_results: self.revalidation_match_results.take(), + } + } + /// Get or compute the class-list associated with this element. pub fn class_list(&mut self, element: E) -> &[Atom] where E: TElement, @@ -122,6 +140,97 @@ impl PartialEq> for StyleSharingCandidate< } } +/// An element we want to test against the style sharing cache. +pub struct StyleSharingTarget { + element: E, + cache: CachedStyleSharingData, +} + +impl Deref for StyleSharingTarget { + type Target = E; + + fn deref(&self) -> &Self::Target { + &self.element + } +} + +impl StyleSharingTarget { + /// Trivially construct a new StyleSharingTarget to test against the cache. + pub fn new(element: E) -> Self { + Self { + element: element, + cache: CachedStyleSharingData::new(), + } + } + + fn class_list(&mut self) -> &[Atom] { + self.cache.class_list(self.element) + } + + fn revalidation_match_results( + &mut self, + stylist: &Stylist, + bloom: &BloomFilter, + selector_flags_map: &mut SelectorFlagsMap + ) -> &BitVec { + // 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 element = self.element; + let mut set_selector_flags = |el: &E, flags: ElementSelectorFlags| { + element.apply_selector_flags(selector_flags_map, el, flags); + }; + + self.cache.revalidation_match_results(self.element, + stylist, + bloom, + &mut set_selector_flags) + } + + /// Attempts to share a style with another node. + pub fn share_style_if_possible( + mut self, + context: &mut StyleContext, + data: &mut ElementData) + -> StyleSharingResult + { + use std::mem; + + let shared_context = &context.shared; + let selector_flags_map = &mut context.thread_local.selector_flags; + let bloom_filter = context.thread_local.bloom_filter.filter(); + + let result = context.thread_local + .style_sharing_candidate_cache + .share_style_if_possible(shared_context, + selector_flags_map, + bloom_filter, + &mut self, + data); + + // FIXME(emilio): Do this in a cleaner way. + mem::swap(&mut self.cache, + &mut context + .thread_local + .current_element_info.as_mut().unwrap() + .cached_style_sharing_data); + + result + } +} + /// A cache miss result. #[derive(Clone, Debug)] pub enum CacheMiss { @@ -198,7 +307,7 @@ impl StyleSharingCandidateCache { element: &E, style: &ComputedValues, relations: StyleRelations, - revalidation_match_results: Option) { + cache: CachedStyleSharingData) { let parent = match element.parent_element() { Some(element) => element, None => { @@ -242,10 +351,7 @@ impl StyleSharingCandidateCache { self.cache.insert(StyleSharingCandidate { element: unsafe { SendElement::new(*element) }, - cache: CachedStyleSharingData { - class_list: None, - revalidation_match_results: revalidation_match_results, - } + cache: cache, }); } @@ -260,43 +366,39 @@ impl StyleSharingCandidateCache { } /// Attempts to share a style with another node. - /// - /// This method is unsafe because it depends on the - /// `style_sharing_candidate_cache` having only live nodes in it, and we - /// have no way to guarantee that at the type system level yet. - pub unsafe fn share_style_if_possible( + fn share_style_if_possible( &mut self, shared_context: &SharedStyleContext, - current_element_info: &mut CurrentElementInfo, selector_flags_map: &mut SelectorFlagsMap, bloom_filter: &BloomFilter, - element: E, + target: &mut StyleSharingTarget, data: &mut ElementData ) -> StyleSharingResult { if shared_context.options.disable_style_sharing_cache { debug!("{:?} Cannot share style: style sharing cache disabled", - element); + target.element); return StyleSharingResult::CannotShare } - if element.parent_element().is_none() { - debug!("{:?} Cannot share style: element has no parent", element); + if target.parent_element().is_none() { + debug!("{:?} Cannot share style: element has no parent", + target.element); return StyleSharingResult::CannotShare } - if element.is_native_anonymous() { - debug!("{:?} Cannot share style: NAC", element); + if target.is_native_anonymous() { + debug!("{:?} Cannot share style: NAC", target.element); return StyleSharingResult::CannotShare; } - if element.style_attribute().is_some() { + if target.style_attribute().is_some() { debug!("{:?} Cannot share style: element has style attribute", - element); + target.element); return StyleSharingResult::CannotShare } - if element.get_id().is_some() { - debug!("{:?} Cannot share style: element has id", element); + if target.get_id().is_some() { + debug!("{:?} Cannot share style: element has id", target.element); return StyleSharingResult::CannotShare } @@ -304,11 +406,10 @@ impl StyleSharingCandidateCache { for (i, candidate) in self.iter_mut().enumerate() { let sharing_result = Self::test_candidate( - element, + target, candidate, &shared_context, bloom_filter, - current_element_info, selector_flags_map ); @@ -321,7 +422,7 @@ impl StyleSharingCandidateCache { let old_values = data.get_styles_mut() .and_then(|s| s.primary.values.take()); let child_cascade_requirement = - element.accumulate_damage( + target.accumulate_damage( &shared_context, data.get_restyle_mut(), old_values.as_ref().map(|v| &**v), @@ -360,7 +461,7 @@ impl StyleSharingCandidateCache { } } - debug!("{:?} Cannot share style: {} cache entries", element, + debug!("{:?} Cannot share style: {} cache entries", target.element, self.cache.num_entries()); if should_clear_cache { @@ -370,11 +471,10 @@ impl StyleSharingCandidateCache { StyleSharingResult::CannotShare } - fn test_candidate(element: E, + fn test_candidate(target: &mut StyleSharingTarget, candidate: &mut StyleSharingCandidate, shared: &SharedStyleContext, bloom: &BloomFilter, - info: &mut CurrentElementInfo, selector_flags_map: &mut SelectorFlagsMap) -> Result { macro_rules! miss { @@ -386,66 +486,66 @@ impl StyleSharingCandidateCache { // Check that we have the same parent, or at least the same pointer // identity for parent computed style. The latter check allows us to // share style between cousins if the parents shared style. - let parent = element.parent_element(); + let parent = target.parent_element(); let candidate_parent = candidate.element.parent_element(); if parent != candidate_parent && !checks::same_computed_values(parent, candidate_parent) { miss!(Parent) } - if element.is_native_anonymous() { + if target.is_native_anonymous() { debug_assert!(!candidate.element.is_native_anonymous(), "Why inserting NAC into the cache?"); miss!(NativeAnonymousContent) } - if *element.get_local_name() != *candidate.element.get_local_name() { + if *target.get_local_name() != *candidate.element.get_local_name() { miss!(LocalName) } - if *element.get_namespace() != *candidate.element.get_namespace() { + if *target.get_namespace() != *candidate.element.get_namespace() { miss!(Namespace) } - if element.is_link() != candidate.element.is_link() { + if target.is_link() != candidate.element.is_link() { miss!(Link) } - if element.matches_user_and_author_rules() != + if target.matches_user_and_author_rules() != candidate.element.matches_user_and_author_rules() { miss!(UserAndAuthorRules) } - if !checks::have_same_state_ignoring_visitedness(element, candidate) { + if !checks::have_same_state_ignoring_visitedness(target.element, candidate) { miss!(State) } - if element.get_id() != candidate.element.get_id() { + if target.get_id() != candidate.element.get_id() { miss!(IdAttr) } - if element.style_attribute().is_some() { + if target.style_attribute().is_some() { miss!(StyleAttr) } - if !checks::have_same_class(element, candidate) { + if !checks::have_same_class(target, candidate) { miss!(Class) } - if checks::has_presentational_hints(element) { + if checks::has_presentational_hints(target.element) { miss!(PresHints) } - if !checks::revalidate(element, candidate, shared, bloom, info, + if !checks::revalidate(target, candidate, shared, bloom, selector_flags_map) { miss!(Revalidation) } let data = candidate.element.borrow_data().unwrap(); - debug_assert!(element.has_current_styles(&data)); + debug_assert!(target.has_current_styles(&data)); debug!("Sharing style between {:?} and {:?}", - element, candidate.element); + target.element, candidate.element); Ok(data.styles().primary.clone()) } } diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 60df72ffeba..ecefa15278a 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -11,7 +11,7 @@ use dom::{DirtyDescendants, NodeInfo, OpaqueNode, TElement, TNode}; use matching::{ChildCascadeRequirement, MatchMethods}; use restyle_hints::{HintComputationContext, RestyleHint}; use selector_parser::RestyleDamage; -use sharing::StyleSharingBehavior; +use sharing::{StyleSharingBehavior, StyleSharingTarget}; #[cfg(feature = "servo")] use servo_config::opts; use smallvec::SmallVec; use std::borrow::BorrowMut; @@ -795,9 +795,9 @@ fn compute_style(_traversal: &D, // 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 { - element.share_style_if_possible(context, data) - }; + let target = StyleSharingTarget::new(element); + let sharing_result = target.share_style_if_possible(context, data); + if let StyleWasShared(index, had_damage) = sharing_result { context.thread_local.statistics.styles_shared += 1; context.thread_local.style_sharing_candidate_cache.touch(index); From 10bd5636dcf665bccfb65956eca3db5bdaaba0b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 29 May 2017 23:12:44 +0200 Subject: [PATCH 6/8] style: Allow sharing style for elements with presentational hints. --- components/style/sharing/checks.rs | 13 ++++---- components/style/sharing/mod.rs | 52 +++++++++++++++++++++++------- 2 files changed, 46 insertions(+), 19 deletions(-) diff --git a/components/style/sharing/checks.rs b/components/style/sharing/checks.rs index 6fad78fcb7e..d773afc2bd4 100644 --- a/components/style/sharing/checks.rs +++ b/components/style/sharing/checks.rs @@ -12,7 +12,6 @@ use element_state::*; use selectors::bloom::BloomFilter; use selectors::matching::StyleRelations; use sharing::{StyleSharingCandidate, StyleSharingTarget}; -use sink::ForgetfulSink; use stylearc::Arc; /// Determines, based on the results of selector matching, whether it's worth to @@ -23,8 +22,7 @@ pub fn relations_are_shareable(relations: &StyleRelations) -> bool { use selectors::matching::*; !relations.intersects(AFFECTED_BY_ID_SELECTOR | AFFECTED_BY_PSEUDO_ELEMENTS | - AFFECTED_BY_STYLE_ATTRIBUTE | - AFFECTED_BY_PRESENTATIONAL_HINTS) + AFFECTED_BY_STYLE_ATTRIBUTE) } /// Whether, given two elements, they have pointer-equal computed values. @@ -51,12 +49,13 @@ pub fn same_computed_values(first: Option, second: Option) -> bool /// We consider not worth to share style with an element that has presentational /// hints, both because implementing the code that compares that the hints are /// equal is somewhat annoying, and also because it'd be expensive enough. -pub fn has_presentational_hints(element: E) -> bool +pub fn have_same_presentational_hints( + target: &mut StyleSharingTarget, + candidate: &mut StyleSharingCandidate +) -> bool where E: TElement, { - let mut hints = ForgetfulSink::new(); - element.synthesize_presentational_hints_for_legacy_attributes(&mut hints); - !hints.is_empty() + target.pres_hints() == candidate.pres_hints() } /// Whether a given element has the same class attribute than a given candidate. diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index c850ee24605..c6fe9a078a8 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -15,10 +15,9 @@ use matching::{ChildCascadeRequirement, MatchMethods}; use properties::ComputedValues; use selectors::bloom::BloomFilter; use selectors::matching::{ElementSelectorFlags, StyleRelations}; -use sink::ForgetfulSink; use smallvec::SmallVec; use std::ops::Deref; -use stylist::Stylist; +use stylist::{ApplicableDeclarationBlock, Stylist}; mod checks; @@ -45,6 +44,9 @@ pub struct CachedStyleSharingData { /// a similar fashion as what Boris is doing for the ID attribute. class_list: Option>, + /// The list of presentational attributes of the element. + pres_hints: Option>, + /// The cached result of matching this entry against the revalidation /// selectors. revalidation_match_results: Option, @@ -56,6 +58,7 @@ impl CachedStyleSharingData { pub fn new() -> Self { Self { class_list: None, + pres_hints: None, revalidation_match_results: None, } } @@ -64,10 +67,24 @@ impl CachedStyleSharingData { pub fn take(&mut self) -> Self { Self { class_list: self.class_list.take(), + pres_hints: self.pres_hints.take(), revalidation_match_results: self.revalidation_match_results.take(), } } + /// Get or compute the list of presentational attributes associated with + /// this element. + pub fn pres_hints(&mut self, element: E) -> &[ApplicableDeclarationBlock] + where E: TElement, + { + if self.pres_hints.is_none() { + let mut pres_hints = SmallVec::new(); + element.synthesize_presentational_hints_for_legacy_attributes(&mut pres_hints); + self.pres_hints = Some(pres_hints); + } + &*self.pres_hints.as_ref().unwrap() + } + /// Get or compute the class-list associated with this element. pub fn class_list(&mut self, element: E) -> &[Atom] where E: TElement, @@ -121,6 +138,11 @@ impl StyleSharingCandidate { self.cache.class_list(*self.element) } + /// Get the pres hints of this candidate. + fn pres_hints(&mut self) -> &[ApplicableDeclarationBlock] { + self.cache.pres_hints(*self.element) + } + /// Get the classlist of this candidate. fn revalidation_match_results( &mut self, @@ -167,6 +189,11 @@ impl StyleSharingTarget { self.cache.class_list(self.element) } + /// Get the pres hints of this candidate. + fn pres_hints(&mut self) -> &[ApplicableDeclarationBlock] { + self.cache.pres_hints(self.element) + } + fn revalidation_match_results( &mut self, stylist: &Stylist, @@ -307,7 +334,9 @@ impl StyleSharingCandidateCache { element: &E, style: &ComputedValues, relations: StyleRelations, - cache: CachedStyleSharingData) { + mut cache: CachedStyleSharingData) { + use selectors::matching::AFFECTED_BY_PRESENTATIONAL_HINTS; + let parent = match element.parent_element() { Some(element) => element, None => { @@ -328,14 +357,6 @@ impl StyleSharingCandidateCache { return; } - // Make sure we noted any presentational hints in the StyleRelations. - if cfg!(debug_assertions) { - let mut hints = ForgetfulSink::new(); - element.synthesize_presentational_hints_for_legacy_attributes(&mut hints); - debug_assert!(hints.is_empty(), - "Style relations should not be shareable!"); - } - let box_style = style.get_box(); if box_style.specifies_transitions() { debug!("Failing to insert to the cache: transitions"); @@ -347,6 +368,13 @@ impl StyleSharingCandidateCache { return; } + // Take advantage of the information we've learned during + // selector-matching. + if !relations.intersects(AFFECTED_BY_PRESENTATIONAL_HINTS) { + debug_assert!(cache.pres_hints.as_ref().map_or(true, |v| v.is_empty())); + cache.pres_hints = Some(SmallVec::new()); + } + debug!("Inserting into cache: {:?} with parent {:?}", element, parent); self.cache.insert(StyleSharingCandidate { @@ -532,7 +560,7 @@ impl StyleSharingCandidateCache { miss!(Class) } - if checks::has_presentational_hints(target.element) { + if !checks::have_same_presentational_hints(target, candidate) { miss!(PresHints) } From 0adc02a317364166d71bd7374857a6bada5e7c90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 29 May 2017 23:24:06 +0200 Subject: [PATCH 7/8] style: Avoid some cfg checks in context.rs --- components/layout/animation.rs | 8 ++++---- components/layout_thread/lib.rs | 18 ++++++++---------- components/style/context.rs | 10 +++++----- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/components/layout/animation.rs b/components/layout/animation.rs index a054927190c..5d7a1a87acc 100644 --- a/components/layout/animation.rs +++ b/components/layout/animation.rs @@ -6,13 +6,13 @@ use context::LayoutContext; use flow::{self, Flow}; +use fnv::FnvHashMap; use gfx::display_list::OpaqueNode; use ipc_channel::ipc::IpcSender; use msg::constellation_msg::PipelineId; use opaque_node::OpaqueNodeMethods; use script_traits::{AnimationState, ConstellationControlMsg, LayoutMsg as ConstellationMsg}; use script_traits::UntrustedNodeAddress; -use std::collections::HashMap; use std::sync::mpsc::Receiver; use style::animation::{Animation, update_style_for_animation}; use style::font_metrics::ServoMetricsProvider; @@ -24,8 +24,8 @@ use style::timer::Timer; /// `expired_animations`. pub fn update_animation_state(constellation_chan: &IpcSender, script_chan: &IpcSender, - running_animations: &mut HashMap>, - expired_animations: &mut HashMap>, + running_animations: &mut FnvHashMap>, + expired_animations: &mut FnvHashMap>, mut newly_transitioning_nodes: Option<&mut Vec>, new_animations_receiver: &Receiver, pipeline_id: PipelineId, @@ -149,7 +149,7 @@ pub fn update_animation_state(constellation_chan: &IpcSender, // this should be made generic. pub fn recalc_style_for_animations(context: &LayoutContext, flow: &mut Flow, - animations: &HashMap>) { let mut damage = RestyleDamage::empty(); flow.mutate_fragments(&mut |fragment| { diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 32cf5dc23a3..9ddff21070e 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -44,7 +44,7 @@ use euclid::point::Point2D; use euclid::rect::Rect; use euclid::scale_factor::ScaleFactor; use euclid::size::Size2D; -use fnv::FnvHasher; +use fnv::FnvHashMap; use gfx::display_list::{OpaqueNode, WebRenderImageInfo}; use gfx::font; use gfx::font_cache_thread::FontCacheThread; @@ -99,7 +99,6 @@ use servo_url::ServoUrl; use std::borrow::ToOwned; use std::cell::{Cell, RefCell}; use std::collections::HashMap; -use std::hash::BuildHasherDefault; use std::marker::PhantomData; use std::mem as std_mem; use std::ops::{Deref, DerefMut}; @@ -203,10 +202,10 @@ pub struct LayoutThread { document_shared_lock: Option, /// The list of currently-running animations. - running_animations: StyleArc>>>, + running_animations: StyleArc>>>, /// The list of animations that have expired since the last style recalculation. - expired_animations: StyleArc>>>, + expired_animations: StyleArc>>>, /// A counter for epoch messages epoch: Cell, @@ -224,9 +223,8 @@ pub struct LayoutThread { /// The CSS error reporter for all CSS loaded in this layout thread error_reporter: CSSErrorReporter, - webrender_image_cache: Arc>>>, + webrender_image_cache: Arc>>, /// Webrender interface. webrender_api: webrender_traits::RenderApi, @@ -492,8 +490,8 @@ impl LayoutThread { outstanding_web_fonts: outstanding_web_fonts_counter, root_flow: RefCell::new(None), document_shared_lock: None, - running_animations: StyleArc::new(RwLock::new(HashMap::new())), - expired_animations: StyleArc::new(RwLock::new(HashMap::new())), + running_animations: StyleArc::new(RwLock::new(FnvHashMap::default())), + expired_animations: StyleArc::new(RwLock::new(FnvHashMap::default())), epoch: Cell::new(Epoch(0)), viewport_size: Size2D::new(Au(0), Au(0)), webrender_api: webrender_api_sender.create_api(), @@ -521,7 +519,7 @@ impl LayoutThread { script_chan: Arc::new(Mutex::new(script_chan)), }, webrender_image_cache: - Arc::new(RwLock::new(HashMap::with_hasher(Default::default()))), + Arc::new(RwLock::new(FnvHashMap::default())), timer: if PREFS.get("layout.animations.test.enabled") .as_boolean().unwrap_or(false) { diff --git a/components/style/context.rs b/components/style/context.rs index ee50e953980..1214040dcf4 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -20,11 +20,8 @@ use font_metrics::FontMetricsProvider; #[cfg(feature = "gecko")] use properties::ComputedValues; use selector_parser::SnapshotMap; use selectors::matching::ElementSelectorFlags; -#[cfg(feature = "servo")] use servo_config::opts; use shared_lock::StylesheetGuards; use sharing::{CachedStyleSharingData, StyleSharingCandidateCache}; -#[cfg(feature = "servo")] use std::collections::HashMap; -#[cfg(feature = "gecko")] use std::env; use std::fmt; use std::ops::Add; #[cfg(feature = "servo")] use std::sync::Mutex; @@ -78,6 +75,7 @@ pub struct StyleSystemOptions { #[cfg(feature = "gecko")] fn get_env(name: &str) -> bool { + use std::env; match env::var(name) { Ok(s) => !s.is_empty(), Err(_) => false, @@ -87,6 +85,8 @@ fn get_env(name: &str) -> bool { impl Default for StyleSystemOptions { #[cfg(feature = "servo")] fn default() -> Self { + use servo_config::opts; + StyleSystemOptions { disable_style_sharing_cache: opts::get().disable_share_style_cache, dump_style_statistics: opts::get().style_sharing_stats, @@ -134,11 +134,11 @@ pub struct SharedStyleContext<'a> { /// The animations that are currently running. #[cfg(feature = "servo")] - pub running_animations: Arc>>>, + pub running_animations: Arc>>>, /// The list of animations that have expired since the last style recalculation. #[cfg(feature = "servo")] - pub expired_animations: Arc>>>, + pub expired_animations: Arc>>>, /// Data needed to create the thread-local style context from the shared one. #[cfg(feature = "servo")] From 7db27763488b5fe9661ae9ec52f497c89db16905 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 30 May 2017 11:23:59 +0200 Subject: [PATCH 8/8] s/CachedStyleSharingData/ValidationData. I still think CachedStyleSharingData should be the name, but not going to fight over it. --- components/style/context.rs | 6 ++-- components/style/matching.rs | 14 +++++---- components/style/sharing/mod.rs | 51 +++++++++++++++++---------------- 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index 1214040dcf4..ef19d45b6d9 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -21,7 +21,7 @@ use font_metrics::FontMetricsProvider; use selector_parser::SnapshotMap; use selectors::matching::ElementSelectorFlags; use shared_lock::StylesheetGuards; -use sharing::{CachedStyleSharingData, StyleSharingCandidateCache}; +use sharing::{ValidationData, StyleSharingCandidateCache}; use std::fmt; use std::ops::Add; #[cfg(feature = "servo")] use std::sync::Mutex; @@ -165,7 +165,7 @@ pub struct CurrentElementInfo { /// Whether the element is being styled for the first time. is_initial_style: bool, /// Lazy cache of the different data used for style sharing. - pub cached_style_sharing_data: CachedStyleSharingData, + pub validation_data: ValidationData, /// A Vec of possibly expired animations. Used only by Servo. #[allow(dead_code)] pub possibly_expired_animations: Vec, @@ -463,7 +463,7 @@ impl ThreadLocalStyleContext { self.current_element_info = Some(CurrentElementInfo { element: element.as_node().opaque(), is_initial_style: !data.has_styles(), - cached_style_sharing_data: CachedStyleSharingData::new(), + validation_data: ValidationData::new(), possibly_expired_animations: Vec::new(), }); } diff --git a/components/style/matching.rs b/components/style/matching.rs index c741a949df2..c811fef3afa 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -855,17 +855,19 @@ pub trait MatchMethods : TElement { // // 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() - .cached_style_sharing_data - .take(); + let validation_data = + context.thread_local + .current_element_info + .as_mut().unwrap() + .validation_data + .take(); + context.thread_local .style_sharing_candidate_cache .insert_if_possible(self, data.styles().primary.values(), primary_results.relations, - revalidation_match_results); + validation_data); } child_cascade_requirement diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index c6fe9a078a8..7eaf0322d6b 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -37,7 +37,7 @@ pub enum StyleSharingBehavior { /// Some data we want to avoid recomputing all the time while trying to share /// style. #[derive(Debug)] -pub struct CachedStyleSharingData { +pub struct ValidationData { /// The class list of this element. /// /// TODO(emilio): See if it's worth to sort them, or doing something else in @@ -52,8 +52,8 @@ pub struct CachedStyleSharingData { revalidation_match_results: Option, } -impl CachedStyleSharingData { - /// Trivially construct an empty `CachedStyleSharingData` with nothing on +impl ValidationData { + /// Trivially construct an empty `ValidationData` with nothing on /// it. pub fn new() -> Self { Self { @@ -129,18 +129,18 @@ pub struct StyleSharingCandidate { /// The element. We use SendElement here so that the cache may live in /// ScopedTLS. element: SendElement, - cache: CachedStyleSharingData, + validation_data: ValidationData, } impl StyleSharingCandidate { /// Get the classlist of this candidate. fn class_list(&mut self) -> &[Atom] { - self.cache.class_list(*self.element) + self.validation_data.class_list(*self.element) } /// Get the pres hints of this candidate. fn pres_hints(&mut self) -> &[ApplicableDeclarationBlock] { - self.cache.pres_hints(*self.element) + self.validation_data.pres_hints(*self.element) } /// Get the classlist of this candidate. @@ -149,10 +149,11 @@ impl StyleSharingCandidate { stylist: &Stylist, bloom: &BloomFilter, ) -> &BitVec { - self.cache.revalidation_match_results(*self.element, - stylist, - bloom, - &mut |_, _| {}) + self.validation_data.revalidation_match_results( + *self.element, + stylist, + bloom, + &mut |_, _| {}) } } @@ -165,7 +166,7 @@ impl PartialEq> for StyleSharingCandidate< /// An element we want to test against the style sharing cache. pub struct StyleSharingTarget { element: E, - cache: CachedStyleSharingData, + validation_data: ValidationData, } impl Deref for StyleSharingTarget { @@ -181,17 +182,17 @@ impl StyleSharingTarget { pub fn new(element: E) -> Self { Self { element: element, - cache: CachedStyleSharingData::new(), + validation_data: ValidationData::new(), } } fn class_list(&mut self) -> &[Atom] { - self.cache.class_list(self.element) + self.validation_data.class_list(self.element) } /// Get the pres hints of this candidate. fn pres_hints(&mut self) -> &[ApplicableDeclarationBlock] { - self.cache.pres_hints(self.element) + self.validation_data.pres_hints(self.element) } fn revalidation_match_results( @@ -220,10 +221,11 @@ impl StyleSharingTarget { element.apply_selector_flags(selector_flags_map, el, flags); }; - self.cache.revalidation_match_results(self.element, - stylist, - bloom, - &mut set_selector_flags) + self.validation_data.revalidation_match_results( + self.element, + stylist, + bloom, + &mut set_selector_flags) } /// Attempts to share a style with another node. @@ -247,12 +249,11 @@ impl StyleSharingTarget { &mut self, data); - // FIXME(emilio): Do this in a cleaner way. - mem::swap(&mut self.cache, + mem::swap(&mut self.validation_data, &mut context .thread_local .current_element_info.as_mut().unwrap() - .cached_style_sharing_data); + .validation_data); result } @@ -334,7 +335,7 @@ impl StyleSharingCandidateCache { element: &E, style: &ComputedValues, relations: StyleRelations, - mut cache: CachedStyleSharingData) { + mut validation_data: ValidationData) { use selectors::matching::AFFECTED_BY_PRESENTATIONAL_HINTS; let parent = match element.parent_element() { @@ -371,15 +372,15 @@ impl StyleSharingCandidateCache { // Take advantage of the information we've learned during // selector-matching. if !relations.intersects(AFFECTED_BY_PRESENTATIONAL_HINTS) { - debug_assert!(cache.pres_hints.as_ref().map_or(true, |v| v.is_empty())); - cache.pres_hints = Some(SmallVec::new()); + debug_assert!(validation_data.pres_hints.as_ref().map_or(true, |v| v.is_empty())); + validation_data.pres_hints = Some(SmallVec::new()); } debug!("Inserting into cache: {:?} with parent {:?}", element, parent); self.cache.insert(StyleSharingCandidate { element: unsafe { SendElement::new(*element) }, - cache: cache, + validation_data: validation_data, }); }