servo: Merge #17245 - Share styles for elements with eager pseudo-elements attached to them (from bzbarsky:share-pseudo-styles); r=emilio
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 08 Jun 2017 15:25:10 -0700
changeset 411206 c608727501c7c6c4c11904b68a3b6963a23af464
parent 411205 ddabdf4ac22249028c167b015610ee24d4b69d62
child 411207 62e44a82730804257046e8fe9aafd8d53652d6ea
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #17245 - Share styles for elements with eager pseudo-elements attached to them (from bzbarsky:share-pseudo-styles); 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 https://bugzilla.mozilla.org/show_bug.cgi?id=1329361 <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- 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: 310408a82851d05db3b310ac5b9cdf49a33e1990
servo/components/style/data.rs
servo/components/style/sharing/checks.rs
servo/components/style/sharing/mod.rs
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -289,16 +289,36 @@ impl EagerPseudoStyles {
             VisitedHandlingMode::AllLinksUnvisited => {
                 self.remove_unvisited_rules(&pseudo)
             },
             VisitedHandlingMode::RelevantLinkVisited => {
                 self.remove_visited_rules(&pseudo)
             },
         }
     }
+
+    /// Returns whether this EagerPseudoStyles has the same set of
+    /// pseudos as the given one.
+    pub fn has_same_pseudos_as(&self, other: &EagerPseudoStyles) -> bool {
+        // We could probably just compare self.keys() to other.keys(), but that
+        // seems like it'll involve a bunch more moving stuff around and
+        // whatnot.
+        match (&self.0, &other.0) {
+            (&Some(ref our_arr), &Some(ref other_arr)) => {
+                for i in 0..EAGER_PSEUDO_COUNT {
+                    if our_arr[i].is_some() != other_arr[i].is_some() {
+                        return false
+                    }
+                }
+                true
+            },
+            (&None, &None) => true,
+            _ => false,
+        }
+    }
 }
 
 /// The styles associated with a node, including the styles for any
 /// pseudo-elements.
 #[derive(Clone, Debug)]
 pub struct ElementStyles {
     /// The element's style.
     pub primary: ComputedStyle,
--- a/servo/components/style/sharing/checks.rs
+++ b/servo/components/style/sharing/checks.rs
@@ -6,31 +6,19 @@
 //! 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::matching::StyleRelations;
 use sharing::{StyleSharingCandidate, StyleSharingTarget};
 use stylearc::Arc;
 
-/// Determines, based on the results of selector matching, whether it's worth to
-/// try to share style with this element, that is, to try to insert the element
-/// in the chache.
-#[inline]
-pub fn relations_are_shareable(relations: &StyleRelations) -> bool {
-    use selectors::matching::*;
-    // 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.
 ///
 /// Both elements need to be styled already.
 ///
 /// This is used to know whether we can share style across cousins (if the two
 /// parents have the same style).
 pub fn same_computed_values<E>(first: Option<E>, second: Option<E>) -> bool
     where E: TElement,
--- a/servo/components/style/sharing/mod.rs
+++ b/servo/components/style/sharing/mod.rs
@@ -27,24 +27,22 @@
 //! 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.
+//!   originating elements of pseudo-elements.  This addresses constraint 4
+//!   (because it prevents a target that has pseudo-element styles from matching
+//!   a candidate that has different pseudo-element styles) 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
@@ -66,20 +64,21 @@
 //! selectors are effectively stripped off, so that matching them all against
 //! elements makes sense.
 
 use Atom;
 use bit_vec::BitVec;
 use bloom::StyleBloom;
 use cache::{LRUCache, LRUCacheMutIterator};
 use context::{SelectorFlagsMap, SharedStyleContext, StyleContext};
-use data::{ComputedStyle, ElementData, ElementStyles};
+use data::{ElementData, ElementStyles};
 use dom::{TElement, SendElement};
 use matching::{ChildCascadeRequirement, MatchMethods};
 use properties::ComputedValues;
+use selector_parser::RestyleDamage;
 use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode, StyleRelations};
 use smallvec::SmallVec;
 use std::mem;
 use std::ops::Deref;
 use stylist::{ApplicableDeclarationBlock, Stylist};
 
 mod checks;
 
@@ -354,16 +353,71 @@ impl<E: TElement> StyleSharingTarget<E> 
                                      &mut self,
                                      data);
 
 
         context.thread_local.current_element_info.as_mut().unwrap().validation_data =
             self.validation_data.take();
         result
     }
+
+    fn accumulate_damage_when_sharing(&self,
+                                      shared_context: &SharedStyleContext,
+                                      shared_style: &ElementStyles,
+                                      data: &mut ElementData) -> ChildCascadeRequirement {
+        // Accumulate restyle damage for the case when our sharing
+        // target managed to share style.  This can come from several
+        // sources:
+        //
+        // 1) We matched a different set of eager pseudos (which
+        //    should cause a reconstruct).
+        // 2) We have restyle damage from the eager pseudo computed
+        //    styles.
+        // 3) We have restyle damage from our own computed styles.
+        if data.has_styles() {
+            // We used to have pseudos (because we had styles).
+            // Check for damage from the set of pseudos changing or
+            // pseudos being restyled.
+            let (styles, restyle_data) = data.styles_and_restyle_mut();
+            if let Some(restyle_data) = restyle_data {
+                let old_pseudos = &styles.pseudos;
+                let new_pseudos = &shared_style.pseudos;
+                if !old_pseudos.has_same_pseudos_as(new_pseudos) {
+                    restyle_data.damage |= RestyleDamage::reconstruct();
+                } else {
+                    // It's a bit unfortunate that we have to keep
+                    // mapping PseudoElements back to indices
+                    // here....
+                    for pseudo in old_pseudos.keys() {
+                        let old_values =
+                            old_pseudos.get(&pseudo).unwrap().values.as_ref().map(|v| &**v);
+                        let new_values =
+                            new_pseudos.get(&pseudo).unwrap().values();
+                        self.element.accumulate_damage(
+                            &shared_context,
+                            Some(restyle_data),
+                            old_values,
+                            new_values,
+                            Some(&pseudo)
+                        );
+                    }
+                }
+            }
+        }
+
+        let old_values = data.get_styles_mut()
+                             .and_then(|s| s.primary.values.take());
+        self.element.accumulate_damage(
+            &shared_context,
+            data.get_restyle_mut(),
+            old_values.as_ref().map(|v| &**v),
+            shared_style.primary.values(),
+            None
+        )
+    }
 }
 
 /// A cache miss result.
 #[derive(Clone, Debug)]
 pub enum CacheMiss {
     /// The parents don't match.
     Parent,
     /// One element was NAC, while the other wasn't.
@@ -456,21 +510,16 @@ impl<E: TElement> StyleSharingCandidateC
 
         if element.is_native_anonymous() {
             debug!("Failing to insert into the cache: NAC");
             return;
         }
 
         // These are things we don't check in the candidate match because they
         // are either uncommon or expensive.
-        if !checks::relations_are_shareable(&relations) {
-            debug!("Failing to insert to the cache: {:?}", relations);
-            return;
-        }
-
         let box_style = style.get_box();
         if box_style.specifies_transitions() {
             debug!("Failing to insert to the cache: transitions");
             return;
         }
 
         if box_style.specifies_animations() {
             debug!("Failing to insert to the cache: animations");
@@ -543,36 +592,23 @@ impl<E: TElement> StyleSharingCandidateC
                     bloom_filter,
                     selector_flags_map
                 );
 
             match sharing_result {
                 Ok(shared_style) => {
                     // Yay, cache hit. Share the style.
 
-                    // Accumulate restyle damage.
                     debug_assert_eq!(data.has_styles(), data.has_restyle());
-                    let old_values = data.get_styles_mut()
-                                         .and_then(|s| s.primary.values.take());
+
                     let child_cascade_requirement =
-                        target.accumulate_damage(
-                            &shared_context,
-                            data.get_restyle_mut(),
-                            old_values.as_ref().map(|v| &**v),
-                            shared_style.values(),
-                            None
-                        );
-
-                    // We never put elements with pseudo style into the style
-                    // sharing cache, so we can just mint an ElementStyles
-                    // directly here.
-                    //
-                    // See https://bugzilla.mozilla.org/show_bug.cgi?id=1329361
-                    let styles = ElementStyles::new(shared_style);
-                    data.set_styles(styles);
+                        target.accumulate_damage_when_sharing(shared_context,
+                                                              &shared_style,
+                                                              data);
+                    data.set_styles(shared_style);
 
                     return StyleSharingResult::StyleWasShared(i, child_cascade_requirement)
                 }
                 Err(miss) => {
                     debug!("Cache miss: {:?}", miss);
 
                     // Cache miss, let's see what kind of failure to decide
                     // whether we keep trying or not.
@@ -593,17 +629,17 @@ impl<E: TElement> StyleSharingCandidateC
         StyleSharingResult::CannotShare
     }
 
     fn test_candidate(target: &mut StyleSharingTarget<E>,
                       candidate: &mut StyleSharingCandidate<E>,
                       shared: &SharedStyleContext,
                       bloom: &StyleBloom<E>,
                       selector_flags_map: &mut SelectorFlagsMap<E>)
-                      -> Result<ComputedStyle, CacheMiss> {
+                      -> Result<ElementStyles, CacheMiss> {
         macro_rules! miss {
             ($miss: ident) => {
                 return Err(CacheMiss::$miss);
             }
         }
 
         // Check that we have the same parent, or at least the same pointer
         // identity for parent computed style. The latter check allows us to
@@ -669,11 +705,11 @@ impl<E: TElement> StyleSharingCandidateC
             miss!(Revalidation)
         }
 
         let data = candidate.element.borrow_data().unwrap();
         debug_assert!(target.has_current_styles(&data));
 
         debug!("Sharing style between {:?} and {:?}",
                target.element, candidate.element);
-        Ok(data.styles().primary.clone())
+        Ok(data.styles().clone())
     }
 }