From bdb84c7539e9e5715d5af6cdc9667b11f0dc252b Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 21 Apr 2017 18:05:29 -0700 Subject: [PATCH 1/6] Don't skip the style sharing cache if an element is affected by state. All elements end up with AFFECTED_BY_STATE right now, presumably due to the UA sheet, and some non-TS pseudo-classes we've added, which means that we currently never insert anything into the cache. MozReview-Commit-ID: 5IU4qrjeJFy --- components/style/matching.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index fd92ae881e7..8b01b9305e4 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -34,7 +34,7 @@ use stylist::ApplicableDeclarationBlock; fn relations_are_shareable(relations: &StyleRelations) -> bool { use selectors::matching::*; !relations.intersects(AFFECTED_BY_ID_SELECTOR | - AFFECTED_BY_PSEUDO_ELEMENTS | AFFECTED_BY_STATE | + AFFECTED_BY_PSEUDO_ELEMENTS | AFFECTED_BY_STYLE_ATTRIBUTE | AFFECTED_BY_PRESENTATIONAL_HINTS) } @@ -822,7 +822,7 @@ 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 sharing == StyleSharingBehavior::Allow { // 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. From 60f846b2ca2662b5acba40eb87061b47a450447c Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 21 Apr 2017 18:11:55 -0700 Subject: [PATCH 2/6] Remove unused StyleRelations. MozReview-Commit-ID: ed0NiuY9Ek --- components/selectors/matching.rs | 62 +++++++------------------------ components/style/gecko/wrapper.rs | 3 -- components/style/stylist.rs | 3 -- 3 files changed, 14 insertions(+), 54 deletions(-) diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index 83a094b637a..63f6429d353 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -18,39 +18,16 @@ bitflags! { /// /// This is used to implement efficient sharing. pub flags StyleRelations: u16 { - /// Whether this element has matched any rule whose matching is - /// determined by its position in the tree (i.e., first-child, - /// nth-child, etc.). - const AFFECTED_BY_CHILD_INDEX = 1 << 1, - - /// Whether this flag is affected by any state (i.e., non - /// tree-structural pseudo-class). - const AFFECTED_BY_STATE = 1 << 2, - /// Whether this element is affected by an ID selector. const AFFECTED_BY_ID_SELECTOR = 1 << 3, - - /// Whether this element matches the :empty pseudo class. - const AFFECTED_BY_EMPTY = 1 << 5, - /// Whether this element has a style attribute. Computed /// externally. const AFFECTED_BY_STYLE_ATTRIBUTE = 1 << 6, - /// Whether this element is affected by presentational hints. This is /// computed externally (that is, in Servo). const AFFECTED_BY_PRESENTATIONAL_HINTS = 1 << 7, - /// Whether this element has pseudo-element styles. Computed externally. const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 8, - - /// Whether this element has effective animation styles. Computed - /// externally. - const AFFECTED_BY_ANIMATIONS = 1 << 9, - - /// Whether this element has effective transition styles. Computed - /// externally. - const AFFECTED_BY_TRANSITIONS = 1 << 10, } } @@ -364,21 +341,17 @@ fn matches_simple_selector( false } Component::NonTSPseudoClass(ref pc) => { - relation_if!(element.match_non_ts_pseudo_class(pc, relations, flags_setter), - AFFECTED_BY_STATE) + element.match_non_ts_pseudo_class(pc, relations, flags_setter) } Component::FirstChild => { - relation_if!(matches_first_child(element, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_first_child(element, flags_setter) } Component::LastChild => { - relation_if!(matches_last_child(element, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_last_child(element, flags_setter) } Component::OnlyChild => { - relation_if!(matches_first_child(element, flags_setter) && - matches_last_child(element, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_first_child(element, flags_setter) && + matches_last_child(element, flags_setter) } Component::Root => { // We never share styles with an element with no parent, so no point @@ -387,36 +360,29 @@ fn matches_simple_selector( } Component::Empty => { flags_setter(element, HAS_EMPTY_SELECTOR); - relation_if!(element.is_empty(), AFFECTED_BY_EMPTY) + element.is_empty() } Component::NthChild(a, b) => { - relation_if!(matches_generic_nth_child(element, a, b, false, false, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_generic_nth_child(element, a, b, false, false, flags_setter) } Component::NthLastChild(a, b) => { - relation_if!(matches_generic_nth_child(element, a, b, false, true, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_generic_nth_child(element, a, b, false, true, flags_setter) } Component::NthOfType(a, b) => { - relation_if!(matches_generic_nth_child(element, a, b, true, false, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_generic_nth_child(element, a, b, true, false, flags_setter) } Component::NthLastOfType(a, b) => { - relation_if!(matches_generic_nth_child(element, a, b, true, true, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_generic_nth_child(element, a, b, true, true, flags_setter) } Component::FirstOfType => { - relation_if!(matches_generic_nth_child(element, 0, 1, true, false, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_generic_nth_child(element, 0, 1, true, false, flags_setter) } Component::LastOfType => { - relation_if!(matches_generic_nth_child(element, 0, 1, true, true, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_generic_nth_child(element, 0, 1, true, true, flags_setter) } Component::OnlyOfType => { - relation_if!(matches_generic_nth_child(element, 0, 1, true, false, flags_setter) && - matches_generic_nth_child(element, 0, 1, true, true, flags_setter), - AFFECTED_BY_CHILD_INDEX) + matches_generic_nth_child(element, 0, 1, true, false, flags_setter) && + matches_generic_nth_child(element, 0, 1, true, true, flags_setter) } Component::Negation(ref negated) => { !negated.iter().all(|s| { diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 1604a09df77..8ac1ac48d5d 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -1129,7 +1129,6 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { } elem = prev; } - relations.insert(AFFECTED_BY_CHILD_INDEX); true } NonTSPseudoClass::MozLastNode => { @@ -1141,7 +1140,6 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { } elem = next; } - relations.insert(AFFECTED_BY_CHILD_INDEX); true } NonTSPseudoClass::MozOnlyWhitespace => { @@ -1149,7 +1147,6 @@ impl<'le> ::selectors::Element for GeckoElement<'le> { if self.as_node().dom_children().any(|c| c.contains_non_whitespace_content()) { return false } - relations.insert(AFFECTED_BY_EMPTY); true } NonTSPseudoClass::MozTableBorderNonzero | diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 934954a93d3..8b698ab692d 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -24,7 +24,6 @@ use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource}; use selector_parser::{SelectorImpl, PseudoElement, Snapshot}; use selectors::Element; use selectors::bloom::BloomFilter; -use selectors::matching::{AFFECTED_BY_ANIMATIONS, AFFECTED_BY_TRANSITIONS}; use selectors::matching::{AFFECTED_BY_STYLE_ATTRIBUTE, AFFECTED_BY_PRESENTATIONAL_HINTS}; use selectors::matching::{ElementSelectorFlags, StyleRelations, matches_selector}; use selectors::parser::{Component, Selector, SelectorInner, LocalName as LocalNameSelector}; @@ -716,7 +715,6 @@ impl Stylist { // The animations sheet (CSS animations, script-generated animations, // and CSS transitions that are no longer tied to CSS markup) if let Some(anim) = animation_rules.0 { - relations |= AFFECTED_BY_ANIMATIONS; Push::push( applicable_declarations, ApplicableDeclarationBlock::from_declarations(anim.clone(), @@ -773,7 +771,6 @@ impl Stylist { // Step 10: Transitions. // The transitions sheet (CSS transitions that are tied to CSS markup) if let Some(anim) = animation_rules.1 { - relations |= AFFECTED_BY_TRANSITIONS; Push::push( applicable_declarations, ApplicableDeclarationBlock::from_declarations(anim.clone(), CascadeLevel::Transitions)); From e4ed69c52b33f3eb1785b6a54037485af223f091 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 21 Apr 2017 18:14:29 -0700 Subject: [PATCH 3/6] Reorganize bits and make the flags usize. These are never stored persistently anywhere, and I'm pretty sure it's slower for the compiler/CPU to operate on non-word-sized flags. MozReview-Commit-ID: LQNsJbUsw85 --- components/selectors/matching.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index 63f6429d353..27aebeb544b 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -17,24 +17,24 @@ bitflags! { /// the selector matching process. /// /// This is used to implement efficient sharing. - pub flags StyleRelations: u16 { + pub flags StyleRelations: usize { /// Whether this element is affected by an ID selector. - const AFFECTED_BY_ID_SELECTOR = 1 << 3, + const AFFECTED_BY_ID_SELECTOR = 1 << 0, /// Whether this element has a style attribute. Computed /// externally. - const AFFECTED_BY_STYLE_ATTRIBUTE = 1 << 6, + const AFFECTED_BY_STYLE_ATTRIBUTE = 1 << 1, /// Whether this element is affected by presentational hints. This is /// computed externally (that is, in Servo). - const AFFECTED_BY_PRESENTATIONAL_HINTS = 1 << 7, + const AFFECTED_BY_PRESENTATIONAL_HINTS = 1 << 2, /// Whether this element has pseudo-element styles. Computed externally. - const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 8, + const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 3, } } bitflags! { /// Set of flags that are set on either the element or its parent (depending /// on the flag) if the element could potentially match a selector. - pub flags ElementSelectorFlags: u8 { + pub flags ElementSelectorFlags: usize { /// When a child is added or removed from the parent, all the children /// must be restyled, because they may match :nth-last-child, /// :last-of-type, :nth-last-of-type, or :only-of-type. From 1d6892afa30b80a10362900434985d9fbad0cf78 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 21 Apr 2017 18:22:17 -0700 Subject: [PATCH 4/6] Make it clearer that we never insert elements with preshints into the cache. MozReview-Commit-ID: 9bdc0sNZeEy --- components/style/matching.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 8b01b9305e4..20344b86f21 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -152,7 +152,7 @@ fn element_matches_candidate(element: &E, miss!(Class) } - if !have_same_presentational_hints(element, candidate_element) { + if has_presentational_hints(element) { miss!(PresHints) } @@ -168,18 +168,10 @@ fn element_matches_candidate(element: &E, Ok(current_styles.primary.clone()) } -fn have_same_presentational_hints(element: &E, candidate: &E) -> bool { - let mut first = ForgetfulSink::new(); - element.synthesize_presentational_hints_for_legacy_attributes(&mut first); - - if cfg!(debug_assertions) { - let mut second = vec![]; - candidate.synthesize_presentational_hints_for_legacy_attributes(&mut second); - debug_assert!(second.is_empty(), - "Should never have inserted an element with preshints in the cache!"); - } - - first.is_empty() +fn has_presentational_hints(element: &E) -> bool { + let mut hints = ForgetfulSink::new(); + element.synthesize_presentational_hints_for_legacy_attributes(&mut hints); + !hints.is_empty() } fn have_same_class(element: &E, @@ -296,6 +288,14 @@ 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"); From 750851bf43eceeaf5df34c4034d144326c7825c8 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 21 Apr 2017 18:35:12 -0700 Subject: [PATCH 5/6] Share styles between cousins if the parent styles were shared. MozReview-Commit-ID: 1QFCSzC5xvk --- components/style/matching.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 20344b86f21..e1c1ba47223 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -102,6 +102,17 @@ pub enum CacheMiss { Revalidation, } +fn same_computed_values(first: Option, second: Option) -> bool { + let (a, b) = match (first, second) { + (Some(f), Some(s)) => (f, s), + _ => return false, + }; + + let eq = ::arc_ptr_eq(a.borrow_data().unwrap().styles().primary.values(), + b.borrow_data().unwrap().styles().primary.values()); + eq +} + fn element_matches_candidate(element: &E, candidate: &mut StyleSharingCandidate, candidate_element: &E, @@ -116,7 +127,12 @@ fn element_matches_candidate(element: &E, } } - if element.parent_element() != candidate_element.parent_element() { + // 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 candidate_parent = candidate_element.parent_element(); + if parent != candidate_parent && !same_computed_values(parent, candidate_parent) { miss!(Parent) } From d7e121961fa0b497d97545728ea3eaa73970ee00 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Sat, 22 Apr 2017 13:12:55 -0700 Subject: [PATCH 6/6] Disable the style sharing cache on opt builds to avoid regressing Talos by matching too many revalidation selectors. MozReview-Commit-ID: 10qSho1gXjw --- components/style/context.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/components/style/context.rs b/components/style/context.rs index bf12841f8a4..01043637a0d 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -94,7 +94,11 @@ impl Default for StyleSystemOptions { #[cfg(feature = "gecko")] fn default() -> Self { StyleSystemOptions { - disable_style_sharing_cache: get_env("DISABLE_STYLE_SHARING_CACHE"), + disable_style_sharing_cache: + // Disable the style sharing cache on opt builds until + // bug 1358693 is fixed, but keep it on debug builds to make + // sure we don't introduce correctness bugs. + if cfg!(debug_assertions) { get_env("DISABLE_STYLE_SHARING_CACHE") } else { true }, dump_style_statistics: get_env("DUMP_STYLE_STATISTICS"), } }