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

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because the only impact is on performance and memory.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

@@ -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<E, F>(
         }
         // 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)

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<Self::Impl>,
                               _combinator_to_right: Option<Combinator>)

servo/components/style/bloom.rs

@@ -158,6 +158,14 @@ impl<E: TElement> StyleBloom<E> {
         }
     }
 
+    /// 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<E> {
+        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<E: TElement> StyleBloom<E> {
             }
         };
 
-        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;
         }

servo/components/style/selector_map.rs

@@ -159,6 +159,12 @@ impl SelectorMap<Rule> {
                     |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,

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<E>(element: E,
 pub fn revalidate<E>(target: &mut StyleSharingTarget<E>,
                      candidate: &mut StyleSharingCandidate<E>,
                      shared_context: &SharedStyleContext,
-                     bloom: &BloomFilter,
+                     bloom: &StyleBloom<E>,
                      selector_flags_map: &mut SelectorFlagsMap<E>)
                      -> bool
     where E: TElement,
@@ -121,3 +123,28 @@ pub fn revalidate<E>(target: &mut StyleSharingTarget<E>,
 
     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
+    }
+}

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<E, F>(
         &mut self,
         element: E,
         stylist: &Stylist,
-        bloom: &BloomFilter,
+        bloom: &StyleBloom<E>,
+        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<E: TElement> StyleSharingCandidate<E> {
         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<E>,
     ) -> &BitVec {
         self.validation_data.revalidation_match_results(
             *self.element,
             stylist,
             bloom,
+            /* bloom_known_valid = */ false,
             &mut |_, _| {})
     }
 }
@@ -204,7 +288,7 @@ impl<E: TElement> StyleSharingTarget<E> {
     fn revalidation_match_results(
         &mut self,
         stylist: &Stylist,
-        bloom: &BloomFilter,
+        bloom: &StyleBloom<E>,
         selector_flags_map: &mut SelectorFlagsMap<E>
     ) -> &BitVec {
         // It's important to set the selector flags. Otherwise, if we succeed in
@@ -231,6 +315,7 @@ impl<E: TElement> StyleSharingTarget<E> {
             self.element,
             stylist,
             bloom,
+            /* bloom_known_valid = */ true,
             &mut set_selector_flags)
     }
 
@@ -243,7 +328,10 @@ impl<E: TElement> StyleSharingTarget<E> {
     {
         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<E: TElement> StyleSharingCandidateCache<E> {
         &mut self,
         shared_context: &SharedStyleContext,
         selector_flags_map: &mut SelectorFlagsMap<E>,
-        bloom_filter: &BloomFilter,
+        bloom_filter: &StyleBloom<E>,
         target: &mut StyleSharingTarget<E>,
         data: &mut ElementData
     ) -> StyleSharingResult {
@@ -421,11 +509,6 @@ impl<E: TElement> StyleSharingCandidateCache<E> {
             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<E: TElement> StyleSharingCandidateCache<E> {
     fn test_candidate(target: &mut StyleSharingTarget<E>,
                       candidate: &mut StyleSharingCandidate<E>,
                       shared: &SharedStyleContext,
-                      bloom: &BloomFilter,
+                      bloom: &StyleBloom<E>,
                       selector_flags_map: &mut SelectorFlagsMap<E>)
                       -> Result<ComputedStyle, CacheMiss> {
         macro_rules! miss {
@@ -544,8 +627,14 @@ impl<E: TElement> StyleSharingCandidateCache<E> {
             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) {

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<E, F>(&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<SelectorImpl>) -> 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<SelectorImpl>,
+                              combinator: Option<Combinator>) -> 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 |

servo/components/style/traversal.rs

@@ -799,19 +799,6 @@ fn compute_style<E, D>(_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<E, D>(_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.

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::<Vec<_>>();
 
     let reference = parse_selectors(&[
+        // ID selectors.
+        "#foo1",
+        "#foo2::before",
+        "#foo3 > span",
+        "#foo1 > span",
+
         // Attribute selectors.
         "div[foo]",
         "div:not([foo])",