From 173ec260e67e705af322348ac98fe8c47d5d04e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Wed, 10 Aug 2016 23:17:00 -0700 Subject: [PATCH] Tidy up, make the cache a bit more performant. Haven't measured a lot, will do tomorrow, but I want a test run. --- components/style/context.rs | 1 - components/style/matching.rs | 186 ++++++++++++++++++++++------------ components/style/traversal.rs | 5 +- 3 files changed, 122 insertions(+), 70 deletions(-) diff --git a/components/style/context.rs b/components/style/context.rs index b5aa009dd25..f1c5ac3f307 100644 --- a/components/style/context.rs +++ b/components/style/context.rs @@ -7,7 +7,6 @@ use animation::Animation; use app_units::Au; use dom::OpaqueNode; -use dom::TElement; use error_reporting::ParseErrorReporter; use euclid::Size2D; use matching::{ApplicableDeclarationsCache, StyleSharingCandidateCache}; diff --git a/components/style/matching.rs b/components/style/matching.rs index d8b014af623..ae1253a94bf 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -15,20 +15,18 @@ use data::PrivateStyleData; use dom::{TElement, TNode, TRestyleDamage, UnsafeNode}; use properties::longhands::display::computed_value as display; use properties::{ComputedValues, PropertyDeclaration, cascade}; -use selector_impl::{ElementExt, TheSelectorImpl, PseudoElement}; +use selector_impl::{TheSelectorImpl, PseudoElement}; use selector_matching::{DeclarationBlock, Stylist}; use selectors::bloom::BloomFilter; use selectors::matching::{StyleRelations, AFFECTED_BY_PSEUDO_ELEMENTS}; use selectors::{Element, MatchAttr}; use sink::ForgetfulSink; use smallvec::SmallVec; -use std::borrow::Borrow; use std::collections::HashMap; -use std::fmt; use std::hash::{BuildHasherDefault, Hash, Hasher}; use std::slice::Iter; use std::sync::Arc; -use string_cache::{Atom, Namespace}; +use string_cache::Atom; use traversal::RestyleResult; use util::opts; @@ -180,6 +178,27 @@ impl ApplicableDeclarationsCache { } } +/// Information regarding a candidate. +/// +/// TODO: We can stick a lot more info here. +#[derive(Debug)] +struct StyleSharingCandidate { + /// The node, guaranteed to be an element. + node: UnsafeNode, + /// The cached computed style, here for convenience. + style: Arc, + /// The cached common style affecting attribute info. + common_style_affecting_attributes: CommonStyleAffectingAttributes, +} + +impl PartialEq for StyleSharingCandidate { + fn eq(&self, other: &Self) -> bool { + self.node == other.node && + arc_ptr_eq(&self.style, &other.style) && + self.common_style_affecting_attributes == other.common_style_affecting_attributes + } +} + /// An LRU cache of the last few nodes seen, so that we can aggressively try to /// reuse their styles. /// @@ -191,7 +210,7 @@ impl ApplicableDeclarationsCache { /// difficulty, but good luck with layout and all the types with assoc. /// lifetimes). pub struct StyleSharingCandidateCache { - cache: LRUCache, + cache: LRUCache, } #[derive(Clone, Debug)] @@ -211,7 +230,8 @@ pub enum CacheMiss { } fn element_matches_candidate(element: &E, - candidate: &E, + candidate: &StyleSharingCandidate, + candidate_element: &E, shared_context: &SharedStyleContext) -> Result, CacheMiss> { macro_rules! miss { @@ -220,23 +240,23 @@ fn element_matches_candidate(element: &E, } } - if element.parent_element() != candidate.parent_element() { + if element.parent_element() != candidate_element.parent_element() { miss!(Parent) } - if *element.get_local_name() != *candidate.get_local_name() { + if *element.get_local_name() != *candidate_element.get_local_name() { miss!(LocalName) } - if *element.get_namespace() != *candidate.get_namespace() { + if *element.get_namespace() != *candidate_element.get_namespace() { miss!(Namespace) } - if element.is_link() != candidate.is_link() { + if element.is_link() != candidate_element.is_link() { miss!(Link) } - if element.get_state() != candidate.get_state() { + if element.get_state() != candidate_element.get_state() { miss!(State) } @@ -248,7 +268,7 @@ fn element_matches_candidate(element: &E, miss!(StyleAttr) } - if !have_same_class(element, candidate) { + if !have_same_class(element, candidate_element) { miss!(Class) } @@ -256,34 +276,35 @@ fn element_matches_candidate(element: &E, miss!(CommonStyleAffectingAttributes) } - if !have_same_presentational_hints(element, candidate) { + if !have_same_presentational_hints(element, candidate_element) { miss!(PresHints) } - if !match_same_sibling_affecting_rules(element, candidate, shared_context) { + if !match_same_sibling_affecting_rules(element, + candidate_element, + shared_context) { miss!(SiblingRules) } - if !match_same_not_common_style_affecting_attributes_rules(element, candidate, shared_context) { + if !match_same_not_common_style_affecting_attributes_rules(element, + candidate_element, + shared_context) { miss!(NonCommonAttrRules) } - let candidate_node = candidate.as_node(); - let candidate_style = candidate_node.borrow_data().unwrap().style.as_ref().unwrap().clone(); - - Ok(candidate_style) + Ok(candidate.style.clone()) } fn have_same_common_style_affecting_attributes(element: &E, - candidate: &E) -> bool { + candidate: &StyleSharingCandidate) -> bool { // XXX probably could do something smarter. Also, the cache should // precompute this for the parent. Just experimenting now though. create_common_style_affecting_attributes_from_element(element) == - create_common_style_affecting_attributes_from_element(candidate) + candidate.common_style_affecting_attributes } fn have_same_presentational_hints(element: &E, candidate: &E) -> bool { - let mut first = vec![]; + let mut first = ForgetfulSink::new(); element.synthesize_presentational_hints_for_legacy_attributes(&mut first); if cfg!(debug_assertions) { let mut second = vec![]; @@ -361,14 +382,15 @@ fn have_same_class(element: &E, candidate: &E) -> bool { first == second } +// TODO: These re-match the candidate every time, which is suboptimal. +#[inline] fn match_same_not_common_style_affecting_attributes_rules(element: &E, candidate: &E, ctx: &SharedStyleContext) -> bool { - // XXX Same here, could store in the cache an index with the matched rules, - // for example. ctx.stylist.match_same_not_common_style_affecting_attributes_rules(element, candidate) } +#[inline] fn match_same_sibling_affecting_rules(element: &E, candidate: &E, ctx: &SharedStyleContext) -> bool { @@ -384,14 +406,13 @@ impl StyleSharingCandidateCache { } } - pub fn iter(&self) -> Iter<(UnsafeNode, ())> { + fn iter(&self) -> Iter<(StyleSharingCandidate, ())> { self.cache.iter() } pub fn insert_if_possible(&mut self, element: &E, relations: StyleRelations) { - use selectors::matching::*; // For flags use traversal::relations_are_shareable; let parent = match element.parent_element() { @@ -409,11 +430,30 @@ impl StyleSharingCandidateCache { return; } - // XXX check transitions/animations and reject! + let node = element.as_node(); + let data = node.borrow_data().unwrap(); + let style = data.style.as_ref().unwrap(); + + let box_style = style.get_box(); + if box_style.transition_property_count() > 0 { + debug!("Failing to insert to the cache: transitions"); + return; + } + + if box_style.animation_name_count() > 0 { + debug!("Failing to insert to the cache: animations"); + return; + } + debug!("Inserting into cache: {:?} with parent {:?}", element.as_node().to_unsafe(), parent.as_node().to_unsafe()); - self.cache.insert(element.as_node().to_unsafe(), ()) + self.cache.insert(StyleSharingCandidate { + node: node.to_unsafe(), + style: style.clone(), + common_style_affecting_attributes: + create_common_style_affecting_attributes_from_element(element), + }, ()); } pub fn touch(&mut self, index: usize) { @@ -580,17 +620,16 @@ trait PrivateElementMatchMethods: TElement { fn share_style_with_candidate_if_possible(&self, parent_node: Self::ConcreteNode, shared_context: &SharedStyleContext, - candidate: &Self) - -> Option> { + candidate: &StyleSharingCandidate) + -> Result, CacheMiss> { debug_assert!(parent_node.is_element()); - match element_matches_candidate(self, candidate, shared_context) { - Ok(cv) => Some(cv), - Err(error) => { - debug!("Cache miss: {:?}", error); - None - } - } + let candidate_element = unsafe { + Self::ConcreteNode::from_unsafe(&candidate.node).as_element().unwrap() + }; + + element_matches_candidate(self, candidate, &candidate_element, + shared_context) } } @@ -658,40 +697,55 @@ pub trait ElementMatchMethods : TElement { _ => return StyleSharingResult::CannotShare, }; - let iter = style_sharing_candidate_cache.iter().map(|&(unsafe_node, ())| { - Self::ConcreteNode::from_unsafe(&unsafe_node).as_element().unwrap() - }); + for (i, &(ref candidate, ())) in style_sharing_candidate_cache.iter().enumerate() { + let sharing_result = self.share_style_with_candidate_if_possible(parent, + shared_context, + candidate); + match sharing_result { + Ok(shared_style) => { + // Yay, cache hit. Share the style. + let node = self.as_node(); + let style = &mut node.mutate_data().unwrap().style; - for (i, candidate) in iter.enumerate() { - if let Some(shared_style) = self.share_style_with_candidate_if_possible(parent, - shared_context, - &candidate) { - // Yay, cache hit. Share the style. - let node = self.as_node(); + // TODO: add the display: none optimisation here too! Even + // better, factor it out/make it a bit more generic so Gecko + // can decide more easily if it knows that it's a child of + // replaced content, or similar stuff! + let damage = + match node.existing_style_for_restyle_damage((*style).as_ref(), None) { + Some(ref source) => { + <::ConcreteNode as TNode> + ::ConcreteRestyleDamage::compute(source, &shared_style) + } + None => { + <::ConcreteNode as TNode> + ::ConcreteRestyleDamage::rebuild_and_reflow() + } + }; - let style = &mut node.mutate_data().unwrap().style; - - let damage = - match node.existing_style_for_restyle_damage((*style).as_ref(), None) { - Some(ref source) => { - <::ConcreteNode as TNode> - ::ConcreteRestyleDamage::compute(source, &shared_style) - } - None => { - <::ConcreteNode as TNode> - ::ConcreteRestyleDamage::rebuild_and_reflow() - } + let restyle_result = if shared_style.get_box().clone_display() == display::T::none { + RestyleResult::Stop + } else { + RestyleResult::Continue }; - let restyle_result = if shared_style.get_box().clone_display() == display::T::none { - RestyleResult::Stop - } else { - RestyleResult::Continue - }; + *style = Some(shared_style); - *style = Some(shared_style); - - return StyleSharingResult::StyleWasShared(i, damage, restyle_result) + return StyleSharingResult::StyleWasShared(i, damage, restyle_result) + } + Err(miss) => { + // Cache miss, let's see what kind of failure to decide + // whether we keep trying or not. + match miss { + // Too expensive failure, give up, we don't want another + // one of these. + CacheMiss::CommonStyleAffectingAttributes | + CacheMiss::PresHints | + CacheMiss::SiblingRules | + CacheMiss::NonCommonAttrRules => break, + _ => {} + } + } } } diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 506e9e018f3..490e6a2f10e 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -11,7 +11,6 @@ use matching::{ApplicableDeclarations, ElementMatchMethods, MatchMethods, StyleS use selectors::bloom::BloomFilter; use selectors::matching::StyleRelations; use std::cell::RefCell; -use std::sync::Arc; use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering}; use tid::tid; use util::opts; @@ -367,8 +366,8 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C, // Perform the CSS cascade. unsafe { restyle_result = node.cascade_node(context, - parent_opt, - &applicable_declarations); + parent_opt, + &applicable_declarations); } // Add ourselves to the LRU cache.