diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index 83a094b637a..27aebeb544b 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -17,47 +17,24 @@ bitflags! { /// the selector matching process. /// /// 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, - + pub flags StyleRelations: usize { /// 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, - + 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, - - /// 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, + 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. @@ -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/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"), } } 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/matching.rs b/components/style/matching.rs index fd92ae881e7..e1c1ba47223 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) } @@ -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) } @@ -152,7 +168,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 +184,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 +304,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"); @@ -822,7 +838,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. 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));