servo: Merge #17713 - style: Kill some style sharing code (from emilio:less-code-is-lovely); r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 14 Jul 2017 00:25:37 -0700
changeset 608903 dce0c8a1352f3070c82794a1aedcc57fb6dc9109
parent 608902 42091be84f6bcd377f99b2cb3ba2711bf34f54e9
child 608904 e9a812d1992459aba4a6069298e43c8ad5b078e6
child 608990 422dc84cfcdca505c3804587d7668e0822ba1aaa
push id68438
push users.kaspari@gmail.com
push dateFri, 14 Jul 2017 09:06:05 +0000
reviewersheycam
milestone56.0a1
servo: Merge #17713 - style: Kill some style sharing code (from emilio:less-code-is-lovely); r=heycam It's trivial to do so after #17688. Source-Repo: https://github.com/servo/servo Source-Revision: 1c85c55d02458afffea0ec898e4560d62d447e4e
servo/components/script_layout_interface/wrapper_traits.rs
servo/components/style/context.rs
servo/components/style/data.rs
servo/components/style/sharing/mod.rs
servo/components/style/traversal.rs
--- a/servo/components/script_layout_interface/wrapper_traits.rs
+++ b/servo/components/script_layout_interface/wrapper_traits.rs
@@ -341,26 +341,26 @@ pub trait ThreadSafeLayoutElement: Clone
 
     fn style_data(&self) -> AtomicRef<ElementData>;
 
     #[inline]
     fn get_pseudo_element_type(&self) -> PseudoElementType<Option<display::T>>;
 
     #[inline]
     fn get_before_pseudo(&self) -> Option<Self> {
-        if self.style_data().styles.pseudos.has(&PseudoElement::Before) {
+        if self.style_data().styles.pseudos.get(&PseudoElement::Before).is_some() {
             Some(self.with_pseudo(PseudoElementType::Before(None)))
         } else {
             None
         }
     }
 
     #[inline]
     fn get_after_pseudo(&self) -> Option<Self> {
-        if self.style_data().styles.pseudos.has(&PseudoElement::After) {
+        if self.style_data().styles.pseudos.get(&PseudoElement::After).is_some() {
             Some(self.with_pseudo(PseudoElementType::After(None)))
         } else {
             None
         }
     }
 
     #[inline]
     fn get_details_summary_pseudo(&self) -> Option<Self> {
--- a/servo/components/style/context.rs
+++ b/servo/components/style/context.rs
@@ -16,17 +16,17 @@ use fnv::FnvHashMap;
 use font_metrics::FontMetricsProvider;
 #[cfg(feature = "gecko")] use gecko_bindings::structs;
 #[cfg(feature = "servo")] use parking_lot::RwLock;
 use properties::ComputedValues;
 use rule_tree::StrongRuleNode;
 use selector_parser::{EAGER_PSEUDO_COUNT, SnapshotMap};
 use selectors::matching::ElementSelectorFlags;
 use shared_lock::StylesheetGuards;
-use sharing::{ValidationData, StyleSharingCandidateCache};
+use sharing::StyleSharingCandidateCache;
 use std::fmt;
 use std::ops::Add;
 #[cfg(feature = "servo")] use std::sync::Mutex;
 #[cfg(feature = "servo")] use std::sync::mpsc::Sender;
 use stylearc::Arc;
 use stylist::Stylist;
 use thread_state;
 use time;
@@ -264,18 +264,16 @@ impl ElementCascadeInputs {
 /// processing.
 pub struct CurrentElementInfo {
     /// The element being processed. Currently we use an OpaqueNode since we
     /// only use this for identity checks, but we could use SendElement if there
     /// were a good reason to.
     element: OpaqueNode,
     /// Whether the element is being styled for the first time.
     is_initial_style: bool,
-    /// Lazy cache of the different data used for style sharing.
-    pub validation_data: ValidationData,
     /// A Vec of possibly expired animations. Used only by Servo.
     #[allow(dead_code)]
     pub possibly_expired_animations: Vec<PropertyAnimation>,
 }
 
 /// Statistics gathered during the traversal. We gather statistics on each
 /// thread and then combine them after the threads join via the Add
 /// implementation below.
@@ -570,17 +568,16 @@ 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::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/data.rs
+++ b/servo/components/style/data.rs
@@ -1,15 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! Per-node data used in style calculation.
 
-use arrayvec::ArrayVec;
 use context::SharedStyleContext;
 use dom::TElement;
 use invalidation::element::restyle_hints::RestyleHint;
 use properties::ComputedValues;
 use properties::longhands::display::computed_value as display;
 use rule_tree::StrongRuleNode;
 use selector_parser::{EAGER_PSEUDO_COUNT, PseudoElement, RestyleDamage};
 use shared_lock::StylesheetGuards;
@@ -152,89 +151,24 @@ impl EagerPseudoStyles {
     }
 
     /// Returns a reference to the style for a given eager pseudo, if it exists.
     pub fn get(&self, pseudo: &PseudoElement) -> Option<&Arc<ComputedValues>> {
         debug_assert!(pseudo.is_eager());
         self.0.as_ref().and_then(|p| p[pseudo.eager_index()].as_ref())
     }
 
-    /// Returns a mutable reference to the style for a given eager pseudo, if it exists.
-    pub fn get_mut(&mut self, pseudo: &PseudoElement) -> Option<&mut Arc<ComputedValues>> {
-        debug_assert!(pseudo.is_eager());
-        match self.0 {
-            None => return None,
-            Some(ref mut arc) => Arc::make_mut(arc)[pseudo.eager_index()].as_mut(),
-        }
-    }
-
-    /// Returns true if the EagerPseudoStyles has the style for |pseudo|.
-    pub fn has(&self, pseudo: &PseudoElement) -> bool {
-        self.get(pseudo).is_some()
-    }
-
     /// Sets the style for the eager pseudo.
     pub fn set(&mut self, pseudo: &PseudoElement, value: Arc<ComputedValues>) {
         if self.0.is_none() {
             self.0 = Some(Arc::new(Default::default()));
         }
         let arr = Arc::make_mut(self.0.as_mut().unwrap());
         arr[pseudo.eager_index()] = Some(value);
     }
-
-    /// Inserts a pseudo-element. The pseudo-element must not already exist.
-    pub fn insert(&mut self, pseudo: &PseudoElement, value: Arc<ComputedValues>) {
-        debug_assert!(!self.has(pseudo));
-        self.set(pseudo, value);
-    }
-
-    /// Removes a pseudo-element style if it exists, and returns it.
-    pub fn take(&mut self, pseudo: &PseudoElement) -> Option<Arc<ComputedValues>> {
-        let result = match self.0 {
-            None => return None,
-            Some(ref mut arc) => Arc::make_mut(arc)[pseudo.eager_index()].take(),
-        };
-        let empty = self.0.as_ref().unwrap().iter().all(|x| x.is_none());
-        if empty {
-            self.0 = None;
-        }
-        result
-    }
-
-    /// Returns a list of the pseudo-elements.
-    pub fn keys(&self) -> ArrayVec<[PseudoElement; EAGER_PSEUDO_COUNT]> {
-        let mut v = ArrayVec::new();
-        if let Some(ref arr) = self.0 {
-            for i in 0..EAGER_PSEUDO_COUNT {
-                if arr[i].is_some() {
-                    v.push(PseudoElement::from_eager_index(i));
-                }
-            }
-        }
-        v
-    }
-
-    /// Returns whether this map has the same set of pseudos as the given one.
-    pub fn has_same_pseudos_as(&self, other: &Self) -> 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, Default)]
 pub struct ElementStyles {
     /// The element's style.
     pub primary: Option<Arc<ComputedValues>>,
@@ -243,21 +177,16 @@ pub struct ElementStyles {
 }
 
 impl ElementStyles {
     /// Returns the primary style.
     pub fn get_primary(&self) -> Option<&Arc<ComputedValues>> {
         self.primary.as_ref()
     }
 
-    /// Returns the mutable primary style.
-    pub fn get_primary_mut(&mut self) -> Option<&mut Arc<ComputedValues>> {
-        self.primary.as_mut()
-    }
-
     /// Returns the primary style.  Panic if no style available.
     pub fn primary(&self) -> &Arc<ComputedValues> {
         self.primary.as_ref().unwrap()
     }
 
     /// Whether this element `display` value is `none`.
     pub fn is_display_none(&self) -> bool {
         self.primary().get_box().clone_display() == display::T::none
--- a/servo/components/style/sharing/mod.rs
+++ b/servo/components/style/sharing/mod.rs
@@ -65,21 +65,20 @@
 //! elements makes sense.
 
 use Atom;
 use applicable_declarations::ApplicableDeclarationBlock;
 use bit_vec::BitVec;
 use bloom::StyleBloom;
 use cache::{LRUCache, LRUCacheMutIterator};
 use context::{SelectorFlagsMap, SharedStyleContext, StyleContext};
-use data::{ElementData, ElementStyles};
+use data::ElementStyles;
 use dom::{TElement, SendElement};
-use matching::{ChildCascadeRequirement, MatchMethods};
+use matching::MatchMethods;
 use properties::ComputedValues;
-use selector_parser::RestyleDamage;
 use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode};
 use smallvec::SmallVec;
 use std::mem;
 use std::ops::Deref;
 use stylist::Stylist;
 
 mod checks;
 
@@ -324,98 +323,43 @@ impl<E: TElement> StyleSharingTarget<E> 
             stylist,
             bloom,
             /* bloom_known_valid = */ true,
             &mut set_selector_flags)
     }
 
     /// Attempts to share a style with another node.
     pub fn share_style_if_possible(
-        mut self,
+        &mut self,
         context: &mut StyleContext<E>,
-        data: &mut ElementData)
-        -> StyleSharingResult
-    {
+    ) -> StyleSharingResult {
         let cache = &mut context.thread_local.style_sharing_candidate_cache;
         let shared_context = &context.shared;
         let selector_flags_map = &mut context.thread_local.selector_flags;
         let bloom_filter = &context.thread_local.bloom_filter;
 
         if cache.dom_depth != bloom_filter.matching_depth() {
             debug!("Can't share style, because DOM depth changed from {:?} to {:?}, element: {:?}",
                    cache.dom_depth, bloom_filter.matching_depth(), self.element);
             return StyleSharingResult::CannotShare;
         }
         debug_assert_eq!(bloom_filter.current_parent(),
                          self.element.traversal_parent());
 
-        let result = cache
-            .share_style_if_possible(shared_context,
-                                     selector_flags_map,
-                                     bloom_filter,
-                                     &mut self,
-                                     data);
-
-
-        context.thread_local.current_element_info.as_mut().unwrap().validation_data =
-            self.validation_data.take();
-        result
+        cache.share_style_if_possible(
+            shared_context,
+            selector_flags_map,
+            bloom_filter,
+            self
+        )
     }
 
-    fn accumulate_damage_when_sharing(&self,
-                                      shared_context: &SharedStyleContext,
-                                      shared_styles: &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, mut restyle_data) = data.styles_and_restyle_mut();
-            let old_pseudos = &styles.pseudos;
-            let new_pseudos = &shared_styles.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).map(|v| &**v);
-                    let new_values =
-                        new_pseudos.get(&pseudo).unwrap();
-                    self.element.accumulate_damage(
-                        &shared_context,
-                        restyle_data,
-                        old_values,
-                        new_values,
-                        Some(&pseudo)
-                    );
-                }
-            }
-        }
-
-        let old_values = data.styles.primary.take();
-        self.element.accumulate_damage(
-            &shared_context,
-            &mut data.restyle,
-            old_values.as_ref().map(|v| &**v),
-            shared_styles.primary(),
-            None
-        )
+    /// Gets the validation data used to match against this target, if any.
+    pub fn take_validation_data(&mut self) -> ValidationData {
+        self.validation_data.take()
     }
 }
 
 /// A cache miss result.
 #[derive(Clone, Debug)]
 pub enum CacheMiss {
     /// The parents don't match.
     Parent,
@@ -446,20 +390,18 @@ pub enum CacheMiss {
     Revalidation,
 }
 
 /// The results of attempting to share a style.
 pub enum StyleSharingResult {
     /// We didn't find anybody to share the style with.
     CannotShare,
     /// The node's style can be shared. The integer specifies the index in the
-    /// LRU cache that was hit and the damage that was done. The
-    /// `ChildCascadeRequirement` indicates whether style changes due to using
-    /// the shared style mean we need to recascade to children.
-    StyleWasShared(usize, ChildCascadeRequirement),
+    /// LRU cache that was hit and the damage that was done.
+    StyleWasShared(usize, ElementStyles),
 }
 
 /// An LRU cache of the last few nodes seen, so that we can aggressively try to
 /// reuse their styles.
 ///
 /// Note that this cache is flushed every time we steal work from the queue, so
 /// storing nodes here temporarily is safe.
 pub struct StyleSharingCandidateCache<E: TElement> {
@@ -548,17 +490,16 @@ impl<E: TElement> StyleSharingCandidateC
 
     /// Attempts to share a style with another node.
     fn share_style_if_possible(
         &mut self,
         shared_context: &SharedStyleContext,
         selector_flags_map: &mut SelectorFlagsMap<E>,
         bloom_filter: &StyleBloom<E>,
         target: &mut StyleSharingTarget<E>,
-        data: &mut ElementData
     ) -> StyleSharingResult {
         if shared_context.options.disable_style_sharing_cache {
             debug!("{:?} Cannot share style: style sharing cache disabled",
                    target.element);
             return StyleSharingResult::CannotShare
         }
 
         if target.traversal_parent().is_none() {
@@ -579,43 +520,37 @@ impl<E: TElement> StyleSharingCandidateC
                     candidate,
                     &shared_context,
                     bloom_filter,
                     selector_flags_map
                 );
 
             match sharing_result {
                 Ok(shared_styles) => {
-                    // Yay, cache hit. Share the style.
-                    let child_cascade_requirement =
-                        target.accumulate_damage_when_sharing(shared_context,
-                                                              &shared_styles,
-                                                              data);
-                    data.styles = shared_styles;
-
-                    return StyleSharingResult::StyleWasShared(i, child_cascade_requirement)
+                    return StyleSharingResult::StyleWasShared(i, shared_styles)
                 }
                 Err(miss) => {
                     debug!("Cache miss: {:?}", miss);
                 }
             }
         }
 
         debug!("{:?} Cannot share style: {} cache entries", target.element,
                self.cache.num_entries());
 
         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<ElementStyles, CacheMiss> {
+    fn test_candidate(
+        target: &mut StyleSharingTarget<E>,
+        candidate: &mut StyleSharingCandidate<E>,
+        shared: &SharedStyleContext,
+        bloom: &StyleBloom<E>,
+        selector_flags_map: &mut SelectorFlagsMap<E>
+    ) -> 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
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -766,60 +766,48 @@ where
                            animation-only traversal");
             // Ensure the bloom filter is up to date.
             context.thread_local.bloom_filter
                    .insert_parents_recovering(element,
                                               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;
-
+            // This is only relevant for animations as of right now.
             important_rules_changed = true;
 
-            // Perform the matching and cascading.
-            let new_styles =
-                StyleResolverForElement::new(element, context, RuleInclusion::All)
-                    .resolve_style_with_default_parents();
+            let mut target = StyleSharingTarget::new(element);
 
-            // If we previously tried to match this element against the cache,
-            // the revalidation match results will already be cached. Otherwise
-            // we'll have None, and compute them later on-demand.
-            //
-            // If we do have the results, grab them here to satisfy the borrow
-            // checker.
-            let validation_data =
-                context.thread_local
-                    .current_element_info
-                    .as_mut().unwrap()
-                    .validation_data
-                    .take();
+            // Now that our bloom filter is set up, try the style sharing
+            // cache.
+            match target.share_style_if_possible(context) {
+                StyleWasShared(index, styles) => {
+                    context.thread_local.statistics.styles_shared += 1;
+                    context.thread_local.style_sharing_candidate_cache.touch(index);
+                    styles
+                }
+                CannotShare => {
+                    context.thread_local.statistics.elements_matched += 1;
+                    // Perform the matching and cascading.
+                    let new_styles =
+                        StyleResolverForElement::new(element, context, RuleInclusion::All)
+                            .resolve_style_with_default_parents();
 
-            let dom_depth = context.thread_local.bloom_filter.matching_depth();
-            context.thread_local
-                   .style_sharing_candidate_cache
-                   .insert_if_possible(
-                       &element,
-                       new_styles.primary(),
-                       validation_data,
-                       dom_depth
-                    );
+                    context.thread_local
+                        .style_sharing_candidate_cache
+                        .insert_if_possible(
+                            &element,
+                            new_styles.primary(),
+                            target.take_validation_data(),
+                            context.thread_local.bloom_filter.matching_depth(),
+                        );
 
-            new_styles
+                    new_styles
+                }
+            }
         }
         CascadeWithReplacements(flags) => {
             // Skipping full matching, load cascade inputs from previous values.
             let mut cascade_inputs =
                 ElementCascadeInputs::new_from_element_data(data);
             important_rules_changed =
                 element.replace_rules(flags, context, &mut cascade_inputs);
             StyleResolverForElement::new(element, context, RuleInclusion::All)