From 0869a243b9da98d1f06e8af2cba82d8ae2e2553c Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 5 Jun 2017 11:05:00 -0700 Subject: [PATCH] servo: Merge #17055 - Allow style sharing for elements with ids as long as the ID is not being used for styling (from bzbarsky:sharing-across-ids); r=emilio --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix part of the problem we're seeing in https://bugzilla.mozilla.org/show_bug.cgi?id=1367862 - [ ] There are tests for these changes OR - [X] These changes do not require tests because the only impact is on performance and memory. Source-Repo: https://github.com/servo/servo Source-Revision: 429db6a3c617ba147312e245f9a69d170c50d43f --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : cdbb999e4c21ba2e53d6e312dc91de7daca0b3f0 --- servo/components/selectors/matching.rs | 9 +- servo/components/selectors/visitor.rs | 2 +- servo/components/style/bloom.rs | 10 +- servo/components/style/selector_map.rs | 6 ++ servo/components/style/sharing/checks.rs | 35 ++++++- servo/components/style/sharing/mod.rs | 121 ++++++++++++++++++++--- servo/components/style/stylist.rs | 60 ++++++++++- servo/components/style/traversal.rs | 25 +++-- servo/tests/unit/style/stylist.rs | 16 ++- 9 files changed, 240 insertions(+), 44 deletions(-) diff --git a/servo/components/selectors/matching.rs b/servo/components/selectors/matching.rs index 07050d8c91bf..444d24a7d1c7 100644 --- a/servo/components/selectors/matching.rs +++ b/servo/components/selectors/matching.rs @@ -21,13 +21,11 @@ bitflags! { /// This is used to implement efficient sharing. #[derive(Default)] pub flags StyleRelations: usize { - /// Whether this element is affected by an ID selector. - const AFFECTED_BY_ID_SELECTOR = 1 << 0, /// Whether this element is affected by presentational hints. This is /// computed externally (that is, in Servo). - const AFFECTED_BY_PRESENTATIONAL_HINTS = 1 << 1, + const AFFECTED_BY_PRESENTATIONAL_HINTS = 1 << 0, /// Whether this element has pseudo-element styles. Computed externally. - const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 2, + const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 1, } } @@ -539,8 +537,7 @@ fn matches_simple_selector( } // TODO: case-sensitivity depends on the document type and quirks mode Component::ID(ref id) => { - relation_if!(element.get_id().map_or(false, |attr| attr == *id), - AFFECTED_BY_ID_SELECTOR) + element.get_id().map_or(false, |attr| attr == *id) } Component::Class(ref class) => { element.has_class(class) diff --git a/servo/components/selectors/visitor.rs b/servo/components/selectors/visitor.rs index 07f121fe82f2..51d335c6a742 100644 --- a/servo/components/selectors/visitor.rs +++ b/servo/components/selectors/visitor.rs @@ -37,7 +37,7 @@ pub trait SelectorVisitor { /// Visits a complex selector. /// /// Gets the combinator to the right of the selector, or `None` if the - /// selector is the leftmost one. + /// selector is the rightmost one. fn visit_complex_selector(&mut self, _: SelectorIter, _combinator_to_right: Option) diff --git a/servo/components/style/bloom.rs b/servo/components/style/bloom.rs index f5df67e6a3f7..8ffe2d2a9689 100644 --- a/servo/components/style/bloom.rs +++ b/servo/components/style/bloom.rs @@ -158,6 +158,14 @@ impl StyleBloom { } } + /// Get the element that represents the chain of things inserted + /// into the filter right now. That chain is the given element + /// (if any) and its ancestors. + #[inline] + pub fn current_parent(&self) -> Option { + self.elements.last().map(|el| **el) + } + /// Insert the parents of an element in the bloom filter, trying to recover /// the filter if the last element inserted doesn't match. /// @@ -185,7 +193,7 @@ impl StyleBloom { } }; - if self.elements.last().map(|el| **el) == Some(parent_element) { + if self.current_parent() == Some(parent_element) { // Ta da, cache hit, we're all done. return; } diff --git a/servo/components/style/selector_map.rs b/servo/components/style/selector_map.rs index 497baf2f9144..846d79728116 100644 --- a/servo/components/style/selector_map.rs +++ b/servo/components/style/selector_map.rs @@ -159,6 +159,12 @@ impl SelectorMap { |block| (block.specificity, block.source_order)); } + /// Check whether we have rules for the given id + #[inline] + pub fn has_rules_for_id(&self, id: &Atom) -> bool { + self.id_hash.get(id).is_some() + } + /// Append to `rule_list` all universal Rules (rules with selector `*|*`) in /// `self` sorted by specificity and source order. pub fn get_universal_rules(&self, diff --git a/servo/components/style/sharing/checks.rs b/servo/components/style/sharing/checks.rs index 40b5140809f4..bfd1778484f5 100644 --- a/servo/components/style/sharing/checks.rs +++ b/servo/components/style/sharing/checks.rs @@ -6,10 +6,11 @@ //! quickly whether it's worth to share style, and whether two different //! elements can indeed share the same style. +use Atom; +use bloom::StyleBloom; use context::{SelectorFlagsMap, SharedStyleContext}; use dom::TElement; use element_state::*; -use selectors::bloom::BloomFilter; use selectors::matching::StyleRelations; use sharing::{StyleSharingCandidate, StyleSharingTarget}; use stylearc::Arc; @@ -20,8 +21,9 @@ use stylearc::Arc; #[inline] pub fn relations_are_shareable(relations: &StyleRelations) -> bool { use selectors::matching::*; - !relations.intersects(AFFECTED_BY_ID_SELECTOR | - AFFECTED_BY_PSEUDO_ELEMENTS) + // If we start sharing things that are AFFECTED_BY_PSEUDO_ELEMENTS, we need + // to track revalidation selectors on a per-pseudo-element basis. + !relations.intersects(AFFECTED_BY_PSEUDO_ELEMENTS) } /// Whether, given two elements, they have pointer-equal computed values. @@ -102,7 +104,7 @@ pub fn have_same_state_ignoring_visitedness(element: E, pub fn revalidate(target: &mut StyleSharingTarget, candidate: &mut StyleSharingCandidate, shared_context: &SharedStyleContext, - bloom: &BloomFilter, + bloom: &StyleBloom, selector_flags_map: &mut SelectorFlagsMap) -> bool where E: TElement, @@ -121,3 +123,28 @@ pub fn revalidate(target: &mut StyleSharingTarget, for_element == for_candidate } + +/// Checks whether we might have rules for either of the two ids. +#[inline] +pub fn may_have_rules_for_ids(shared_context: &SharedStyleContext, + element_id: Option<&Atom>, + candidate_id: Option<&Atom>) -> bool +{ + // We shouldn't be called unless the ids are different. + debug_assert!(element_id.is_some() || candidate_id.is_some()); + let stylist = &shared_context.stylist; + + let may_have_rules_for_element = match element_id { + Some(id) => stylist.may_have_rules_for_id(id), + None => false + }; + + if may_have_rules_for_element { + return true; + } + + match candidate_id { + Some(id) => stylist.may_have_rules_for_id(id), + None => false + } +} diff --git a/servo/components/style/sharing/mod.rs b/servo/components/style/sharing/mod.rs index c72344be2010..a6598ec67691 100644 --- a/servo/components/style/sharing/mod.rs +++ b/servo/components/style/sharing/mod.rs @@ -4,16 +4,77 @@ //! Code related to the style sharing cache, an optimization that allows similar //! nodes to share style without having to run selector matching twice. +//! +//! The basic setup is as follows. We have an LRU cache of style sharing +//! candidates. When we try to style a target element, we first check whether +//! we can quickly determine that styles match something in this cache, and if +//! so we just use the cached style information. This check is done with a +//! StyleBloom filter set up for the target element, which may not be a correct +//! state for the cached candidate element if they're cousins instead of +//! siblings. +//! +//! The complicated part is determining that styles match. This is subject to +//! the following constraints: +//! +//! 1) The target and candidate must be inheriting the same styles. +//! 2) The target and candidate must have exactly the same rules matching them. +//! 3) The target and candidate must have exactly the same non-selector-based +//! style information (inline styles, presentation hints). +//! 4) The target and candidate must have exactly the same rules matching their +//! pseudo-elements, because an element's style data points to the style +//! data for its pseudo-elements. +//! +//! These constraints are satisfied in the following ways: +//! +//! * We check that the parents of the target and the candidate have the same +//! computed style. This addresses constraint 1. +//! +//! * We check that the target and candidate have the same inline style and +//! presentation hint declarations. This addresses constraint 3. +//! +//! * We ensure that elements that have pseudo-element styles are not inserted +//! into the cache. This partially addresses constraint 4. +//! +//! * We ensure that a target matches a candidate only if they have the same +//! matching result for all selectors that target either elements or the +//! originating elements of pseudo-elements. This addresses the second half +//! of constraint 4 (because it prevents a target that has pseudo-element +//! styles from matching any candidate) as well as constraint 2. +//! +//! The actual checks that ensure that elements match the same rules are +//! conceptually split up into two pieces. First, we do various checks on +//! elements that make sure that the set of possible rules in all selector maps +//! in the stylist (for normal styling and for pseudo-elements) that might match +//! the two elements is the same. For example, we enforce that the target and +//! candidate must have the same localname and namespace. Second, we have a +//! selector map of "revalidation selectors" that the stylist maintains that we +//! actually match against the target and candidate and then check whether the +//! two sets of results were the same. Due to the up-front selector map checks, +//! we know that the target and candidate will be matched against the same exact +//! set of revalidation selectors, so the match result arrays can be compared +//! directly. +//! +//! It's very important that a selector be added to the set of revalidation +//! selectors any time there are two elements that could pass all the up-front +//! checks but match differently against some ComplexSelector in the selector. +//! If that happens, then they can have descendants that might themselves pass +//! the up-front checks but would have different matching results for the +//! selector in question. In this case, "descendants" includes pseudo-elements, +//! so there is a single selector map of revalidation selectors that includes +//! both selectors targeting element and selectors targeting pseudo-elements. +//! This relies on matching an element against a pseudo-element-targeting +//! selector being a sensible operation that will effectively check whether that +//! element is a matching originating element for the selector. use Atom; use bit_vec::BitVec; +use bloom::StyleBloom; use cache::{LRUCache, LRUCacheMutIterator}; use context::{SelectorFlagsMap, SharedStyleContext, StyleContext}; use data::{ComputedStyle, ElementData, ElementStyles}; use dom::{TElement, SendElement}; use matching::{ChildCascadeRequirement, MatchMethods}; use properties::ComputedValues; -use selectors::bloom::BloomFilter; use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode, StyleRelations}; use smallvec::SmallVec; use std::mem; @@ -95,19 +156,40 @@ impl ValidationData { } /// Computes the revalidation results if needed, and returns it. + /// Inline so we know at compile time what bloom_known_valid is. + #[inline] fn revalidation_match_results( &mut self, element: E, stylist: &Stylist, - bloom: &BloomFilter, + bloom: &StyleBloom, + bloom_known_valid: bool, flags_setter: &mut F ) -> &BitVec where E: TElement, F: FnMut(&E, ElementSelectorFlags), { if self.revalidation_match_results.is_none() { + // The bloom filter may already be set up for our element. + // If it is, use it. If not, we must be in a candidate + // (i.e. something in the cache), and the element is one + // of our cousins, not a sibling. In that case, we'll + // just do revalidation selector matching without a bloom + // filter, to avoid thrashing the filter. + let bloom_to_use = if bloom_known_valid { + debug_assert_eq!(bloom.current_parent(), + element.parent_element()); + Some(bloom.filter()) + } else { + if bloom.current_parent() == element.parent_element() { + Some(bloom.filter()) + } else { + None + } + }; self.revalidation_match_results = - Some(stylist.match_revalidation_selectors(&element, bloom, + Some(stylist.match_revalidation_selectors(&element, + bloom_to_use, flags_setter)); } @@ -149,16 +231,18 @@ impl StyleSharingCandidate { self.validation_data.pres_hints(*self.element) } - /// Get the classlist of this candidate. + /// Compute the bit vector of revalidation selector match results + /// for this candidate. fn revalidation_match_results( &mut self, stylist: &Stylist, - bloom: &BloomFilter, + bloom: &StyleBloom, ) -> &BitVec { self.validation_data.revalidation_match_results( *self.element, stylist, bloom, + /* bloom_known_valid = */ false, &mut |_, _| {}) } } @@ -204,7 +288,7 @@ impl StyleSharingTarget { fn revalidation_match_results( &mut self, stylist: &Stylist, - bloom: &BloomFilter, + bloom: &StyleBloom, selector_flags_map: &mut SelectorFlagsMap ) -> &BitVec { // It's important to set the selector flags. Otherwise, if we succeed in @@ -231,6 +315,7 @@ impl StyleSharingTarget { self.element, stylist, bloom, + /* bloom_known_valid = */ true, &mut set_selector_flags) } @@ -243,7 +328,10 @@ impl StyleSharingTarget { { let shared_context = &context.shared; let selector_flags_map = &mut context.thread_local.selector_flags; - let bloom_filter = context.thread_local.bloom_filter.filter(); + let bloom_filter = &context.thread_local.bloom_filter; + + debug_assert_eq!(bloom_filter.current_parent(), + self.element.parent_element()); let result = context.thread_local .style_sharing_candidate_cache @@ -400,7 +488,7 @@ impl StyleSharingCandidateCache { &mut self, shared_context: &SharedStyleContext, selector_flags_map: &mut SelectorFlagsMap, - bloom_filter: &BloomFilter, + bloom_filter: &StyleBloom, target: &mut StyleSharingTarget, data: &mut ElementData ) -> StyleSharingResult { @@ -421,11 +509,6 @@ impl StyleSharingCandidateCache { return StyleSharingResult::CannotShare; } - if target.get_id().is_some() { - debug!("{:?} Cannot share style: element has id", target.element); - return StyleSharingResult::CannotShare - } - let mut should_clear_cache = false; for (i, candidate) in self.iter_mut().enumerate() { let sharing_result = @@ -498,7 +581,7 @@ impl StyleSharingCandidateCache { fn test_candidate(target: &mut StyleSharingTarget, candidate: &mut StyleSharingCandidate, shared: &SharedStyleContext, - bloom: &BloomFilter, + bloom: &StyleBloom, selector_flags_map: &mut SelectorFlagsMap) -> Result { macro_rules! miss { @@ -544,8 +627,14 @@ impl StyleSharingCandidateCache { miss!(State) } - if target.get_id() != candidate.element.get_id() { - miss!(IdAttr) + let element_id = target.element.get_id(); + let candidate_id = candidate.element.get_id(); + if element_id != candidate_id { + // It's possible that there are no styles for either id. + if checks::may_have_rules_for_ids(shared, element_id.as_ref(), + candidate_id.as_ref()) { + miss!(IdAttr) + } } if !checks::have_same_style_attribute(target, candidate) { diff --git a/servo/components/style/stylist.rs b/servo/components/style/stylist.rs index 3a5a2489522f..5df0bf2717ba 100644 --- a/servo/components/style/stylist.rs +++ b/servo/components/style/stylist.rs @@ -146,6 +146,15 @@ pub struct Stylist { /// when an irrelevant element state bit changes. state_dependencies: ElementState, + /// The ids that appear in the rightmost complex selector of selectors (and + /// hence in our selector maps). Used to determine when sharing styles is + /// safe: we disallow style sharing for elements whose id matches this + /// filter, and hence might be in one of our selector maps. + /// + /// FIXME(bz): This doesn't really need to be a counting Blooom filter. + #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] + mapped_ids: BloomFilter, + /// Selectors that require explicit cache revalidation (i.e. which depend /// on state that is not otherwise visible to the cache, like attributes or /// tree-structural state like child index and pseudos). @@ -249,6 +258,7 @@ impl Stylist { attribute_dependencies: BloomFilter::new(), style_attribute_dependency: false, state_dependencies: ElementState::empty(), + mapped_ids: BloomFilter::new(), selectors_for_cache_revalidation: SelectorMap::new(), num_selectors: 0, num_declarations: 0, @@ -323,6 +333,7 @@ impl Stylist { self.attribute_dependencies.clear(); self.style_attribute_dependency = false; self.state_dependencies = ElementState::empty(); + self.mapped_ids.clear(); self.selectors_for_cache_revalidation = SelectorMap::new(); self.num_selectors = 0; self.num_declarations = 0; @@ -468,6 +479,9 @@ impl Stylist { style_attribute_dependency: &mut self.style_attribute_dependency, state_dependencies: &mut self.state_dependencies, }); + selector.visit(&mut MappedIdVisitor { + mapped_ids: &mut self.mapped_ids, + }); } self.rules_source_order += 1; } @@ -1069,6 +1083,13 @@ impl Stylist { debug!("push_applicable_declarations: shareable: {:?}", context.relations); } + /// Given an id, returns whether there might be any rules for that id in any + /// of our rule maps. + #[inline] + pub fn may_have_rules_for_id(&self, id: &Atom) -> bool { + self.mapped_ids.might_contain(id) + } + /// Return whether the device is dirty, that is, whether the screen size or /// media type have changed (for now). #[inline] @@ -1092,7 +1113,7 @@ impl Stylist { /// revalidation selectors. pub fn match_revalidation_selectors(&self, element: &E, - bloom: &BloomFilter, + bloom: Option<&BloomFilter>, flags_setter: &mut F) -> BitVec where E: TElement, @@ -1101,7 +1122,7 @@ impl Stylist { // NB: `MatchingMode` doesn't really matter, given we don't share style // between pseudos. let mut matching_context = - MatchingContext::new(MatchingMode::Normal, Some(bloom)); + MatchingContext::new(MatchingMode::Normal, bloom); // Note that, by the time we're revalidating, we're guaranteed that the // candidate and the entry have the same id, classes, and local name. @@ -1232,6 +1253,37 @@ impl<'a> SelectorVisitor for AttributeAndStateDependencyVisitor<'a> { } } +/// Visitor to collect ids that appear in the rightmost portion of selectors. +struct MappedIdVisitor<'a> { + mapped_ids: &'a mut BloomFilter, +} + +impl<'a> SelectorVisitor for MappedIdVisitor<'a> { + type Impl = SelectorImpl; + + /// We just want to insert all the ids we find into mapped_ids. + fn visit_simple_selector(&mut self, s: &Component) -> bool { + if let Component::ID(ref id) = *s { + self.mapped_ids.insert(id); + } + true + } + + /// We want to stop as soon as we've moved off the rightmost ComplexSelector + /// that is not a psedo-element. That can be detected by a + /// visit_complex_selector call with a combinator other than None and + /// PseudoElement. Importantly, this call happens before we visit any of + /// the simple selectors in that ComplexSelector. + fn visit_complex_selector(&mut self, + _: SelectorIter, + combinator: Option) -> bool { + match combinator { + None | Some(Combinator::PseudoElement) => true, + _ => false, + } + } +} + /// Visitor determine whether a selector requires cache revalidation. /// /// Note that we just check simple selectors and eagerly return when the first @@ -1282,6 +1334,10 @@ impl SelectorVisitor for RevalidationVisitor { Component::AttributeInNoNamespace { .. } | Component::AttributeOther(_) | Component::Empty | + // FIXME(bz) We really only want to do this for some cases of id + // selectors. See + // https://bugzilla.mozilla.org/show_bug.cgi?id=1369611 + Component::ID(_) | Component::FirstChild | Component::LastChild | Component::OnlyChild | diff --git a/servo/components/style/traversal.rs b/servo/components/style/traversal.rs index 820bc41996ec..536ddf69f87e 100644 --- a/servo/components/style/traversal.rs +++ b/servo/components/style/traversal.rs @@ -799,19 +799,6 @@ fn compute_style(_traversal: &D, context.thread_local.statistics.elements_styled += 1; let kind = data.restyle_kind(); - // First, try the style sharing cache. If we get a match we can skip the rest - // of the work. - if let MatchAndCascade = kind { - let target = StyleSharingTarget::new(element); - let sharing_result = target.share_style_if_possible(context, data); - - if let StyleWasShared(index, had_damage) = sharing_result { - context.thread_local.statistics.styles_shared += 1; - context.thread_local.style_sharing_candidate_cache.touch(index); - return had_damage; - } - } - match kind { MatchAndCascade => { // Ensure the bloom filter is up to date. @@ -820,6 +807,18 @@ fn compute_style(_traversal: &D, traversal_data.current_dom_depth); context.thread_local.bloom_filter.assert_complete(element); + + // Now that our bloom filter is set up, try the style sharing + // cache. If we get a match we can skip the rest of the work. + let target = StyleSharingTarget::new(element); + let sharing_result = target.share_style_if_possible(context, data); + + if let StyleWasShared(index, had_damage) = sharing_result { + context.thread_local.statistics.styles_shared += 1; + context.thread_local.style_sharing_candidate_cache.touch(index); + return had_damage; + } + context.thread_local.statistics.elements_matched += 1; // Perform the matching and cascading. diff --git a/servo/tests/unit/style/stylist.rs b/servo/tests/unit/style/stylist.rs index 3d307d230578..afd967338f98 100644 --- a/servo/tests/unit/style/stylist.rs +++ b/servo/tests/unit/style/stylist.rs @@ -77,11 +77,19 @@ fn test_revalidation_selectors() { let test = parse_selectors(&[ // Not revalidation selectors. "div", - "#bar", "div:not(.foo)", "div span", "div > span", + // ID selectors. + "#foo1", // FIXME(bz) This one should not be a revalidation + // selector once we fix + // https://bugzilla.mozilla.org/show_bug.cgi?id=1369611 + "#foo2::before", + "#foo3 > span", + "#foo1 > span", // FIXME(bz) This one should not be a + // revalidation selector either. + // Attribute selectors. "div[foo]", "div:not([foo])", @@ -122,6 +130,12 @@ fn test_revalidation_selectors() { .collect::>(); let reference = parse_selectors(&[ + // ID selectors. + "#foo1", + "#foo2::before", + "#foo3 > span", + "#foo1 > span", + // Attribute selectors. "div[foo]", "div:not([foo])",