Bug 1498943 - Don't copy structs to write the same value to them. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 15 Oct 2018 03:04:44 +0000
changeset 489613 434e41d68832d1cabc346286fe6874c0dd6f34c7
parent 489612 988a4a44d33160beeec1f09237c1d1ee011c8307
child 489614 69252f7e6d81935bacf4f12e45aa055026328a50
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersheycam
bugs1498943
milestone64.0a1
Bug 1498943 - Don't copy structs to write the same value to them. r=heycam This makes us not allocate useless style structs when you're doing something like resetting an already-reset property, or inheriting an already-inherited property. Seemed simple enough that I think we should do it. In practice we don't even should pay an extra branch because I expect the compiler to be smart enough and merge it with the one in the mutate() call. Differential Revision: https://phabricator.services.mozilla.com/D8685
servo/components/style/properties/properties.mako.rs
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -3132,32 +3132,49 @@ pub enum StyleStructRef<'a, T: 'static> 
     Borrowed(&'a BuilderArc<T>),
     /// An owned struct, that we've already mutated.
     Owned(UniqueArc<T>),
     /// Temporarily vacated, will panic if accessed
     Vacated,
 }
 
 impl<'a, T: 'a> StyleStructRef<'a, T>
-    where T: Clone,
+where
+    T: Clone,
 {
     /// Ensure a mutable reference of this value exists, either cloning the
     /// borrowed value, or returning the owned one.
     pub fn mutate(&mut self) -> &mut T {
         if let StyleStructRef::Borrowed(v) = *self {
             *self = StyleStructRef::Owned(UniqueArc::new((**v).clone()));
         }
 
         match *self {
             StyleStructRef::Owned(ref mut v) => v,
             StyleStructRef::Borrowed(..) => unreachable!(),
             StyleStructRef::Vacated => panic!("Accessed vacated style struct")
         }
     }
 
+    /// Whether this is pointer-equal to the struct we're going to copy the
+    /// value from.
+    ///
+    /// This is used to avoid allocations when people write stuff like `font:
+    /// inherit` or such `all: initial`.
+    #[inline]
+    pub fn ptr_eq(&self, struct_to_copy_from: &T) -> bool {
+        match *self {
+            StyleStructRef::Owned(..) => false,
+            StyleStructRef::Borrowed(arc) => {
+                &**arc as *const T == struct_to_copy_from as *const T
+            }
+            StyleStructRef::Vacated => panic!("Accessed vacated style struct")
+        }
+    }
+
     /// Extract a unique Arc from this struct, vacating it.
     ///
     /// The vacated state is a transient one, please put the Arc back
     /// when done via `put()`. This function is to be used to separate
     /// the struct being mutated from the computed context
     pub fn take(&mut self) -> UniqueArc<T> {
         use std::mem::replace;
         let inner = replace(self, StyleStructRef::Vacated);
@@ -3383,16 +3400,20 @@ impl<'a> StyleBuilder<'a> {
         % if property.ident == "content":
         self.flags.insert(ComputedValueFlags::INHERITS_CONTENT);
         % endif
 
         % if property.ident == "display":
         self.flags.insert(ComputedValueFlags::INHERITS_DISPLAY);
         % endif
 
+        if self.${property.style_struct.ident}.ptr_eq(inherited_struct) {
+            return;
+        }
+
         self.${property.style_struct.ident}.mutate()
             .copy_${property.ident}_from(
                 inherited_struct,
                 % if property.logical:
                 self.writing_mode,
                 % endif
             );
     }
@@ -3402,20 +3423,20 @@ impl<'a> StyleBuilder<'a> {
     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
 
-        // TODO(emilio): There's a maybe-worth it optimization here: We should
-        // avoid allocating a new reset struct if `reset_struct` and our struct
-        // is the same pointer. Would remove a bunch of stupid allocations if
-        // you did something like `* { all: initial }` or what not.
+        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
             );
     }