From 2d66301e9cca77fc2c6812b1576092791597fa61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 13 Sep 2017 12:56:39 +0200 Subject: [PATCH 1/8] Be consistent about pseudo-elements in cascade_style. Fortunately the checks were done in such a way that no incorrect behavior is exposed. But worth doing it anyway. MozReview-Commit-ID: 8873FkMXXKo --- components/style/style_resolver.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index 3dec436a53c..546a38ed221 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -145,7 +145,6 @@ where relevant_link_found || parent_style.and_then(|s| s.get_visited_style()).is_some(); - let pseudo = self.element.implemented_pseudo_element(); if should_compute_visited_style { visited_style = Some(self.cascade_style( visited_rules.as_ref().or(Some(&primary_results.rule_node)), @@ -153,7 +152,7 @@ where parent_style, layout_parent_style, CascadeVisitedMode::Visited, - /* pseudo = */ pseudo.as_ref(), + /* pseudo = */ None, )); } let style = self.cascade_style( @@ -162,7 +161,7 @@ where parent_style, layout_parent_style, CascadeVisitedMode::Unvisited, - /* pseudo = */ pseudo.as_ref(), + /* pseudo = */ None, ); PrimaryStyle { style, } @@ -485,6 +484,12 @@ where cascade_visited: CascadeVisitedMode, pseudo: Option<&PseudoElement>, ) -> Arc { + debug_assert!( + self.element.implemented_pseudo_element().is_none() || pseudo.is_none(), + "Pseudo-elements can't have other pseudos!" + ); + debug_assert!(pseudo.map_or(true, |p| p.is_eager())); + let mut cascade_flags = CascadeFlags::empty(); if self.element.skip_root_and_item_based_display_fixup() || From 46288f9c4631629edd7ad979d8e2d61e646f6ee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 13 Sep 2017 13:08:49 +0200 Subject: [PATCH 2/8] Share more code for the primary style resolution. The code before predated the existence of CascadeInputs, I believe. MozReview-Commit-ID: 2zPVfWZkgoh --- components/style/style_resolver.rs | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index 546a38ed221..4d806f92de1 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -140,34 +140,19 @@ where None }; - let mut visited_style = None; - let should_compute_visited_style = - relevant_link_found || - parent_style.and_then(|s| s.get_visited_style()).is_some(); - - if should_compute_visited_style { - visited_style = Some(self.cascade_style( - visited_rules.as_ref().or(Some(&primary_results.rule_node)), - /* style_if_visited = */ None, + PrimaryStyle { + style: self.cascade_style_and_visited( + CascadeInputs { + rules: Some(primary_results.rule_node), + visited_rules, + }, parent_style, layout_parent_style, - CascadeVisitedMode::Visited, /* pseudo = */ None, - )); + ), } - let style = self.cascade_style( - Some(&primary_results.rule_node), - visited_style, - parent_style, - layout_parent_style, - CascadeVisitedMode::Unvisited, - /* pseudo = */ None, - ); - - PrimaryStyle { style, } } - /// Resolve the style of a given element, and all its eager pseudo-elements. pub fn resolve_style( &mut self, From 7b019f807b9044c522c3a8b8c2ac9f012083b3ec Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 12 Sep 2017 12:36:41 -0700 Subject: [PATCH 3/8] Use inheritance_parent to control style_sharing. Using traversal_parent here is wrong. MozReview-Commit-ID: GHCIjkgx4VE --- components/style/sharing/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 6aecbd3e3d0..47f82e1a095 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -582,7 +582,7 @@ impl StyleSharingCache { return None; } - if target.traversal_parent().is_none() { + if target.inheritance_parent().is_none() { debug!("{:?} Cannot share style: element has no parent", target.element); return None; @@ -615,8 +615,8 @@ impl StyleSharingCache { // share styles and permit sharing across their children. The latter // check allows us to share style between cousins if the parents // shared style. - let parent = target.traversal_parent(); - let candidate_parent = candidate.element.traversal_parent(); + let parent = target.inheritance_parent(); + let candidate_parent = candidate.element.inheritance_parent(); if parent != candidate_parent && !checks::can_share_style_across_parents(parent, candidate_parent) { trace!("Miss: Parent"); From 729db5ccecc3bb0d624c03f8e9635fdca82bd0ac Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 12 Sep 2017 12:29:29 -0700 Subject: [PATCH 4/8] Cache the parent CV identity on ValidationData. This will make the linear probing faster. If we end up implementing the two-tier cache setup, this code will be unnecessary and can go away. MozReview-Commit-ID: BRfV5ump34n --- components/style/sharing/checks.rs | 42 ++++++++++++++++-------------- components/style/sharing/mod.rs | 42 ++++++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 24 deletions(-) diff --git a/components/style/sharing/checks.rs b/components/style/sharing/checks.rs index 7c82b2550d8..6ff0bf68f51 100644 --- a/components/style/sharing/checks.rs +++ b/components/style/sharing/checks.rs @@ -10,26 +10,30 @@ use Atom; use bloom::StyleBloom; use context::{SelectorFlagsMap, SharedStyleContext}; use dom::TElement; -use servo_arc::Arc; use sharing::{StyleSharingCandidate, StyleSharingTarget}; -/// Whether styles may be shared across the children of the given parent elements. -/// This is used to share style across cousins. -/// -/// Both elements need to be styled already. -pub fn can_share_style_across_parents(first: Option, second: Option) -> bool +/// Determines whether a target and a candidate have compatible parents for sharing. +pub fn parents_allow_sharing( + target: &mut StyleSharingTarget, + candidate: &mut StyleSharingCandidate +) -> bool where E: TElement, { - let (first, second) = match (first, second) { - (Some(f), Some(s)) => (f, s), - _ => return false, - }; + // If the identity of the parent style isn't equal, we can't share. We check + // this first, because the result is cached. + if target.parent_style_identity() != candidate.parent_style_identity() { + return false; + } - debug_assert_ne!(first, second); - - let first_data = first.borrow_data().unwrap(); - let second_data = second.borrow_data().unwrap(); + // Siblings can always share. + let parent = target.inheritance_parent().unwrap(); + let candidate_parent = candidate.element.inheritance_parent().unwrap(); + if parent == candidate_parent { + return true; + } + // Cousins are a bit more complicated. + // // If a parent element was already styled and we traversed past it without // restyling it, that may be because our clever invalidation logic was able // to prove that the styles of that element would remain unchanged despite @@ -40,14 +44,14 @@ pub fn can_share_style_across_parents(first: Option, second: Option) -> // // This is a somewhat conservative check. We could tighten it by having the // invalidation logic explicitly flag elements for which it ellided styling. - if first_data.traversed_without_styling() || second_data.traversed_without_styling() { + let parent_data = parent.borrow_data().unwrap(); + let candidate_parent_data = candidate_parent.borrow_data().unwrap(); + if parent_data.traversed_without_styling() || + candidate_parent_data.traversed_without_styling() { return false; } - let same_computed_values = - Arc::ptr_eq(first_data.styles.primary(), second_data.styles.primary()); - - same_computed_values + true } /// Whether two elements have the same same style attribute (by pointer identity). diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 47f82e1a095..db980a8c79a 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -76,7 +76,7 @@ use matching::MatchMethods; use owning_ref::OwningHandle; use properties::ComputedValues; use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode}; -use servo_arc::Arc; +use servo_arc::{Arc, NonZeroPtrMut}; use smallbitvec::SmallBitVec; use smallvec::SmallVec; use std::marker::PhantomData; @@ -108,6 +108,16 @@ pub enum StyleSharingBehavior { Disallow, } +/// Opaque pointer type to compare ComputedValues identities. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct OpaqueComputedValues(NonZeroPtrMut<()>); +impl OpaqueComputedValues { + fn from(cv: &ComputedValues) -> Self { + let p = NonZeroPtrMut::new(cv as *const ComputedValues as *const () as *mut ()); + OpaqueComputedValues(p) + } +} + /// Some data we want to avoid recomputing all the time while trying to share /// style. #[derive(Debug, Default)] @@ -121,6 +131,9 @@ pub struct ValidationData { /// The list of presentational attributes of the element. pres_hints: Option>, + /// The pointer identity of the parent ComputedValues. + parent_style_identity: Option, + /// The cached result of matching this entry against the revalidation /// selectors. revalidation_match_results: Option, @@ -167,6 +180,18 @@ impl ValidationData { &*self.class_list.as_ref().unwrap() } + /// Get or compute the parent style identity. + pub fn parent_style_identity(&mut self, el: E) -> OpaqueComputedValues + where E: TElement, + { + if self.parent_style_identity.is_none() { + let parent = el.inheritance_parent().unwrap(); + self.parent_style_identity = + Some(OpaqueComputedValues::from(parent.borrow_data().unwrap().styles.primary())); + } + self.parent_style_identity.as_ref().unwrap().clone() + } + /// Computes the revalidation results if needed, and returns it. /// Inline so we know at compile time what bloom_known_valid is. #[inline] @@ -250,6 +275,11 @@ impl StyleSharingCandidate { self.validation_data.pres_hints(self.element) } + /// Get the parent style identity. + fn parent_style_identity(&mut self) -> OpaqueComputedValues { + self.validation_data.parent_style_identity(self.element) + } + /// Compute the bit vector of revalidation selector match results /// for this candidate. fn revalidation_match_results( @@ -304,6 +334,11 @@ impl StyleSharingTarget { self.validation_data.pres_hints(self.element) } + /// Get the parent style identity. + fn parent_style_identity(&mut self) -> OpaqueComputedValues { + self.validation_data.parent_style_identity(self.element) + } + fn revalidation_match_results( &mut self, stylist: &Stylist, @@ -615,10 +650,7 @@ impl StyleSharingCache { // share styles and permit sharing across their children. The latter // check allows us to share style between cousins if the parents // shared style. - let parent = target.inheritance_parent(); - let candidate_parent = candidate.element.inheritance_parent(); - if parent != candidate_parent && - !checks::can_share_style_across_parents(parent, candidate_parent) { + if !checks::parents_allow_sharing(target, candidate) { trace!("Miss: Parent"); return false; } From 9092e6b4c2086f9380a397a5f03ca429d1b6045f Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 12 Sep 2017 16:42:04 -0700 Subject: [PATCH 5/8] Return the element rather than styles from the style sharing cache. This gives us more flexibility, and doesn't cost us anything. MozReview-Commit-ID: CuvOEcLA3My --- components/style/sharing/mod.rs | 9 ++++----- components/style/traversal.rs | 5 +++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index db980a8c79a..435944bb1c7 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -70,7 +70,6 @@ use atomic_refcell::{AtomicRefCell, AtomicRefMut}; use bloom::StyleBloom; use cache::LRUCache; use context::{SelectorFlagsMap, SharedStyleContext, StyleContext}; -use data::ElementStyles; use dom::{TElement, SendElement}; use matching::MatchMethods; use owning_ref::OwningHandle; @@ -377,7 +376,7 @@ impl StyleSharingTarget { pub fn share_style_if_possible( &mut self, context: &mut StyleContext, - ) -> Option { + ) -> Option { let cache = &mut context.thread_local.sharing_cache; let shared_context = &context.shared; let selector_flags_map = &mut context.thread_local.selector_flags; @@ -435,7 +434,7 @@ impl SharingCache { }); } - fn lookup(&mut self, mut is_match: F) -> Option + fn lookup(&mut self, mut is_match: F) -> Option where F: FnMut(&mut StyleSharingCandidate) -> bool { @@ -453,7 +452,7 @@ impl SharingCache { self.entries.touch(i); let front = self.entries.front_mut().unwrap(); debug_assert!(is_match(front)); - Some(front.element.borrow_data().unwrap().styles.clone()) + Some(front.element) } } } @@ -610,7 +609,7 @@ impl StyleSharingCache { selector_flags_map: &mut SelectorFlagsMap, bloom_filter: &StyleBloom, target: &mut StyleSharingTarget, - ) -> Option { + ) -> Option { if shared_context.options.disable_style_sharing_cache { debug!("{:?} Cannot share style: style sharing cache disabled", target.element); diff --git a/components/style/traversal.rs b/components/style/traversal.rs index fdab3ab73c6..dc422f64cad 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -655,9 +655,10 @@ where // Now that our bloom filter is set up, try the style sharing // cache. match target.share_style_if_possible(context) { - Some(styles) => { + Some(shareable_element) => { context.thread_local.statistics.styles_shared += 1; - styles + let shareable_data = shareable_element.borrow_data().unwrap(); + shareable_data.styles.clone() } None => { context.thread_local.statistics.elements_matched += 1; From 7a7070e0758744cced62d2c39e52d92caff39cf7 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 13 Sep 2017 12:22:18 -0700 Subject: [PATCH 6/8] Add some wrapper types to propagate styles out of style resolver. We'll use these next to propagate information about style reuse to the ElementDataFlags. MozReview-Commit-ID: Dya6vgzydpL --- components/style/data.rs | 20 +++++ components/style/matching.rs | 14 ++-- components/style/style_resolver.rs | 124 +++++++++++++++++++---------- components/style/traversal.rs | 11 ++- ports/geckolib/glue.rs | 6 +- 5 files changed, 118 insertions(+), 57 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index 006f0593c76..430737c499e 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -17,7 +17,9 @@ use selector_parser::{EAGER_PSEUDO_COUNT, PseudoElement, RestyleDamage}; use servo_arc::Arc; use shared_lock::StylesheetGuards; use std::fmt; +use std::mem; use std::ops::{Deref, DerefMut}; +use style_resolver::{PrimaryStyle, ResolvedElementStyles, ResolvedStyle}; bitflags! { #[derive(Default)] @@ -264,6 +266,24 @@ impl ElementData { self.styles.primary.is_some() } + /// Returns this element's styles as resolved styles to use for sharing. + pub fn share_styles(&self) -> ResolvedElementStyles { + ResolvedElementStyles { + primary: self.share_primary_style(), + pseudos: self.styles.pseudos.clone(), + } + } + + /// Returns this element's primary style as a resolved style to use for sharing. + pub fn share_primary_style(&self) -> PrimaryStyle { + PrimaryStyle(ResolvedStyle::new(self.styles.primary().clone())) + } + + /// Sets a new set of styles, returning the old ones. + pub fn set_styles(&mut self, new_styles: ResolvedElementStyles) -> ElementStyles { + mem::replace(&mut self.styles, new_styles.into()) + } + /// Returns the kind of restyling that we're going to need to do on this /// element, based of the stored restyle hint. pub fn restyle_kind( diff --git a/components/style/matching.rs b/components/style/matching.rs index 1f140feeb8a..46b16f1c179 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -8,7 +8,7 @@ #![deny(missing_docs)] use context::{ElementCascadeInputs, SelectorFlagsMap, SharedStyleContext, StyleContext}; -use data::{ElementData, ElementStyles}; +use data::ElementData; use dom::TElement; use invalidation::element::restyle_hints::{RESTYLE_CSS_ANIMATIONS, RESTYLE_CSS_TRANSITIONS}; use invalidation::element::restyle_hints::{RESTYLE_SMIL, RESTYLE_STYLE_ATTRIBUTE}; @@ -18,6 +18,7 @@ use rule_tree::{CascadeLevel, StrongRuleNode}; use selector_parser::{PseudoElement, RestyleDamage}; use selectors::matching::ElementSelectorFlags; use servo_arc::{Arc, ArcBorrow}; +use style_resolver::ResolvedElementStyles; use traversal_flags; /// Represents the result of comparing an element's old and new style. @@ -156,7 +157,7 @@ trait PrivateMatchMethods: TElement { StyleResolverForElement::new(*self, context, RuleInclusion::All, PseudoElementResolution::IfApplicable) .cascade_style_and_visited_with_default_parents(inputs); - Some(style) + Some(style.into()) } #[cfg(feature = "gecko")] @@ -530,26 +531,23 @@ pub trait MatchMethods : TElement { &self, context: &mut StyleContext, data: &mut ElementData, - mut new_styles: ElementStyles, + mut new_styles: ResolvedElementStyles, important_rules_changed: bool, ) -> ChildCascadeRequirement { use app_units::Au; use dom::TNode; use std::cmp; - use std::mem; - - debug_assert!(new_styles.primary.is_some(), "How did that happen?"); self.process_animations( context, &mut data.styles.primary, - &mut new_styles.primary.as_mut().unwrap(), + &mut new_styles.primary.0.style, data.hint, important_rules_changed, ); // First of all, update the styles. - let old_styles = mem::replace(&mut data.styles, new_styles); + let old_styles = data.set_styles(new_styles); // Propagate the "can be fragmented" bit. It would be nice to // encapsulate this better. diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index 4d806f92de1..184f095c09e 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -49,12 +49,52 @@ struct MatchingResults { relevant_link_found: bool, } -/// The primary style of an element or an element-backed pseudo-element. -pub struct PrimaryStyle { - /// The style per se. +/// A style returned from the resolver machinery. +pub struct ResolvedStyle { + /// The style itself. pub style: Arc, } +/// The primary style of an element or an element-backed pseudo-element. +pub struct PrimaryStyle(pub ResolvedStyle); + +/// A set of style returned from the resolver machinery. +pub struct ResolvedElementStyles { + /// Primary style. + pub primary: PrimaryStyle, + /// Pseudo styles. + pub pseudos: EagerPseudoStyles, +} + +impl ResolvedStyle { + /// Creates a new ResolvedStyle. + pub fn new(style: Arc) -> Self { + ResolvedStyle { style } + } +} + +impl PrimaryStyle { + /// Convenience accessor for the style. + pub fn style(&self) -> &ComputedValues { + &*self.0.style + } +} + +impl From for Arc { + fn from(r: ResolvedStyle) -> Arc { + r.style + } +} + +impl From for ElementStyles { + fn from(r: ResolvedElementStyles) -> ElementStyles { + ElementStyles { + primary: Some(r.primary.0.into()), + pseudos: r.pseudos, + } + } +} + fn with_default_parent_styles(element: E, f: F) -> R where E: TElement, @@ -140,8 +180,8 @@ where None }; - PrimaryStyle { - style: self.cascade_style_and_visited( + PrimaryStyle( + self.cascade_style_and_visited( CascadeInputs { rules: Some(primary_results.rule_node), visited_rules, @@ -149,8 +189,8 @@ where parent_style, layout_parent_style, /* pseudo = */ None, - ), - } + ) + ) } /// Resolve the style of a given element, and all its eager pseudo-elements. @@ -158,7 +198,7 @@ where &mut self, parent_style: Option<&ComputedValues>, layout_parent_style: Option<&ComputedValues>, - ) -> ElementStyles { + ) -> ResolvedElementStyles { let primary_style = self.resolve_primary_style(parent_style, layout_parent_style); @@ -166,10 +206,10 @@ where if self.element.implemented_pseudo_element().is_none() { let layout_parent_style_for_pseudo = - if primary_style.style.is_display_contents() { + if primary_style.style().is_display_contents() { layout_parent_style } else { - Some(&*primary_style.style) + Some(primary_style.style()) }; SelectorImpl::each_eagerly_cascaded_pseudo_element(|pseudo| { let pseudo_style = self.resolve_pseudo_style( @@ -188,16 +228,15 @@ where }) } - ElementStyles { - // FIXME(emilio): Remove the Option<>. - primary: Some(primary_style.style), + ResolvedElementStyles { + primary: primary_style, pseudos: pseudo_styles, } } /// Resolve an element's styles with the default inheritance parent/layout /// parents. - pub fn resolve_style_with_default_parents(&mut self) -> ElementStyles { + pub fn resolve_style_with_default_parents(&mut self) -> ResolvedElementStyles { with_default_parent_styles(self.element, |parent_style, layout_parent_style| { self.resolve_style(parent_style, layout_parent_style) }) @@ -207,7 +246,7 @@ where pub fn cascade_style_and_visited_with_default_parents( &mut self, inputs: CascadeInputs, - ) -> Arc { + ) -> ResolvedStyle { with_default_parent_styles(self.element, |parent_style, layout_parent_style| { self.cascade_style_and_visited( inputs, @@ -224,7 +263,7 @@ where parent_style: Option<&ComputedValues>, layout_parent_style: Option<&ComputedValues>, pseudo: Option<&PseudoElement>, - ) -> Arc { + ) -> ResolvedStyle { let mut style_if_visited = None; if parent_style.map_or(false, |s| s.get_visited_style().is_some()) || inputs.visited_rules.is_some() { @@ -237,38 +276,41 @@ where pseudo, )); } - self.cascade_style( - inputs.rules.as_ref(), - style_if_visited, - parent_style, - layout_parent_style, - CascadeVisitedMode::Unvisited, - pseudo, - ) + + ResolvedStyle { + style: self.cascade_style( + inputs.rules.as_ref(), + style_if_visited, + parent_style, + layout_parent_style, + CascadeVisitedMode::Unvisited, + pseudo, + ) + } } /// Cascade the element and pseudo-element styles with the default parents. pub fn cascade_styles_with_default_parents( &mut self, inputs: ElementCascadeInputs, - ) -> ElementStyles { + ) -> ResolvedElementStyles { with_default_parent_styles(self.element, move |parent_style, layout_parent_style| { - let primary_style = PrimaryStyle { - style: self.cascade_style_and_visited( + let primary_style = PrimaryStyle( + self.cascade_style_and_visited( inputs.primary, parent_style, layout_parent_style, /* pseudo = */ None, - ), - }; + ) + ); let mut pseudo_styles = EagerPseudoStyles::default(); if let Some(mut pseudo_array) = inputs.pseudos.into_array() { let layout_parent_style_for_pseudo = - if primary_style.style.is_display_contents() { + if primary_style.style().is_display_contents() { layout_parent_style } else { - Some(&*primary_style.style) + Some(primary_style.style()) }; for (i, inputs) in pseudo_array.iter_mut().enumerate() { @@ -278,23 +320,23 @@ where let style = self.cascade_style_and_visited( inputs, - Some(&*primary_style.style), + Some(&*primary_style.style()), layout_parent_style_for_pseudo, Some(&pseudo), ); if !matches!(self.pseudo_resolution, PseudoElementResolution::Force) && - eager_pseudo_is_definitely_not_generated(&pseudo, &style) { + eager_pseudo_is_definitely_not_generated(&pseudo, &style.style) { continue; } - pseudo_styles.set(&pseudo, style); + pseudo_styles.set(&pseudo, style.style); } } } - ElementStyles { - primary: Some(primary_style.style), + ResolvedElementStyles { + primary: primary_style, pseudos: pseudo_styles, } }) @@ -307,7 +349,7 @@ where layout_parent_style: Option<&ComputedValues>, ) -> Option> { let rules = self.match_pseudo( - &originating_element_style.style, + originating_element_style.style(), pseudo, VisitedHandlingMode::AllLinksUnvisited ); @@ -317,9 +359,9 @@ where }; let mut visited_rules = None; - if originating_element_style.style.get_visited_style().is_some() { + if originating_element_style.style().get_visited_style().is_some() { visited_rules = self.match_pseudo( - &originating_element_style.style, + originating_element_style.style(), pseudo, VisitedHandlingMode::RelevantLinkVisited, ); @@ -330,10 +372,10 @@ where rules: Some(rules), visited_rules }, - Some(&originating_element_style.style), + Some(originating_element_style.style()), layout_parent_style, Some(pseudo), - )) + ).style) } fn match_primary( diff --git a/components/style/traversal.rs b/components/style/traversal.rs index dc422f64cad..f7009f04abf 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -419,9 +419,9 @@ where layout_parent_style.as_ref().map(|s| &**s) ); - let is_display_contents = primary_style.style.is_display_contents(); + let is_display_contents = primary_style.style().is_display_contents(); - style = Some(primary_style.style); + style = Some(primary_style.0.into()); if !is_display_contents { layout_parent_style = style.clone(); } @@ -434,7 +434,7 @@ where .resolve_style( style.as_ref().map(|s| &**s), layout_parent_style.as_ref().map(|s| &**s) - ) + ).into() } /// Calculates the style for a single node. @@ -657,8 +657,7 @@ where match target.share_style_if_possible(context) { Some(shareable_element) => { context.thread_local.statistics.styles_shared += 1; - let shareable_data = shareable_element.borrow_data().unwrap(); - shareable_data.styles.clone() + shareable_element.borrow_data().unwrap().share_styles() } None => { context.thread_local.statistics.elements_matched += 1; @@ -679,7 +678,7 @@ where .sharing_cache .insert_if_possible( &element, - new_styles.primary(), + new_styles.primary.style(), &mut target, context.thread_local.bloom_filter.matching_depth(), ); diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 2667a46f4eb..ad1209dd287 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -727,9 +727,11 @@ pub extern "C" fn Servo_StyleSet_GetBaseComputedValuesForElement(raw_data: RawSe }; // Actually `PseudoElementResolution` doesn't matter. - StyleResolverForElement::new(element, &mut context, RuleInclusion::All, PseudoElementResolution::IfApplicable) + let style: Arc = + StyleResolverForElement::new(element, &mut context, RuleInclusion::All, PseudoElementResolution::IfApplicable) .cascade_style_and_visited_with_default_parents(inputs) - .into() + .into(); + style.into() } #[no_mangle] From 1c9b39a8e8760ff087a98eb356f822cbc6eeee13 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 8 Sep 2017 16:03:24 -0700 Subject: [PATCH 7/8] Do a second pass on the sharing cache to reuse style by rule node identity. MozReview-Commit-ID: H67j3Sbt3gr --- components/style/context.rs | 5 +++ components/style/data.rs | 42 ++++++++++++++++-- .../style/properties/properties.mako.rs | 7 +++ components/style/sharing/checks.rs | 3 +- components/style/sharing/mod.rs | 43 +++++++++++++++++++ components/style/style_resolver.rs | 30 +++++++++++-- 6 files changed, 122 insertions(+), 8 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index de8c95bb430..b94f7049129 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -313,6 +313,9 @@ pub struct TraversalStatistics { pub elements_matched: u32, /// The number of cache hits from the StyleSharingCache. pub styles_shared: u32, + /// The number of styles reused via rule node comparison from the + /// StyleSharingCache. + pub styles_reused: u32, /// The number of selectors in the stylist. pub selectors: u32, /// The number of revalidation selectors. @@ -347,6 +350,7 @@ impl<'a> ops::Add for &'a TraversalStatistics { elements_styled: self.elements_styled + other.elements_styled, elements_matched: self.elements_matched + other.elements_matched, styles_shared: self.styles_shared + other.styles_shared, + styles_reused: self.styles_reused + other.styles_reused, selectors: 0, revalidation_selectors: 0, dependency_selectors: 0, @@ -374,6 +378,7 @@ impl fmt::Display for TraversalStatistics { writeln!(f, "[PERF],elements_styled,{}", self.elements_styled)?; writeln!(f, "[PERF],elements_matched,{}", self.elements_matched)?; writeln!(f, "[PERF],styles_shared,{}", self.styles_shared)?; + writeln!(f, "[PERF],styles_reused,{}", self.styles_reused)?; writeln!(f, "[PERF],selectors,{}", self.selectors)?; writeln!(f, "[PERF],revalidation_selectors,{}", self.revalidation_selectors)?; writeln!(f, "[PERF],dependency_selectors,{}", self.dependency_selectors)?; diff --git a/components/style/data.rs b/components/style/data.rs index 430737c499e..575d18e55b0 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -22,8 +22,9 @@ use std::ops::{Deref, DerefMut}; use style_resolver::{PrimaryStyle, ResolvedElementStyles, ResolvedStyle}; bitflags! { + /// Various flags stored on ElementData. #[derive(Default)] - flags ElementDataFlags: u8 { + pub flags ElementDataFlags: u8 { /// Whether the styles changed for this restyle. const WAS_RESTYLED = 1 << 0, /// Whether the last traversal of this element did not do @@ -37,6 +38,13 @@ bitflags! { const TRAVERSED_WITHOUT_STYLING = 1 << 1, /// Whether we reframed/reconstructed any ancestor or self. const ANCESTOR_WAS_RECONSTRUCTED = 1 << 2, + /// Whether the primary style of this element data was reused from another + /// element via a rule node comparison. This allows us to differentiate + /// between elements that shared styles because they met all the criteria + /// of the style sharing cache, compared to elements that reused style + /// structs via rule node identity. The former gives us stronger transitive + /// guarantees that allows us to apply the style sharing cache to cousins. + const PRIMARY_STYLE_REUSED_VIA_RULE_NODE = 1 << 3, } } @@ -202,7 +210,7 @@ pub struct ElementData { pub hint: RestyleHint, /// Flags. - flags: ElementDataFlags, + pub flags: ElementDataFlags, } /// The kind of restyle that a single element should do. @@ -276,11 +284,17 @@ impl ElementData { /// Returns this element's primary style as a resolved style to use for sharing. pub fn share_primary_style(&self) -> PrimaryStyle { - PrimaryStyle(ResolvedStyle::new(self.styles.primary().clone())) + let primary_is_reused = self.flags.contains(PRIMARY_STYLE_REUSED_VIA_RULE_NODE); + PrimaryStyle(ResolvedStyle::new(self.styles.primary().clone(), primary_is_reused)) } /// Sets a new set of styles, returning the old ones. pub fn set_styles(&mut self, new_styles: ResolvedElementStyles) -> ElementStyles { + if new_styles.primary.0.reused_via_rule_node { + self.flags.insert(PRIMARY_STYLE_REUSED_VIA_RULE_NODE); + } else { + self.flags.remove(PRIMARY_STYLE_REUSED_VIA_RULE_NODE); + } mem::replace(&mut self.styles, new_styles.into()) } @@ -442,6 +456,28 @@ impl ElementData { #[cfg(feature = "servo")] pub fn skip_applying_damage(&self) -> bool { false } + /// Returns whether it is safe to perform cousin sharing based on the ComputedValues + /// identity of the primary style in this ElementData. There are a few subtle things + /// to check. + /// + /// First, if a parent element was already styled and we traversed past it without + /// restyling it, that may be because our clever invalidation logic was able to prove + /// that the styles of that element would remain unchanged despite changes to the id + /// or class attributes. However, style sharing relies on the strong guarantee that all + /// the classes and ids up the respective parent chains are identical. As such, if we + /// skipped styling for one (or both) of the parents on this traversal, we can't share + /// styles across cousins. Note that this is a somewhat conservative check. We could + /// tighten it by having the invalidation logic explicitly flag elements for which it + /// ellided styling. + /// + /// Second, we want to only consider elements whose ComputedValues match due to a hit + /// in the style sharing cache, rather than due to the rule-node-based reuse that + /// happens later in the styling pipeline. The former gives us the stronger guarantees + /// we need for style sharing, the latter does not. + pub fn safe_for_cousin_sharing(&self) -> bool { + !self.flags.intersects(TRAVERSED_WITHOUT_STYLING | PRIMARY_STYLE_REUSED_VIA_RULE_NODE) + } + /// Measures memory usage. #[cfg(feature = "gecko")] pub fn size_of_excluding_cvs(&self, ops: &mut MallocSizeOfOps) -> usize { diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 91429401b6c..44da289ca0f 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1997,6 +1997,13 @@ pub struct ComputedValues { inner: ComputedValuesInner, } +impl ComputedValues { + /// Returns the visited rules, if applicable. + pub fn visited_rules(&self) -> Option<<&StrongRuleNode> { + self.visited_style.as_ref().and_then(|s| s.rules.as_ref()) + } +} + #[cfg(feature = "servo")] impl ComputedValues { /// Create a new refcounted `ComputedValues` diff --git a/components/style/sharing/checks.rs b/components/style/sharing/checks.rs index 6ff0bf68f51..d5607590cd3 100644 --- a/components/style/sharing/checks.rs +++ b/components/style/sharing/checks.rs @@ -46,8 +46,7 @@ pub fn parents_allow_sharing( // invalidation logic explicitly flag elements for which it ellided styling. let parent_data = parent.borrow_data().unwrap(); let candidate_parent_data = candidate_parent.borrow_data().unwrap(); - if parent_data.traversed_without_styling() || - candidate_parent_data.traversed_without_styling() { + if !parent_data.safe_for_cousin_sharing() || !candidate_parent_data.safe_for_cousin_sharing() { return false; } diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 435944bb1c7..8046d2339e6 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -74,6 +74,7 @@ use dom::{TElement, SendElement}; use matching::MatchMethods; use owning_ref::OwningHandle; use properties::ComputedValues; +use rule_tree::StrongRuleNode; use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode}; use servo_arc::{Arc, NonZeroPtrMut}; use smallbitvec::SmallBitVec; @@ -115,6 +116,8 @@ impl OpaqueComputedValues { let p = NonZeroPtrMut::new(cv as *const ComputedValues as *const () as *mut ()); OpaqueComputedValues(p) } + + fn eq(&self, cv: &ComputedValues) -> bool { Self::from(cv) == *self } } /// Some data we want to avoid recomputing all the time while trying to share @@ -730,4 +733,44 @@ impl StyleSharingCache { true } + + /// Attempts to find an element in the cache with the given primary rule node and parent. + pub fn lookup_by_rules( + &mut self, + inherited: &ComputedValues, + rules: &StrongRuleNode, + visited_rules: Option<&StrongRuleNode>, + target: E, + ) -> Option { + self.cache_mut().lookup(|candidate| { + if !candidate.parent_style_identity().eq(inherited) { + return false; + } + let data = candidate.element.borrow_data().unwrap(); + let style = data.styles.primary(); + if style.rules.as_ref() != Some(&rules) { + return false; + } + if style.visited_rules() != visited_rules { + return false; + } + + // Rule nodes and styles are computed independent of the element's + // actual visitedness, but at the end of the cascade (in + // `adjust_for_visited`) we do store the visitedness as a flag in + // style. (This is a subtle change from initial visited work that + // landed when computed values were fused, see + // https://bugzilla.mozilla.org/show_bug.cgi?id=1381635.) + // So at the moment, we need to additionally compare visitedness, + // since that is not accounted for by rule nodes alone. + // FIXME(jryans): This seems like it breaks the constant time + // requirements of visited, assuming we get a cache hit on only one + // of unvisited vs. visited. + if target.is_visited_link() != candidate.element.is_visited_link() { + return false; + } + + true + }) + } } diff --git a/components/style/style_resolver.rs b/components/style/style_resolver.rs index 184f095c09e..f236d9dfb1f 100644 --- a/components/style/style_resolver.rs +++ b/components/style/style_resolver.rs @@ -53,6 +53,8 @@ struct MatchingResults { pub struct ResolvedStyle { /// The style itself. pub style: Arc, + /// Whether the style was reused from another element via the rule node. + pub reused_via_rule_node: bool, } /// The primary style of an element or an element-backed pseudo-element. @@ -68,8 +70,8 @@ pub struct ResolvedElementStyles { impl ResolvedStyle { /// Creates a new ResolvedStyle. - pub fn new(style: Arc) -> Self { - ResolvedStyle { style } + pub fn new(style: Arc, reused_via_rule_node: bool) -> Self { + ResolvedStyle { style, reused_via_rule_node } } } @@ -264,6 +266,27 @@ where layout_parent_style: Option<&ComputedValues>, pseudo: Option<&PseudoElement>, ) -> ResolvedStyle { + // Before doing the cascade, check the sharing cache and see if we can reuse + // the style via rule node identity. + let may_reuse = pseudo.is_none() && !self.element.is_native_anonymous() && + parent_style.is_some() && inputs.rules.is_some(); + if may_reuse { + let cached = self.context.thread_local.sharing_cache.lookup_by_rules( + parent_style.unwrap(), + inputs.rules.as_ref().unwrap(), + inputs.visited_rules.as_ref(), + self.element, + ); + if let Some(el) = cached { + self.context.thread_local.statistics.styles_reused += 1; + let mut style = el.borrow_data().unwrap().share_primary_style().0; + style.reused_via_rule_node = true; + return style; + } + } + + // No style to reuse. Cascade the style, starting with visited style + // if necessary. let mut style_if_visited = None; if parent_style.map_or(false, |s| s.get_visited_style().is_some()) || inputs.visited_rules.is_some() { @@ -285,7 +308,8 @@ where layout_parent_style, CascadeVisitedMode::Unvisited, pseudo, - ) + ), + reused_via_rule_node: false, } } From 8e5c1c771e85a69b2de8b49a224bee01d32aa1a7 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 12 Sep 2017 18:54:19 -0700 Subject: [PATCH 8/8] Share styles during recascades. MozReview-Commit-ID: AFTwtzi4P93 --- components/style/sharing/mod.rs | 17 +++++++++++------ components/style/traversal.rs | 34 +++++++++++++++++++++++---------- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 8046d2339e6..ec818add99c 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -430,11 +430,16 @@ impl SharingCacheBase { } impl SharingCache { - fn insert(&mut self, el: E, validation_data_holder: &mut StyleSharingTarget) { - self.entries.insert(StyleSharingCandidate { - element: el, - validation_data: validation_data_holder.take_validation_data(), - }); + fn insert( + &mut self, + element: E, + validation_data_holder: Option<&mut StyleSharingTarget>, + ) { + let validation_data = match validation_data_holder { + Some(v) => v.take_validation_data(), + None => ValidationData::default(), + }; + self.entries.insert(StyleSharingCandidate { element, validation_data }); } fn lookup(&mut self, mut is_match: F) -> Option @@ -544,7 +549,7 @@ impl StyleSharingCache { pub fn insert_if_possible(&mut self, element: &E, style: &ComputedValues, - validation_data_holder: &mut StyleSharingTarget, + validation_data_holder: Option<&mut StyleSharingTarget>, dom_depth: usize) { let parent = match element.traversal_parent() { Some(element) => element, diff --git a/components/style/traversal.rs b/components/style/traversal.rs index f7009f04abf..2823abddec8 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -646,6 +646,10 @@ where traversal_data.current_dom_depth); context.thread_local.bloom_filter.assert_complete(element); + debug_assert_eq!( + context.thread_local.bloom_filter.matching_depth(), + traversal_data.current_dom_depth + ); // This is only relevant for animations as of right now. important_rules_changed = true; @@ -679,8 +683,8 @@ where .insert_if_possible( &element, new_styles.primary.style(), - &mut target, - context.thread_local.bloom_filter.matching_depth(), + Some(&mut target), + traversal_data.current_dom_depth, ); new_styles @@ -709,15 +713,25 @@ where let cascade_inputs = ElementCascadeInputs::new_from_element_data(data); - let mut resolver = - StyleResolverForElement::new( - element, - context, - RuleInclusion::All, - PseudoElementResolution::IfApplicable - ); + let new_styles = { + let mut resolver = + StyleResolverForElement::new( + element, + context, + RuleInclusion::All, + PseudoElementResolution::IfApplicable + ); - resolver.cascade_styles_with_default_parents(cascade_inputs) + resolver.cascade_styles_with_default_parents(cascade_inputs) + }; + context.thread_local.sharing_cache.insert_if_possible( + &element, + new_styles.primary.style(), + None, + traversal_data.current_dom_depth, + ); + + new_styles } };