Bug 1215878 - Optimize cascading of other wide keywords if possible. r=xidorn
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 07 Mar 2019 12:48:07 +0000
changeset 520751 0d33ffb859fa4e1ea031c3b2cb3274d71d66a0d5
parent 520750 1ae26ce1cf090db6b0b19ea7d7eccd42373dd5fa
child 520752 931c2305411f2d77d65f88e7c4d70d29b2ac415f
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersxidorn
bugs1215878
milestone67.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 1215878 - Optimize cascading of other wide keywords if possible. r=xidorn The way the copy-on-write stuff works, and the way that we have to apply properties from most specific to less specific guarantees that always that we're going to inherit an inherited property, or reset a reset property, we have already the right value on the style. Revert relies on that, so there doesn't seem to be a reason to not use that fact more often and skip useless work earlier. Font-size is still special of course... I think I have a way to move the specialness outside of the style, but piece by piece. Differential Revision: https://phabricator.services.mozilla.com/D21882
servo/components/style/properties/cascade.rs
servo/components/style/properties/helpers.mako.rs
servo/components/style/properties/properties.mako.rs
--- a/servo/components/style/properties/cascade.rs
+++ b/servo/components/style/properties/cascade.rs
@@ -6,17 +6,17 @@
 
 use crate::context::QuirksMode;
 use crate::custom_properties::CustomPropertiesBuilder;
 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};
+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::selector_parser::PseudoElement;
 use crate::stylesheets::{Origin, PerOrigin};
 use servo_arc::Arc;
 use crate::shared_lock::StylesheetGuards;
@@ -537,27 +537,44 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
                     self.context.builder.pseudo,
                     &mut declaration,
                 );
                 if should_ignore {
                     continue;
                 }
             }
 
-            if declaration.is_revert() {
+            let css_wide_keyword = declaration.get_css_wide_keyword();
+            if let Some(CSSWideKeyword::Revert) = css_wide_keyword {
+                // We intentionally don't want to insert it into `self.seen`,
+                // `reverted` takes care of rejecting other declarations as
+                // needed.
                 for origin in origin.following_including() {
                     self.reverted
                         .borrow_mut_for_origin(&origin)
                         .insert(physical_longhand_id);
                 }
                 continue;
             }
 
             self.seen.insert(physical_longhand_id);
 
+            let unset = css_wide_keyword.map_or(false, |css_wide_keyword| {
+                match css_wide_keyword {
+                    CSSWideKeyword::Unset => true,
+                    CSSWideKeyword::Inherit => inherited,
+                    CSSWideKeyword::Initial => !inherited,
+                    CSSWideKeyword::Revert => unreachable!(),
+                }
+            });
+
+            if unset {
+                continue;
+            }
+
             // FIXME(emilio): We should avoid generating code for logical
             // longhands and just use the physical ones, then rename
             // physical_longhand_id to just longhand_id.
             self.apply_declaration::<Phase>(longhand_id, &*declaration);
         }
 
         if Phase::is_early() {
             self.fixup_font_and_apply_saved_font_properties();
@@ -806,25 +823,21 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
         } else {
             #[cfg(feature = "gecko")]
             {
                 if self.seen.contains(LonghandId::XLang) ||
                     self.seen.contains(LonghandId::MozScriptLevel) ||
                     self.seen.contains(LonghandId::MozMinFontSizeRatio) ||
                     self.seen.contains(LonghandId::FontFamily)
                 {
-                    use crate::properties::{CSSWideKeyword, WideKeywordDeclaration};
+                    use crate::values::computed::FontSize;
 
                     // font-size must be explicitly inherited to handle lang
                     // changes and scriptlevel changes.
                     //
                     // FIXME(emilio): That looks a bit bogus...
-                    let inherit = PropertyDeclaration::CSSWideKeyword(WideKeywordDeclaration {
-                        id: LonghandId::FontSize,
-                        keyword: CSSWideKeyword::Inherit,
-                    });
-
-                    self.apply_declaration_ignoring_phase(LonghandId::FontSize, &inherit);
+                    self.context.for_non_inherited_property = None;
+                    FontSize::cascade_inherit_font_size(&mut self.context);
                 }
             }
         }
     }
 }
--- a/servo/components/style/properties/helpers.mako.rs
+++ b/servo/components/style/properties/helpers.mako.rs
@@ -321,36 +321,38 @@
                     Some(LonghandId::${property.camel_case});
                 % endif
 
             let specified_value = match *declaration {
                 PropertyDeclaration::${property.camel_case}(ref value) => value,
                 PropertyDeclaration::CSSWideKeyword(ref declaration) => {
                     debug_assert_eq!(declaration.id, LonghandId::${property.camel_case});
                     match declaration.keyword {
-                        % if not data.current_style_struct.inherited:
+                        % if not property.style_struct.inherited:
                         CSSWideKeyword::Unset |
                         % endif
                         CSSWideKeyword::Initial => {
-                            % if property.ident == "font_size":
+                            % if not property.style_struct.inherited:
+                                debug_assert!(false, "Should be handled in apply_properties");
+                            % else:
+                                % if property.name == "font-size":
                                 computed::FontSize::cascade_initial_font_size(context);
-                            % else:
+                                % else:
                                 context.builder.reset_${property.ident}();
+                                % endif
                             % endif
                         },
-                        % if data.current_style_struct.inherited:
+                        % if property.style_struct.inherited:
                         CSSWideKeyword::Unset |
                         % endif
                         CSSWideKeyword::Inherit => {
-                            % if not property.style_struct.inherited:
+                            % if property.style_struct.inherited:
+                                debug_assert!(false, "Should be handled in apply_properties");
+                            % else:
                                 context.rule_cache_conditions.borrow_mut().set_uncacheable();
-                            % endif
-                            % if property.ident == "font_size":
-                                computed::FontSize::cascade_inherit_font_size(context);
-                            % else:
                                 context.builder.inherit_${property.ident}();
                             % endif
                         }
                         CSSWideKeyword::Revert => unreachable!("Should never get here"),
                     }
                     return;
                 }
                 PropertyDeclaration::WithVariables(..) => {
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -2112,22 +2112,16 @@ impl PropertyDeclaration {
         match *self {
             PropertyDeclaration::CSSWideKeyword(ref declaration) => {
                 Some(declaration.keyword)
             },
             _ => None,
         }
     }
 
-    /// Whether this declaration is the `revert` keyword.
-    #[inline]
-    pub fn is_revert(&self) -> bool {
-        self.get_css_wide_keyword().map_or(false, |kw| kw == CSSWideKeyword::Revert)
-    }
-
     /// Returns whether or not the property is set by a system font
     pub fn get_system(&self) -> Option<SystemFont> {
         match *self {
             % if product == "gecko":
             % for prop in SYSTEM_FONT_LONGHANDS:
                 PropertyDeclaration::${to_camel_case(prop)}(ref prop) => {
                     prop.get_system()
                 }
@@ -3442,32 +3436,26 @@ impl<'a> StyleBuilder<'a> {
         % if not style_struct.inherited:
         self.${style_struct.ident} =
             StyleStructRef::Borrowed(style.${style_struct.name_lower}_arc());
         % endif
         % endfor
     }
 
     % for property in data.longhands:
-    % if property.ident != "font_size":
+    % if not property.style_struct.inherited:
     /// Inherit `${property.ident}` from our parent style.
     #[allow(non_snake_case)]
     pub fn inherit_${property.ident}(&mut self) {
         let inherited_struct =
-        % if property.style_struct.inherited:
-            self.inherited_style.get_${property.style_struct.name_lower}();
-        % else:
             self.inherited_style_ignoring_first_line
                 .get_${property.style_struct.name_lower}();
-        % endif
-
-        % if not property.style_struct.inherited:
+
+        self.modified_reset = true;
         self.flags.insert(ComputedValueFlags::INHERITS_RESET_STYLE);
-        self.modified_reset = true;
-        % endif
 
         % if property.ident == "content":
         self.flags.insert(ComputedValueFlags::INHERITS_CONTENT);
         % endif
 
         % if property.ident == "display":
         self.flags.insert(ComputedValueFlags::INHERITS_DISPLAY);
         % endif
@@ -3479,39 +3467,36 @@ impl<'a> StyleBuilder<'a> {
         self.${property.style_struct.ident}.mutate()
             .copy_${property.ident}_from(
                 inherited_struct,
                 % if property.logical:
                 self.writing_mode,
                 % endif
             );
     }
-
+    % elif property.name != "font-size":
     /// Reset `${property.ident}` to the initial value.
     #[allow(non_snake_case)]
     pub fn reset_${property.ident}(&mut self) {
         let reset_struct =
             self.reset_style.get_${property.style_struct.name_lower}();
 
-        % if not property.style_struct.inherited:
-        self.modified_reset = true;
-        % endif
-
         if self.${property.style_struct.ident}.ptr_eq(reset_struct) {
             return;
         }
 
         self.${property.style_struct.ident}.mutate()
             .reset_${property.ident}(
                 reset_struct,
                 % if property.logical:
                 self.writing_mode,
                 % endif
             );
     }
+    % endif
 
     % if not property.is_vector:
     /// Set the `${property.ident}` to the computed value `value`.
     #[allow(non_snake_case)]
     pub fn set_${property.ident}(
         &mut self,
         value: longhands::${property.ident}::computed_value::T
     ) {
@@ -3523,17 +3508,16 @@ impl<'a> StyleBuilder<'a> {
             .set_${property.ident}(
                 value,
                 % if property.logical:
                 self.writing_mode,
                 % endif
             );
     }
     % endif
-    % endif
     % endfor
     <% del property %>
 
     /// Inherits style from the parent element, accounting for the default
     /// computed values that need to be provided as well.
     pub fn for_inheritance(
         device: &'a Device,
         parent: Option<<&'a ComputedValues>,