Mirrors/servo
1
0
Fork 0
mirror of https://github.com/servo/servo.git synced 2025-08-06 14:10:11 +01:00
Code Issues Projects Releases Packages Wiki Activity

Auto merge of #16578 - bholley:fix_style_sharing_cache, r=emilio

Browse source 
Fix up the style sharing cache

https://bugzilla.mozilla.org/show_bug.cgi?id=1358694
This commit is contained in:
bors-servo 2017-04-23 14:45:29 -05:00 • committed by GitHub
commit a26079d322
5 changed files with 57 additions and 77 deletions
Download patch file Download diff file Expand all files Collapse all files

74
components/selectors/matching.rs
View file

@ -17,47 +17,24 @@ bitflags! {
/// the selector matching process. /// the selector matching process.
/// ///
/// This is used to implement efficient sharing. /// This is used to implement efficient sharing.
pub flags StyleRelations: u16 { pub flags StyleRelations: usize {
/// 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. /// 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 matches the :empty pseudo class.
const AFFECTED_BY_EMPTY = 1 << 5,
/// Whether this element has a style attribute. Computed /// Whether this element has a style attribute. Computed
/// externally. /// 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 /// Whether this element is affected by presentational hints. This is
/// computed externally (that is, in Servo). /// 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. /// Whether this element has pseudo-element styles. Computed externally.
const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 8, const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 3,
/// 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,
} }
} }
bitflags! { bitflags! {
/// Set of flags that are set on either the element or its parent (depending /// 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. /// 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 /// When a child is added or removed from the parent, all the children
/// must be restyled, because they may match :nth-last-child, /// must be restyled, because they may match :nth-last-child,
/// :last-of-type, :nth-last-of-type, or :only-of-type. /// :last-of-type, :nth-last-of-type, or :only-of-type.
@ -364,21 +341,17 @@ fn matches_simple_selector<E, F>(
false false
} }
Component::NonTSPseudoClass(ref pc) => { Component::NonTSPseudoClass(ref pc) => {
relation_if!(element.match_non_ts_pseudo_class(pc, relations, flags_setter), element.match_non_ts_pseudo_class(pc, relations, flags_setter)
AFFECTED_BY_STATE)
} }
Component::FirstChild => { Component::FirstChild => {
relation_if!(matches_first_child(element, flags_setter), matches_first_child(element, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::LastChild => { Component::LastChild => {
relation_if!(matches_last_child(element, flags_setter), matches_last_child(element, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::OnlyChild => { Component::OnlyChild => {
relation_if!(matches_first_child(element, flags_setter) && matches_first_child(element, flags_setter) &&
matches_last_child(element, flags_setter), matches_last_child(element, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::Root => { Component::Root => {
// We never share styles with an element with no parent, so no point // We never share styles with an element with no parent, so no point
@ -387,36 +360,29 @@ fn matches_simple_selector<E, F>(
} }
Component::Empty => { Component::Empty => {
flags_setter(element, HAS_EMPTY_SELECTOR); flags_setter(element, HAS_EMPTY_SELECTOR);
relation_if!(element.is_empty(), AFFECTED_BY_EMPTY) element.is_empty()
} }
Component::NthChild(a, b) => { Component::NthChild(a, b) => {
relation_if!(matches_generic_nth_child(element, a, b, false, false, flags_setter), matches_generic_nth_child(element, a, b, false, false, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::NthLastChild(a, b) => { Component::NthLastChild(a, b) => {
relation_if!(matches_generic_nth_child(element, a, b, false, true, flags_setter), matches_generic_nth_child(element, a, b, false, true, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::NthOfType(a, b) => { Component::NthOfType(a, b) => {
relation_if!(matches_generic_nth_child(element, a, b, true, false, flags_setter), matches_generic_nth_child(element, a, b, true, false, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::NthLastOfType(a, b) => { Component::NthLastOfType(a, b) => {
relation_if!(matches_generic_nth_child(element, a, b, true, true, flags_setter), matches_generic_nth_child(element, a, b, true, true, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::FirstOfType => { Component::FirstOfType => {
relation_if!(matches_generic_nth_child(element, 0, 1, true, false, flags_setter), matches_generic_nth_child(element, 0, 1, true, false, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::LastOfType => { Component::LastOfType => {
relation_if!(matches_generic_nth_child(element, 0, 1, true, true, flags_setter), matches_generic_nth_child(element, 0, 1, true, true, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::OnlyOfType => { Component::OnlyOfType => {
relation_if!(matches_generic_nth_child(element, 0, 1, true, false, flags_setter) && matches_generic_nth_child(element, 0, 1, true, false, flags_setter) &&
matches_generic_nth_child(element, 0, 1, true, true, flags_setter), matches_generic_nth_child(element, 0, 1, true, true, flags_setter)
AFFECTED_BY_CHILD_INDEX)
} }
Component::Negation(ref negated) => { Component::Negation(ref negated) => {
!negated.iter().all(|s| { !negated.iter().all(|s| {

6
components/style/context.rs
View file

@ -94,7 +94,11 @@ impl Default for StyleSystemOptions {
#[cfg(feature = "gecko")] #[cfg(feature = "gecko")]
fn default() -> Self { fn default() -> Self {
StyleSystemOptions { 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"), dump_style_statistics: get_env("DUMP_STYLE_STATISTICS"),
} }
} }

3
components/style/gecko/wrapper.rs
View file

@ -1129,7 +1129,6 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
} }
elem = prev; elem = prev;
} }
relations.insert(AFFECTED_BY_CHILD_INDEX);
true true
} }
NonTSPseudoClass::MozLastNode => { NonTSPseudoClass::MozLastNode => {
@ -1141,7 +1140,6 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
} }
elem = next; elem = next;
} }
relations.insert(AFFECTED_BY_CHILD_INDEX);
true true
} }
NonTSPseudoClass::MozOnlyWhitespace => { 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()) { if self.as_node().dom_children().any(|c| c.contains_non_whitespace_content()) {
return false return false
} }
relations.insert(AFFECTED_BY_EMPTY);
true true
} }
NonTSPseudoClass::MozTableBorderNonzero | NonTSPseudoClass::MozTableBorderNonzero |

48
components/style/matching.rs
View file

@ -34,7 +34,7 @@ use stylist::ApplicableDeclarationBlock;
fn relations_are_shareable(relations: &StyleRelations) -> bool { fn relations_are_shareable(relations: &StyleRelations) -> bool {
use selectors::matching::*; use selectors::matching::*;
!relations.intersects(AFFECTED_BY_ID_SELECTOR | !relations.intersects(AFFECTED_BY_ID_SELECTOR |
AFFECTED_BY_PSEUDO_ELEMENTS | AFFECTED_BY_STATE | AFFECTED_BY_PSEUDO_ELEMENTS |
AFFECTED_BY_STYLE_ATTRIBUTE | AFFECTED_BY_STYLE_ATTRIBUTE |
AFFECTED_BY_PRESENTATIONAL_HINTS) AFFECTED_BY_PRESENTATIONAL_HINTS)
} }
@ -102,6 +102,17 @@ pub enum CacheMiss {
Revalidation, Revalidation,
} }
fn same_computed_values<E: TElement>(first: Option<E>, second: Option<E>) -> 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<E: TElement>(element: &E, fn element_matches_candidate<E: TElement>(element: &E,
candidate: &mut StyleSharingCandidate<E>, candidate: &mut StyleSharingCandidate<E>,
candidate_element: &E, candidate_element: &E,
@ -116,7 +127,12 @@ fn element_matches_candidate<E: TElement>(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) miss!(Parent)
} }
@ -152,7 +168,7 @@ fn element_matches_candidate<E: TElement>(element: &E,
miss!(Class) miss!(Class)
} }
if !have_same_presentational_hints(element, candidate_element) { if has_presentational_hints(element) {
miss!(PresHints) miss!(PresHints)
} }
@ -168,18 +184,10 @@ fn element_matches_candidate<E: TElement>(element: &E,
Ok(current_styles.primary.clone()) Ok(current_styles.primary.clone())
} }
fn have_same_presentational_hints<E: TElement>(element: &E, candidate: &E) -> bool { fn has_presentational_hints<E: TElement>(element: &E) -> bool {
let mut first = ForgetfulSink::new(); let mut hints = ForgetfulSink::new();
element.synthesize_presentational_hints_for_legacy_attributes(&mut first); element.synthesize_presentational_hints_for_legacy_attributes(&mut hints);
!hints.is_empty()
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 have_same_class<E: TElement>(element: &E, fn have_same_class<E: TElement>(element: &E,
@ -296,6 +304,14 @@ impl<E: TElement> StyleSharingCandidateCache<E> {
return; 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(); let box_style = style.get_box();
if box_style.specifies_transitions() { if box_style.specifies_transitions() {
debug!("Failing to insert to the cache: 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 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, // If we previously tried to match this element against the cache,
// the revalidation match results will already be cached. Otherwise // the revalidation match results will already be cached. Otherwise
// we'll have None, and compute them later on-demand. // we'll have None, and compute them later on-demand.

3
components/style/stylist.rs
View file

@ -24,7 +24,6 @@ use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource};
use selector_parser::{SelectorImpl, PseudoElement, Snapshot}; use selector_parser::{SelectorImpl, PseudoElement, Snapshot};
use selectors::Element; use selectors::Element;
use selectors::bloom::BloomFilter; 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::{AFFECTED_BY_STYLE_ATTRIBUTE, AFFECTED_BY_PRESENTATIONAL_HINTS};
use selectors::matching::{ElementSelectorFlags, StyleRelations, matches_selector}; use selectors::matching::{ElementSelectorFlags, StyleRelations, matches_selector};
use selectors::parser::{Component, Selector, SelectorInner, LocalName as LocalNameSelector}; use selectors::parser::{Component, Selector, SelectorInner, LocalName as LocalNameSelector};
@ -716,7 +715,6 @@ impl Stylist {
// The animations sheet (CSS animations, script-generated animations, // The animations sheet (CSS animations, script-generated animations,
// and CSS transitions that are no longer tied to CSS markup) // and CSS transitions that are no longer tied to CSS markup)
if let Some(anim) = animation_rules.0 { if let Some(anim) = animation_rules.0 {
relations |= AFFECTED_BY_ANIMATIONS;
Push::push( Push::push(
applicable_declarations, applicable_declarations,
ApplicableDeclarationBlock::from_declarations(anim.clone(), ApplicableDeclarationBlock::from_declarations(anim.clone(),
@ -773,7 +771,6 @@ impl Stylist {
// Step 10: Transitions. // Step 10: Transitions.
// The transitions sheet (CSS transitions that are tied to CSS markup) // The transitions sheet (CSS transitions that are tied to CSS markup)
if let Some(anim) = animation_rules.1 { if let Some(anim) = animation_rules.1 {
relations |= AFFECTED_BY_TRANSITIONS;
Push::push( Push::push(
applicable_declarations, applicable_declarations,
ApplicableDeclarationBlock::from_declarations(anim.clone(), CascadeLevel::Transitions)); ApplicableDeclarationBlock::from_declarations(anim.clone(), CascadeLevel::Transitions));