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 499709 434e41d68832d1cabc346286fe6874c0dd6f34c7
parent 499708 988a4a44d33160beeec1f09237c1d1ee011c8307
child 499710 69252f7e6d81935bacf4f12e45aa055026328a50
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1498943
milestone64.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 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
             );
     }