servo: Merge #17110 - Improve Style Sharing (from bholley:better_style_sharing); r=emilio
authorBobby Holley <bobbyholley@gmail.com>
Wed, 31 May 2017 10:07:53 -0500
changeset 409704 09fc715942f2b303b1a1ed5f65e977e55b35b890
parent 409703 6a894530a99bfec36784225b0c6c54a0a85dca75
child 409705 46391a33a8f9efb4b29efd58087500ab63ca3fee
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 #17110 - Improve Style Sharing (from bholley:better_style_sharing); r=emilio Reviewed in: https://bugzilla.mozilla.org/show_bug.cgi?id=1368665 https://bugzilla.mozilla.org/show_bug.cgi?id=1368399 Source-Repo: https://github.com/servo/servo Source-Revision: 1b9cc2de3418f6dbe27102d02ac4d4fadb6cc643
servo/components/selectors/matching.rs
servo/components/style/context.rs
servo/components/style/sharing/checks.rs
servo/components/style/sharing/mod.rs
servo/components/style/stylist.rs
--- a/servo/components/selectors/matching.rs
+++ b/servo/components/selectors/matching.rs
@@ -18,24 +18,21 @@ bitflags! {
     /// Set of flags that determine the different kind of elements affected by
     /// the selector matching process.
     ///
     /// 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 has a style attribute. Computed
-        /// externally.
-        const AFFECTED_BY_STYLE_ATTRIBUTE = 1 << 1,
         /// Whether this element is affected by presentational hints. This is
         /// computed externally (that is, in Servo).
-        const AFFECTED_BY_PRESENTATIONAL_HINTS = 1 << 2,
+        const AFFECTED_BY_PRESENTATIONAL_HINTS = 1 << 1,
         /// Whether this element has pseudo-element styles. Computed externally.
-        const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 3,
+        const AFFECTED_BY_PSEUDO_ELEMENTS = 1 << 2,
     }
 }
 
 bitflags! {
     /// 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.
     pub flags ElementSelectorFlags: usize {
         /// When a child is added or removed from the parent, all the children
--- a/servo/components/style/context.rs
+++ b/servo/components/style/context.rs
@@ -458,17 +458,17 @@ impl<E: TElement> ThreadLocalStyleContex
     }
 
     /// Notes when the style system starts traversing an element.
     pub fn begin_element(&mut self, element: E, data: &ElementData) {
         debug_assert!(self.current_element_info.is_none());
         self.current_element_info = Some(CurrentElementInfo {
             element: element.as_node().opaque(),
             is_initial_style: !data.has_styles(),
-            validation_data: ValidationData::new(),
+            validation_data: ValidationData::default(),
             possibly_expired_animations: Vec::new(),
         });
     }
 
     /// Notes when the style system finishes traversing an element.
     pub fn end_element(&mut self, element: E) {
         debug_assert!(self.current_element_info.is_some());
         debug_assert!(self.current_element_info.as_ref().unwrap().element ==
--- a/servo/components/style/sharing/checks.rs
+++ b/servo/components/style/sharing/checks.rs
@@ -16,18 +16,17 @@ 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::*;
     !relations.intersects(AFFECTED_BY_ID_SELECTOR |
-                          AFFECTED_BY_PSEUDO_ELEMENTS |
-                          AFFECTED_BY_STYLE_ATTRIBUTE)
+                          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).
@@ -39,21 +38,31 @@ pub fn same_computed_values<E>(first: Op
         _ => return false,
     };
 
     let eq = Arc::ptr_eq(a.borrow_data().unwrap().styles().primary.values(),
                          b.borrow_data().unwrap().styles().primary.values());
     eq
 }
 
-/// Whether a given element has presentational hints.
-///
-/// We consider not worth to share style with an element that has presentational
-/// hints, both because implementing the code that compares that the hints are
-/// equal is somewhat annoying, and also because it'd be expensive enough.
+/// Whether two elements have the same same style attribute (by pointer identity).
+pub fn have_same_style_attribute<E>(
+    target: &mut StyleSharingTarget<E>,
+    candidate: &mut StyleSharingCandidate<E>
+) -> bool
+    where E: TElement,
+{
+    match (target.style_attribute(), candidate.style_attribute()) {
+        (None, None) => true,
+        (Some(_), None) | (None, Some(_)) => false,
+        (Some(a), Some(b)) => Arc::ptr_eq(a, b)
+    }
+}
+
+/// Whether two elements have the same same presentational attributes.
 pub fn have_same_presentational_hints<E>(
     target: &mut StyleSharingTarget<E>,
     candidate: &mut StyleSharingCandidate<E>
 ) -> bool
     where E: TElement,
 {
     target.pres_hints() == candidate.pres_hints()
 }
--- a/servo/components/style/sharing/mod.rs
+++ b/servo/components/style/sharing/mod.rs
@@ -11,16 +11,17 @@ use cache::{LRUCache, LRUCacheMutIterato
 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, StyleRelations};
 use smallvec::SmallVec;
+use std::mem;
 use std::ops::Deref;
 use stylist::{ApplicableDeclarationBlock, Stylist};
 
 mod checks;
 
 /// The amount of nodes that the style sharing candidate cache should hold at
 /// most.
 pub const STYLE_SHARING_CANDIDATE_CACHE_SIZE: usize = 8;
@@ -31,50 +32,36 @@ pub enum StyleSharingBehavior {
     /// Style sharing allowed.
     Allow,
     /// Style sharing disallowed.
     Disallow,
 }
 
 /// Some data we want to avoid recomputing all the time while trying to share
 /// style.
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub struct ValidationData {
     /// The class list of this element.
     ///
     /// TODO(emilio): See if it's worth to sort them, or doing something else in
     /// a similar fashion as what Boris is doing for the ID attribute.
     class_list: Option<SmallVec<[Atom; 5]>>,
 
     /// The list of presentational attributes of the element.
     pres_hints: Option<SmallVec<[ApplicableDeclarationBlock; 5]>>,
 
     /// The cached result of matching this entry against the revalidation
     /// selectors.
     revalidation_match_results: Option<BitVec>,
 }
 
 impl ValidationData {
-    /// Trivially construct an empty `ValidationData` with nothing on
-    /// it.
-    pub fn new() -> Self {
-        Self {
-            class_list: None,
-            pres_hints: None,
-            revalidation_match_results: None,
-        }
-    }
-
     /// Move the cached data to a new instance, and return it.
     pub fn take(&mut self) -> Self {
-        Self {
-            class_list: self.class_list.take(),
-            pres_hints: self.pres_hints.take(),
-            revalidation_match_results: self.revalidation_match_results.take(),
-        }
+        mem::replace(self, Self::default())
     }
 
     /// Get or compute the list of presentational attributes associated with
     /// this element.
     pub fn pres_hints<E>(&mut self, element: E) -> &[ApplicableDeclarationBlock]
         where E: TElement,
     {
         if self.pres_hints.is_none() {
@@ -85,18 +72,25 @@ impl ValidationData {
         &*self.pres_hints.as_ref().unwrap()
     }
 
     /// Get or compute the class-list associated with this element.
     pub fn class_list<E>(&mut self, element: E) -> &[Atom]
         where E: TElement,
     {
         if self.class_list.is_none() {
-            let mut class_list = SmallVec::new();
+            let mut class_list = SmallVec::<[Atom; 5]>::new();
             element.each_class(|c| class_list.push(c.clone()));
+            // Assuming there are a reasonable number of classes (we use the
+            // inline capacity as "reasonable number"), sort them to so that
+            // we don't mistakenly reject sharing candidates when one element
+            // has "foo bar" and the other has "bar foo".
+            if !class_list.spilled() {
+                class_list.sort_by(|a, b| a.get_hash().cmp(&b.get_hash()));
+            }
             self.class_list = Some(class_list);
         }
         &*self.class_list.as_ref().unwrap()
     }
 
     /// Computes the revalidation results if needed, and returns it.
     fn revalidation_match_results<E, F>(
         &mut self,
@@ -127,16 +121,25 @@ impl ValidationData {
 #[derive(Debug)]
 pub struct StyleSharingCandidate<E: TElement> {
     /// The element. We use SendElement here so that the cache may live in
     /// ScopedTLS.
     element: SendElement<E>,
     validation_data: ValidationData,
 }
 
+impl<E: TElement> Deref for StyleSharingCandidate<E> {
+    type Target = E;
+
+    fn deref(&self) -> &Self::Target {
+        &self.element
+    }
+}
+
+
 impl<E: TElement> StyleSharingCandidate<E> {
     /// Get the classlist of this candidate.
     fn class_list(&mut self) -> &[Atom] {
         self.validation_data.class_list(*self.element)
     }
 
     /// Get the pres hints of this candidate.
     fn pres_hints(&mut self) -> &[ApplicableDeclarationBlock] {
@@ -177,17 +180,17 @@ impl<E: TElement> Deref for StyleSharing
     }
 }
 
 impl<E: TElement> StyleSharingTarget<E> {
     /// Trivially construct a new StyleSharingTarget to test against the cache.
     pub fn new(element: E) -> Self {
         Self {
             element: element,
-            validation_data: ValidationData::new(),
+            validation_data: ValidationData::default(),
         }
     }
 
     fn class_list(&mut self) -> &[Atom] {
         self.validation_data.class_list(self.element)
     }
 
     /// Get the pres hints of this candidate.
@@ -230,36 +233,31 @@ impl<E: TElement> StyleSharingTarget<E> 
 
     /// Attempts to share a style with another node.
     pub fn share_style_if_possible(
         mut self,
         context: &mut StyleContext<E>,
         data: &mut ElementData)
         -> StyleSharingResult
     {
-        use std::mem;
-
         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 result = context.thread_local
             .style_sharing_candidate_cache
             .share_style_if_possible(shared_context,
                                      selector_flags_map,
                                      bloom_filter,
                                      &mut self,
                                      data);
 
-        mem::swap(&mut self.validation_data,
-                  &mut context
-                      .thread_local
-                      .current_element_info.as_mut().unwrap()
-                      .validation_data);
 
+        context.thread_local.current_element_info.as_mut().unwrap().validation_data =
+            self.validation_data.take();
         result
     }
 }
 
 /// A cache miss result.
 #[derive(Clone, Debug)]
 pub enum CacheMiss {
     /// The parents don't match.
@@ -415,22 +413,16 @@ impl<E: TElement> StyleSharingCandidateC
             return StyleSharingResult::CannotShare
         }
 
         if target.is_native_anonymous() {
             debug!("{:?} Cannot share style: NAC", target.element);
             return StyleSharingResult::CannotShare;
         }
 
-        if target.style_attribute().is_some() {
-            debug!("{:?} Cannot share style: element has style attribute",
-                   target.element);
-            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 =
@@ -548,17 +540,17 @@ impl<E: TElement> StyleSharingCandidateC
         if !checks::have_same_state_ignoring_visitedness(target.element, candidate) {
             miss!(State)
         }
 
         if target.get_id() != candidate.element.get_id() {
             miss!(IdAttr)
         }
 
-        if target.style_attribute().is_some() {
+        if !checks::have_same_style_attribute(target, candidate) {
             miss!(StyleAttr)
         }
 
         if !checks::have_same_class(target, candidate) {
             miss!(Class)
         }
 
         if !checks::have_same_presentational_hints(target, candidate) {
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -21,18 +21,18 @@ use properties::{AnimationRules, Propert
 #[cfg(feature = "servo")]
 use properties::INHERIT_ALL;
 use restyle_hints::{HintComputationContext, DependencySet, RestyleHint};
 use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource};
 use selector_map::{SelectorMap, SelectorMapEntry};
 use selector_parser::{SelectorImpl, PseudoElement};
 use selectors::attr::NamespaceConstraint;
 use selectors::bloom::BloomFilter;
-use selectors::matching::{AFFECTED_BY_STYLE_ATTRIBUTE, AFFECTED_BY_PRESENTATIONAL_HINTS};
 use selectors::matching::{ElementSelectorFlags, matches_selector, MatchingContext, MatchingMode};
+use selectors::matching::AFFECTED_BY_PRESENTATIONAL_HINTS;
 use selectors::parser::{Combinator, Component, Selector, SelectorInner, SelectorIter, SelectorMethods};
 use selectors::visitor::SelectorVisitor;
 use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards};
 use sink::Push;
 use smallvec::{SmallVec, VecLike};
 #[cfg(feature = "servo")]
 use std::marker::PhantomData;
 use style_traits::viewport::ViewportConstraints;
@@ -961,17 +961,19 @@ impl Stylist {
             let length_before_preshints = applicable_declarations.len();
             element.synthesize_presentational_hints_for_legacy_attributes(applicable_declarations);
             if applicable_declarations.len() != length_before_preshints {
                 if cfg!(debug_assertions) {
                     for declaration in &applicable_declarations[length_before_preshints..] {
                         assert_eq!(declaration.level, CascadeLevel::PresHints);
                     }
                 }
-                // Never share style for elements with preshints
+                // Note the existence of presentational attributes so that the
+                // style sharing cache can avoid re-querying them if they don't
+                // exist.
                 context.relations |= AFFECTED_BY_PRESENTATIONAL_HINTS;
             }
             debug!("preshints: {:?}", context.relations);
         }
 
         // NB: the following condition, although it may look somewhat
         // inaccurate, would be equivalent to something like:
         //
@@ -1000,17 +1002,16 @@ impl Stylist {
                                               applicable_declarations,
                                               context,
                                               flags_setter,
                                               CascadeLevel::AuthorNormal);
             debug!("author normal: {:?}", context.relations);
 
             // Step 4: Normal style attributes.
             if let Some(sa) = style_attribute {
-                context.relations |= AFFECTED_BY_STYLE_ATTRIBUTE;
                 Push::push(
                     applicable_declarations,
                     ApplicableDeclarationBlock::from_declarations(sa.clone(),
                                                                   CascadeLevel::StyleAttributeNormal));
             }
 
             debug!("style attr: {:?}", context.relations);