From fae0e8e68732394e0ea53bf42fbe59d70e5d0d99 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Mon, 15 Oct 2018 03:04:44 +0000
Subject: [PATCH] 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

--HG--
extra : moz-landing-system : lando
---
 .../style/properties/properties.mako.rs       | 31 ++++++++++++++++---
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/servo/components/style/properties/properties.mako.rs b/servo/components/style/properties/properties.mako.rs
index 5968d84bd33b..109f21960c16 100644
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -3137,7 +3137,8 @@ pub enum StyleStructRef<'a, T: 'static> {
 }
 
 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.
@@ -3153,6 +3154,22 @@ impl<'a, T: 'a> StyleStructRef<'a, T>
         }
     }
 
+    /// 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
@@ -3388,6 +3405,10 @@ impl<'a> StyleBuilder<'a> {
         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,
@@ -3407,10 +3428,10 @@ impl<'a> StyleBuilder<'a> {
         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,