style: Tweak cascade priority to split writing-mode and font properties

This makes the worst case for cascade performance slightly more
expensive (4 rather than three declaration walks), but my hope is that
it will make the average case faster, since the best case is now just
two walks instead of three, and writing mode properties are somewhat
rare.

This needs a test, but needs to wait until the writing-mode dependent
viewport units land (will wait to land with a test).

Differential Revision: https://phabricator.services.mozilla.com/D143261

components/style/properties/cascade.rs

@@ -28,34 +28,8 @@ use smallvec::SmallVec;
 use std::borrow::Cow;
 use std::cell::RefCell;
 
-/// We split the cascade in two phases: 'early' properties, and 'late'
-/// properties.
-///
-/// Early properties are the ones that don't have dependencies _and_ other
-/// properties depend on, for example, writing-mode related properties, color
-/// (for currentColor), or font-size (for em, etc).
-///
-/// Late properties are all the others.
-trait CascadePhase {
-    fn is_early() -> bool;
-}
-
-struct EarlyProperties;
-impl CascadePhase for EarlyProperties {
-    fn is_early() -> bool {
-        true
-    }
-}
-
-struct LateProperties;
-impl CascadePhase for LateProperties {
-    fn is_early() -> bool {
-        false
-    }
-}
-
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
-enum ApplyResetProperties {
+enum CanHaveLogicalProperties {
     No,
     Yes,
 }
@@ -283,6 +257,7 @@ where
     let inherited_style = parent_style.unwrap_or(device.default_computed_values());
 
     let mut declarations = SmallVec::<[(&_, CascadePriority); 32]>::new();
+    let mut referenced_properties = LonghandIdSet::default();
     let custom_properties = {
         let mut builder = CustomPropertiesBuilder::new(inherited_style.custom_properties(), device);
 
@@ -290,6 +265,8 @@ where
             declarations.push((declaration, priority));
             if let PropertyDeclaration::Custom(ref declaration) = *declaration {
                 builder.cascade(declaration, priority);
+            } else {
+                referenced_properties.insert(declaration.id().as_longhand().unwrap());
             }
         }
 
@@ -320,14 +297,25 @@ where
     };
 
     let using_cached_reset_properties = {
-        let mut cascade = Cascade::new(&mut context, cascade_mode);
+        let mut cascade = Cascade::new(&mut context, cascade_mode, &referenced_properties);
         let mut shorthand_cache = ShorthandsWithPropertyReferencesCache::default();
-
+        if cascade.apply_properties(
-        cascade.apply_properties::<EarlyProperties, _>(
+            CanHaveLogicalProperties::No,
-            ApplyResetProperties::Yes,
+            LonghandIdSet::writing_mode_group(),
             declarations.iter().cloned(),
             &mut shorthand_cache,
-        );
+        ) {
+            cascade.compute_writing_mode();
+        }
+
+        if cascade.apply_properties(
+            CanHaveLogicalProperties::No,
+            LonghandIdSet::fonts_and_color_group(),
+            declarations.iter().cloned(),
+            &mut shorthand_cache,
+        ) {
+            cascade.fixup_font_stuff();
+        }
 
         cascade.compute_visited_style_if_needed(
             element,
@@ -340,18 +328,21 @@ where
         let using_cached_reset_properties =
             cascade.try_to_use_cached_reset_properties(rule_cache, guards);
 
-        let apply_reset = if using_cached_reset_properties {
+        let properties_to_apply = if using_cached_reset_properties {
-            ApplyResetProperties::No
+            LonghandIdSet::late_group_only_inherited()
         } else {
-            ApplyResetProperties::Yes
+            LonghandIdSet::late_group()
         };
 
-        cascade.apply_properties::<LateProperties, _>(
+        cascade.apply_properties(
-            apply_reset,
+            CanHaveLogicalProperties::Yes,
+            properties_to_apply,
             declarations.iter().cloned(),
             &mut shorthand_cache,
         );
 
+        cascade.finished_applying_properties();
+
         using_cached_reset_properties
     };
 
@@ -500,6 +491,8 @@ fn tweak_when_ignoring_colors(
 struct Cascade<'a, 'b: 'a> {
     context: &'a mut computed::Context<'b>,
     cascade_mode: CascadeMode<'a>,
+    /// All the properties that have a declaration in the cascade.
+    referenced: &'a LonghandIdSet,
     seen: LonghandIdSet,
     author_specified: LonghandIdSet,
     reverted_set: LonghandIdSet,
@@ -507,10 +500,15 @@ struct Cascade<'a, 'b: 'a> {
 }
 
 impl<'a, 'b: 'a> Cascade<'a, 'b> {
-    fn new(context: &'a mut computed::Context<'b>, cascade_mode: CascadeMode<'a>) -> Self {
+    fn new(
+        context: &'a mut computed::Context<'b>,
+        cascade_mode: CascadeMode<'a>,
+        referenced: &'a LonghandIdSet,
+    ) -> Self {
         Self {
             context,
             cascade_mode,
+            referenced,
             seen: LonghandIdSet::default(),
             author_specified: LonghandIdSet::default(),
             reverted_set: Default::default(),
@@ -578,23 +576,21 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
         (CASCADE_PROPERTY[discriminant])(declaration, &mut self.context);
     }
 
-    fn apply_properties<'decls, Phase, I>(
+    fn apply_properties<'decls, I>(
         &mut self,
-        apply_reset: ApplyResetProperties,
+        can_have_logical_properties: CanHaveLogicalProperties,
+        properties_to_apply: &'a LonghandIdSet,
         declarations: I,
         mut shorthand_cache: &mut ShorthandsWithPropertyReferencesCache,
-    ) where
+    ) -> bool
-        Phase: CascadePhase,
+    where
         I: Iterator<Item = (&'decls PropertyDeclaration, CascadePriority)>,
     {
-        let apply_reset = apply_reset == ApplyResetProperties::Yes;
+        if !self.referenced.contains_any(properties_to_apply) {
+            return false;
+        }
 
-        debug_assert!(
+        let can_have_logical_properties = can_have_logical_properties == CanHaveLogicalProperties::Yes;
-            !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();
         let mut declarations_to_apply_unless_overriden = DeclarationsToApplyUnlessOverriden::new();
@@ -608,20 +604,15 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
                 PropertyDeclarationId::Custom(..) => continue,
             };
 
-            let inherited = longhand_id.inherited();
+            if !properties_to_apply.contains(longhand_id) {
-            if !apply_reset && !inherited {
                 continue;
             }
 
-            if Phase::is_early() != longhand_id.is_early_property() {
+            debug_assert!(can_have_logical_properties || !longhand_id.is_logical());
-                continue;
+            let physical_longhand_id = if can_have_logical_properties {
-            }
-
-            debug_assert!(!Phase::is_early() || !longhand_id.is_logical());
-            let physical_longhand_id = if Phase::is_early() {
-                longhand_id
-            } else {
                 longhand_id.to_physical(self.context.builder.writing_mode)
+            } else {
+                longhand_id
             };
 
             if self.seen.contains(physical_longhand_id) {
@@ -678,8 +669,8 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
                         continue;
                     },
                     CSSWideKeyword::Unset => true,
-                    CSSWideKeyword::Inherit => inherited,
+                    CSSWideKeyword::Inherit => longhand_id.inherited(),
-                    CSSWideKeyword::Initial => !inherited,
+                    CSSWideKeyword::Initial => !longhand_id.inherited(),
                 },
                 None => false,
             };
@@ -713,12 +704,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
             }
         }
 
-        if Phase::is_early() {
+        true
-            self.fixup_font_stuff();
-            self.compute_writing_mode();
-        } else {
-            self.finished_applying_properties();
-        }
     }
 
     fn compute_writing_mode(&mut self) {

components/style/properties/properties.mako.rs

@@ -893,6 +893,49 @@ impl<'a> Iterator for LonghandIdSetIterator<'a> {
     }
 }
 
+<%
+
+CASCADE_GROUPS = {
+    # The writing-mode group has the most priority of all property groups, as
+    # sizes like font-size can depend on it.
+    "writing_mode": [
+        "writing-mode",
+        "direction",
+        "text-orientation",
+    ],
+    # The fonts and colors group has the second priority, as all other lengths
+    # and colors depend on them.
+    #
+    # There are some interdependencies between these, but we fix them up in
+    # Cascade::fixup_font_stuff.
+    "fonts_and_color": [
+        # Needed to properly compute the zoomed font-size.
+        # FIXME(emilio): This could probably just be a cascade flag
+        # like IN_SVG_SUBTREE or such, and we could nuke this property.
+        "-x-text-zoom",
+        # Needed to do font-size computation in a language-dependent way.
+        "-x-lang",
+        # Needed for ruby to respect language-dependent min-font-size
+        # preferences properly, see bug 1165538.
+        "-moz-min-font-size-ratio",
+        # font-size depends on math-depth's computed value.
+        "math-depth",
+        # Needed to compute the first available font, in order to
+        # compute font-relative units correctly.
+        "font-size",
+        "font-weight",
+        "font-stretch",
+        "font-style",
+        "font-family",
+        # color-scheme affects how system colors resolve.
+        "color-scheme",
+    ],
+}
+def in_late_group(p):
+    return p.name not in CASCADE_GROUPS["writing_mode"] and p.name not in CASCADE_GROUPS["fonts_and_color"]
+
+%>
+
 impl LonghandIdSet {
     #[inline]
     fn reset() -> &'static Self {
@@ -921,7 +964,7 @@ impl LonghandIdSet {
     /// Returns the set of longhands that are ignored when document colors are
     /// disabled.
     #[inline]
-    pub fn ignored_when_colors_disabled() -> &'static Self {
+    fn ignored_when_colors_disabled() -> &'static Self {
         ${static_longhand_id_set(
             "IGNORED_WHEN_COLORS_DISABLED",
             lambda p: p.ignored_when_colors_disabled
@@ -929,6 +972,36 @@ impl LonghandIdSet {
         &IGNORED_WHEN_COLORS_DISABLED
     }
 
+    #[inline]
+    fn writing_mode_group() -> &'static Self {
+        ${static_longhand_id_set(
+            "WRITING_MODE_GROUP",
+            lambda p: p.name in CASCADE_GROUPS["writing_mode"]
+        )}
+        &WRITING_MODE_GROUP
+    }
+
+    #[inline]
+    fn fonts_and_color_group() -> &'static Self {
+        ${static_longhand_id_set(
+            "FONTS_AND_COLOR_GROUP",
+            lambda p: p.name in CASCADE_GROUPS["fonts_and_color"]
+        )}
+        &FONTS_AND_COLOR_GROUP
+    }
+
+    #[inline]
+    fn late_group_only_inherited() -> &'static Self {
+        ${static_longhand_id_set("LATE_GROUP_ONLY_INHERITED", lambda p: p.style_struct.inherited and in_late_group(p))}
+        &LATE_GROUP_ONLY_INHERITED
+    }
+
+    #[inline]
+    fn late_group() -> &'static Self {
+        ${static_longhand_id_set("LATE_GROUP", lambda p: in_late_group(p))}
+        &LATE_GROUP
+    }
+
     /// Returns the set of properties that are declared as having no effect on
     /// Gecko <scrollbar> elements or their descendant scrollbar parts.
     #[cfg(debug_assertions)]
@@ -1362,60 +1435,6 @@ impl LonghandId {
     fn ignored_when_document_colors_disabled(self) -> bool {
         LonghandIdSet::ignored_when_colors_disabled().contains(self)
     }
-
-    /// The computed value of some properties depends on the (sometimes
-    /// computed) value of *other* properties.
-    ///
-    /// So we classify properties into "early" and "other", such that the only
-    /// dependencies can be from "other" to "early".
-    ///
-    /// Unfortunately, it’s not easy to check that this classification is
-    /// correct.
-    fn is_early_property(&self) -> bool {
-        matches!(*self,
-            % if engine == "gecko":
-
-            // Needed to properly compute the writing mode, to resolve logical
-            // properties, and similar stuff. In this block instead of along
-            // `WritingMode` and `Direction` just for convenience, since it's
-            // Gecko-only (for now at least).
-            //
-            // see WritingMode::new.
-            LonghandId::TextOrientation |
-
-            // Needed to properly compute the zoomed font-size.
-            //
-            // FIXME(emilio): This could probably just be a cascade flag like
-            // IN_SVG_SUBTREE or such, and we could nuke this property.
-            LonghandId::XTextZoom |
-
-            // Needed to do font-size computation in a language-dependent way.
-            LonghandId::XLang |
-            // Needed for ruby to respect language-dependent min-font-size
-            // preferences properly, see bug 1165538.
-            LonghandId::MozMinFontSizeRatio |
-
-            // font-size depends on math-depth's computed value.
-            LonghandId::MathDepth |
-
-            // color-scheme affects how system colors resolve.
-            LonghandId::ColorScheme |
-            % endif
-
-            // Needed to compute the first available font, in order to
-            // compute font-relative units correctly.
-            LonghandId::FontSize |
-            LonghandId::FontWeight |
-            LonghandId::FontStretch |
-            LonghandId::FontStyle |
-            LonghandId::FontFamily |
-
-            // Needed to properly compute the writing mode, to resolve logical
-            // properties, and similar stuff.
-            LonghandId::WritingMode |
-            LonghandId::Direction
-        )
-    }
 }
 
 /// An iterator over all the property ids that are enabled for a given