Auto merge of #17110 - bholley:better_style_sharing, r=emilio

Improve Style Sharing

Reviewed in:
https://bugzilla.mozilla.org/show_bug.cgi?id=1368665
https://bugzilla.mozilla.org/show_bug.cgi?id=1368399

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17110)
<!-- Reviewable:end -->

components/selectors/matching.rs

@@ -23,14 +23,11 @@ bitflags! {
     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,
     }
 }
 

components/style/context.rs

@@ -463,7 +463,7 @@ impl<E: TElement> ThreadLocalStyleContext<E> {
         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(),
         });
     }

components/style/sharing/checks.rs

@@ -21,8 +21,7 @@ use stylearc::Arc;
 pub fn relations_are_shareable(relations: &StyleRelations) -> bool {
     use selectors::matching::*;
     !relations.intersects(AFFECTED_BY_ID_SELECTOR |
-                          AFFECTED_BY_PSEUDO_ELEMENTS |
+                          AFFECTED_BY_PSEUDO_ELEMENTS)
-                          AFFECTED_BY_STYLE_ATTRIBUTE)
 }
 
 /// Whether, given two elements, they have pointer-equal computed values.
@@ -44,11 +43,21 @@ pub fn same_computed_values<E>(first: Option<E>, second: Option<E>) -> bool
     eq
 }
 
-/// Whether a given element has presentational hints.
+/// Whether two elements have the same same style attribute (by pointer identity).
-///
+pub fn have_same_style_attribute<E>(
-/// We consider not worth to share style with an element that has presentational
+    target: &mut StyleSharingTarget<E>,
-/// hints, both because implementing the code that compares that the hints are
+    candidate: &mut StyleSharingCandidate<E>
-/// equal is somewhat annoying, and also because it'd be expensive enough.
+) -> 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>

components/style/sharing/mod.rs

@@ -16,6 +16,7 @@ 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};
 
@@ -36,7 +37,7 @@ pub enum StyleSharingBehavior {
 
 /// 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.
     ///
@@ -53,23 +54,9 @@ pub struct ValidationData {
 }
 
 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 {
+        mem::replace(self, Self::default())
-            class_list: self.class_list.take(),
-            pres_hints: self.pres_hints.take(),
-            revalidation_match_results: self.revalidation_match_results.take(),
-        }
     }
 
     /// Get or compute the list of presentational attributes associated with
@@ -90,8 +77,15 @@ impl ValidationData {
         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()
@@ -132,6 +126,15 @@ pub struct StyleSharingCandidate<E: TElement> {
     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] {
@@ -182,7 +185,7 @@ impl<E: TElement> StyleSharingTarget<E> {
     pub fn new(element: E) -> Self {
         Self {
             element: element,
-            validation_data: ValidationData::new(),
+            validation_data: ValidationData::default(),
         }
     }
 
@@ -235,8 +238,6 @@ impl<E: TElement> StyleSharingTarget<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();
@@ -249,12 +250,9 @@ impl<E: TElement> StyleSharingTarget<E> {
                                      &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
     }
 }
@@ -420,12 +418,6 @@ impl<E: TElement> StyleSharingCandidateCache<E> {
             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
@@ -553,7 +545,7 @@ impl<E: TElement> StyleSharingCandidateCache<E> {
             miss!(IdAttr)
         }
 
-        if target.style_attribute().is_some() {
+        if !checks::have_same_style_attribute(target, candidate) {
             miss!(StyleAttr)
         }
 

components/style/stylist.rs

@@ -26,8 +26,8 @@ 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};
@@ -966,7 +966,9 @@ impl Stylist {
                         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);
@@ -1005,7 +1007,6 @@ impl Stylist {
 
             // 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(),