Bug 1596712 - Use only Origin during the cascade, rather than CascadeLevel. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 17 Nov 2019 23:28:39 +0000
changeset 502360 a78c989f5538874bf4c5a0c98b199441ed6921f6
parent 502359 44a3e5f050de4aff4e3080532319595185f1db3a
child 502361 78d380a2241afa5c60a90ac8ceca9b3108b99345
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1596712
milestone72.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
Bug 1596712 - Use only Origin during the cascade, rather than CascadeLevel. r=heycam The micro-benchmark `style-attr-1.html` regressed slightly with my patch, after the CascadeLevel size increase. This benchmark is meant to test for the "changing the style attribute doesn't cause selector-matching" optimization (which, mind you, keeps working). But in the process it creates 10k rules which form a perfect path in the rule tree and that we put into a SmallVec during the cascade, and the benchmark spends most of the time pushing to that SmallVec and iterating the declarations (as there's only one property to apply). So we could argue that the regression is minor and is not what the benchark is supposed to be testing, but given I did the digging... :) My patch made CascadeLevel bigger, which means that we create a somewhat bigger vector in this case. Thankfully it also removed the dependency in the CascadeLevel, so we can stop using that and use just Origin which is one byte to revert the perf regression. Differential Revision: https://phabricator.services.mozilla.com/D53181
servo/components/style/animation.rs
servo/components/style/properties/cascade.rs
servo/components/style/stylist.rs
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -11,18 +11,18 @@
 use crate::bezier::Bezier;
 use crate::context::SharedStyleContext;
 use crate::dom::{OpaqueNode, TElement};
 use crate::font_metrics::FontMetricsProvider;
 use crate::properties::animated_properties::AnimatedProperty;
 use crate::properties::longhands::animation_direction::computed_value::single_value::T as AnimationDirection;
 use crate::properties::longhands::animation_play_state::computed_value::single_value::T as AnimationPlayState;
 use crate::properties::{self, CascadeMode, ComputedValues, LonghandId};
-use crate::rule_tree::CascadeLevel;
 use crate::stylesheets::keyframes_rule::{KeyframesAnimation, KeyframesStep, KeyframesStepValue};
+use crate::stylesheets::Origin;
 use crate::timer::Timer;
 use crate::values::computed::box_::TransitionProperty;
 use crate::values::computed::Time;
 use crate::values::computed::TimingFunction;
 use crate::values::generics::box_::AnimationIterationCount;
 use crate::values::generics::easing::{StepPosition, TimingFunction as GenericTimingFunction};
 use crate::Atom;
 #[cfg(feature = "servo")]
@@ -486,17 +486,17 @@ where
             let iter = || {
                 // It's possible to have !important properties in keyframes
                 // so we have to filter them out.
                 // See the spec issue https://github.com/w3c/csswg-drafts/issues/1824
                 // Also we filter our non-animatable properties.
                 guard
                     .normal_declaration_iter()
                     .filter(|declaration| declaration.is_animatable())
-                    .map(|decl| (decl, CascadeLevel::Animations))
+                    .map(|decl| (decl, Origin::Author))
             };
 
             // This currently ignores visited styles, which seems acceptable,
             // as existing browsers don't appear to animate visited styles.
             let computed = properties::apply_declarations::<E, _, _>(
                 context.stylist.device(),
                 /* pseudo = */ None,
                 previous_style.rules(),
--- a/servo/components/style/properties/cascade.rs
+++ b/servo/components/style/properties/cascade.rs
@@ -10,17 +10,17 @@ use crate::dom::TElement;
 use crate::font_metrics::FontMetricsProvider;
 use crate::logical_geometry::WritingMode;
 use crate::media_queries::Device;
 use crate::properties::{ComputedValues, StyleBuilder};
 use crate::properties::{LonghandId, LonghandIdSet, CSSWideKeyword};
 use crate::properties::{PropertyDeclaration, PropertyDeclarationId, DeclarationImportanceIterator};
 use crate::properties::CASCADE_PROPERTY;
 use crate::rule_cache::{RuleCache, RuleCacheConditions};
-use crate::rule_tree::{CascadeLevel, StrongRuleNode};
+use crate::rule_tree::StrongRuleNode;
 use crate::selector_parser::PseudoElement;
 use crate::stylesheets::{Origin, PerOrigin};
 use servo_arc::Arc;
 use crate::shared_lock::StylesheetGuards;
 use smallbitvec::SmallBitVec;
 use smallvec::SmallVec;
 use std::borrow::Cow;
 use std::cell::RefCell;
@@ -129,43 +129,47 @@ where
     debug_assert_eq!(
         parent_style.is_some(),
         parent_style_ignoring_first_line.is_some()
     );
     let empty = SmallBitVec::new();
     let restriction = pseudo.and_then(|p| p.property_restriction());
     let iter_declarations = || {
         rule_node.self_and_ancestors().flat_map(|node| {
-            let cascade_level = node.cascade_level();
+            let origin = node.cascade_level().origin();
             let node_importance = node.importance();
+            let guard = match origin {
+                Origin::Author => guards.author,
+                Origin::User | Origin::UserAgent => guards.ua_or_user,
+            };
             let declarations = match node.style_source() {
                 Some(source) => source
-                    .read(cascade_level.guard(guards))
+                    .read(guard)
                     .declaration_importance_iter(),
                 None => DeclarationImportanceIterator::new(&[], &empty),
             };
 
             declarations
                 // Yield declarations later in source order (with more precedence) first.
                 .rev()
                 .filter_map(move |(declaration, declaration_importance)| {
                     if let Some(restriction) = restriction {
                         // declaration.id() is either a longhand or a custom
                         // property.  Custom properties are always allowed, but
                         // longhands are only allowed if they have our
                         // restriction flag set.
                         if let PropertyDeclarationId::Longhand(id) = declaration.id() {
-                            if !id.flags().contains(restriction) && cascade_level.origin() != Origin::UserAgent {
+                            if !id.flags().contains(restriction) && origin != Origin::UserAgent {
                                 return None;
                             }
                         }
                     }
 
                     if declaration_importance == node_importance {
-                        Some((declaration, cascade_level))
+                        Some((declaration, origin))
                     } else {
                         None
                     }
                 })
         })
     };
 
     apply_declarations(
@@ -218,17 +222,17 @@ pub fn apply_declarations<'a, E, F, I>(
     quirks_mode: QuirksMode,
     rule_cache: Option<&RuleCache>,
     rule_cache_conditions: &mut RuleCacheConditions,
     element: Option<E>,
 ) -> Arc<ComputedValues>
 where
     E: TElement,
     F: Fn() -> I,
-    I: Iterator<Item = (&'a PropertyDeclaration, CascadeLevel)>,
+    I: Iterator<Item = (&'a PropertyDeclaration, Origin)>,
 {
     debug_assert!(layout_parent_style.is_none() || parent_style.is_some());
     debug_assert_eq!(
         parent_style.is_some(),
         parent_style_ignoring_first_line.is_some()
     );
     #[cfg(feature = "gecko")]
     debug_assert!(
@@ -237,27 +241,27 @@ where
                 parent_style.unwrap(),
                 parent_style_ignoring_first_line.unwrap()
             ) ||
             parent_style.unwrap().is_first_line_style()
     );
 
     let inherited_style = parent_style.unwrap_or(device.default_computed_values());
 
-    let mut declarations = SmallVec::<[(&_, CascadeLevel); 32]>::new();
+    let mut declarations = SmallVec::<[(&_, Origin); 32]>::new();
     let custom_properties = {
         let mut builder = CustomPropertiesBuilder::new(
             inherited_style.custom_properties(),
             device.environment(),
         );
 
-        for (declaration, cascade_level) in iter_declarations() {
-            declarations.push((declaration, cascade_level));
+        for (declaration, origin) in iter_declarations() {
+            declarations.push((declaration, origin));
             if let PropertyDeclaration::Custom(ref declaration) = *declaration {
-                builder.cascade(declaration, cascade_level.origin());
+                builder.cascade(declaration, origin);
             }
         }
 
         builder.build()
     };
 
     let mut context = computed::Context {
         is_root_element: pseudo.is_none() && element.map_or(false, |e| e.is_root()),
@@ -327,26 +331,25 @@ where
     }
 
     context.builder.build()
 }
 
 fn should_ignore_declaration_when_ignoring_document_colors(
     device: &Device,
     longhand_id: LonghandId,
-    cascade_level: CascadeLevel,
+    origin: Origin,
     pseudo: Option<&PseudoElement>,
     declaration: &mut Cow<PropertyDeclaration>,
 ) -> bool {
     if !longhand_id.ignored_when_document_colors_disabled() {
         return false;
     }
 
-    let is_ua_or_user_rule =
-        matches!(cascade_level.origin(), Origin::User | Origin::UserAgent);
+    let is_ua_or_user_rule = matches!(origin, Origin::User | Origin::UserAgent);
     if is_ua_or_user_rule {
         return false;
     }
 
     // Don't override background-color on ::-moz-color-swatch. It is set as an
     // author style (via the style attribute), but it's pretty important for it
     // to show up for obvious reasons :)
     if pseudo.map_or(false, |p| p.is_color_swatch()) && longhand_id == LonghandId::BackgroundColor {
@@ -441,32 +444,31 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
     }
 
     fn apply_properties<'decls, Phase, I>(
         &mut self,
         apply_reset: ApplyResetProperties,
         declarations: I,
     ) where
         Phase: CascadePhase,
-        I: Iterator<Item = (&'decls PropertyDeclaration, CascadeLevel)>,
+        I: Iterator<Item = (&'decls PropertyDeclaration, Origin)>,
     {
         let apply_reset = apply_reset == ApplyResetProperties::Yes;
 
         debug_assert!(
             !Phase::is_early() || apply_reset,
             "Should always apply reset properties in the early phase, since we \
              need to know font-size / writing-mode to decide whether to use the \
              cached reset properties"
         );
 
         let ignore_colors = !self.context.builder.device.use_document_colors();
 
-        for (declaration, cascade_level) in declarations {
+        for (declaration, origin) in declarations {
             let declaration_id = declaration.id();
-            let origin = cascade_level.origin();
             let longhand_id = match declaration_id {
                 PropertyDeclarationId::Longhand(id) => id,
                 PropertyDeclarationId::Custom(..) => continue,
             };
 
             let inherited = longhand_id.inherited();
             if !apply_reset && !inherited {
                 continue;
@@ -504,17 +506,17 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
 
             // When document colors are disabled, skip properties that are
             // marked as ignored in that mode, unless they come from a UA or
             // user style sheet.
             if ignore_colors {
                 let should_ignore = should_ignore_declaration_when_ignoring_document_colors(
                     self.context.builder.device,
                     longhand_id,
-                    cascade_level,
+                    origin,
                     self.context.builder.pseudo,
                     &mut declaration,
                 );
                 if should_ignore {
                     continue;
                 }
             }
 
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -1317,26 +1317,27 @@ impl Stylist {
         E: TElement,
     {
         use crate::font_metrics::get_metrics_provider_for_product;
 
         let block = declarations.read_with(guards.author);
         let iter_declarations = || {
             block
                 .declaration_importance_iter()
-                .map(|(declaration, importance)| {
-                    debug_assert!(!importance.important());
-                    (declaration, CascadeLevel::same_tree_author_normal())
-                })
+                .map(|(declaration, _)| (declaration, Origin::Author))
         };
 
         let metrics = get_metrics_provider_for_product();
 
         // We don't bother inserting these declarations in the rule tree, since
         // it'd be quite useless and slow.
+        //
+        // TODO(emilio): Now that we fixed bug 1493420, we should consider
+        // reversing this as it shouldn't be slow anymore, and should avoid
+        // generating two instantiations of apply_declarations.
         properties::apply_declarations::<E, _, _>(
             &self.device,
             /* pseudo = */ None,
             self.rule_tree.root(),
             guards,
             iter_declarations,
             Some(parent_style),
             Some(parent_style),