From 7c538e8147a1eb661fc3cc921a5844bfb41904e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 18 Jun 2023 14:46:52 +0200 Subject: [PATCH] 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 | 120 +++++++--------- .../style/properties/properties.mako.rs | 129 ++++++++++-------- 2 files changed, 127 insertions(+), 122 deletions(-) diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index f36109d059e..fb615375b19 100644 --- a/components/style/properties/cascade.rs +++ b/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(); - - cascade.apply_properties::( - ApplyResetProperties::Yes, + if cascade.apply_properties( + CanHaveLogicalProperties::No, + 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 { - ApplyResetProperties::No + let properties_to_apply = if using_cached_reset_properties { + LonghandIdSet::late_group_only_inherited() } else { - ApplyResetProperties::Yes + LonghandIdSet::late_group() }; - cascade.apply_properties::( - apply_reset, + cascade.apply_properties( + 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 - Phase: CascadePhase, + ) -> bool + where I: Iterator, { - let apply_reset = apply_reset == ApplyResetProperties::Yes; + if !self.referenced.contains_any(properties_to_apply) { + return false; + } - 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 can_have_logical_properties = can_have_logical_properties == CanHaveLogicalProperties::Yes; 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 !apply_reset && !inherited { + if !properties_to_apply.contains(longhand_id) { continue; } - if Phase::is_early() != longhand_id.is_early_property() { - continue; - } - - debug_assert!(!Phase::is_early() || !longhand_id.is_logical()); - let physical_longhand_id = if Phase::is_early() { - longhand_id - } else { + debug_assert!(can_have_logical_properties || !longhand_id.is_logical()); + let physical_longhand_id = if can_have_logical_properties { 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::Initial => !inherited, + CSSWideKeyword::Inherit => longhand_id.inherited(), + CSSWideKeyword::Initial => !longhand_id.inherited(), }, None => false, }; @@ -713,12 +704,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { } } - if Phase::is_early() { - self.fixup_font_stuff(); - self.compute_writing_mode(); - } else { - self.finished_applying_properties(); - } + true } fn compute_writing_mode(&mut self) { diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index e4f0ecc848e..7b6f8380d2d 100644 --- a/components/style/properties/properties.mako.rs +++ b/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 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