Bug 1353208 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue; r=dholbert
authorBrian Birtles <birtles@gmail.com>
Thu, 30 Mar 2017 13:10:07 +0900
changeset 351216 ad7060dae117652c429ae599c9f1bdb2ae4018bb
parent 351215 7788981a1e0255061121aa50e844e785ef008bcd
child 351217 0d77b2992fc83adc2ff5f6916785149310129a99
push id40095
push userbbirtles@mozilla.com
push dateWed, 05 Apr 2017 06:15:05 +0000
treeherderautoland@95e21766cd72 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1353208, 533291
milestone55.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 1353208 - Don't allocate separate heap memory for nsSMILCompositor::mCachedBaseValue; r=dholbert nsSMILCompositor::mCachedBaseValue is an nsAutoPtr<nsSMILValue> that we allocate on the heap. It looks like we did that back in bug 533291 presumably because it makes transferring these cached values between nsSMILCompositor objects cheaper. One drawback of this, however, is that mCachedBaseValue has two null states: the mCachedBaseValue pointer can be null, and the pointed-to nsSMILValue can be a null value (i.e. IsNull() returns true). Now that we have move ctors and operators defined for nsSMILValue we can transfer these objects between compositors cheaply without requiring the object to be allocated as separate heap object. This patch makes mCachedBaseValue just a regular nsSMILValue class member (i.e. drops the nsAutoPtr). There's a subtle difference in behavior with regards to the first sample. Previously we would compare the (initially) null mCachedBaseValue pointer with the passed-in nsSMILValue and set mForceCompositing to true. With this patch, however, we will only set mForceCompositing to true if the passed-in mCachedBaseValue is not null. I believe this is correct, however, since if we don't call GetBaseValue in ComposeAttribute we should not be setting mForceCompositing to true (something else should ensure that gets set to true), and if we do call GetBaseValue the result should not be a null nsSMILValue (except in some OOM cases where we don't really care if we miss a sample). This patch adds an assertion to check that GetBaseValue does, in fact, return a non-null value. (I checked the code and this appears to be the case. Even in error cases we typically return an empty nsSMILValue of a non-null type. For example, the early return in nsSMILCSSProperty::GetBaseValue() does this.) MozReview-Commit-ID: BRJFa4xMdxz
dom/smil/nsSMILCompositor.cpp
dom/smil/nsSMILCompositor.h
--- a/dom/smil/nsSMILCompositor.cpp
+++ b/dom/smil/nsSMILCompositor.cpp
@@ -77,16 +77,18 @@ nsSMILCompositor::ComposeAttribute(bool&
   // THIRD: Step backwards through animation functions to find out
   // which ones we actually care about.
   uint32_t firstFuncToCompose = GetFirstFuncToAffectSandwich();
 
   // FOURTH: Get & cache base value
   nsSMILValue sandwichResultValue;
   if (!mAnimationFunctions[firstFuncToCompose]->WillReplace()) {
     sandwichResultValue = smilAttr->GetBaseValue();
+    MOZ_ASSERT(!sandwichResultValue.IsNull(),
+               "Result of GetBaseValue should not be null");
   }
   UpdateCachedBaseValue(sandwichResultValue);
 
   if (!mForceCompositing) {
     return;
   }
 
   // FIFTH: Compose animation functions
@@ -193,19 +195,14 @@ nsSMILCompositor::GetFirstFuncToAffectSa
     }
   }
   return i;
 }
 
 void
 nsSMILCompositor::UpdateCachedBaseValue(const nsSMILValue& aBaseValue)
 {
-  if (!mCachedBaseValue) {
-    // We don't have last sample's base value cached. Assume it's changed.
-    mCachedBaseValue = new nsSMILValue(aBaseValue);
-    NS_WARNING_ASSERTION(mCachedBaseValue, "failed to cache base value (OOM?)");
-    mForceCompositing = true;
-  } else if (*mCachedBaseValue != aBaseValue) {
+  if (mCachedBaseValue != aBaseValue) {
     // Base value has changed since last sample.
-    *mCachedBaseValue = aBaseValue;
+    mCachedBaseValue = aBaseValue;
     mForceCompositing = true;
   }
 }
--- a/dom/smil/nsSMILCompositor.h
+++ b/dom/smil/nsSMILCompositor.h
@@ -4,17 +4,16 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef NS_SMILCOMPOSITOR_H_
 #define NS_SMILCOMPOSITOR_H_
 
 #include "mozilla/Move.h"
 #include "mozilla/UniquePtr.h"
-#include "nsAutoPtr.h"
 #include "nsTHashtable.h"
 #include "nsString.h"
 #include "nsSMILAnimationFunction.h"
 #include "nsSMILTargetIdentifier.h"
 #include "nsSMILCompositorTable.h"
 #include "PLDHashTable.h"
 
 //----------------------------------------------------------------------
@@ -94,14 +93,15 @@ public:
 
   // Member data for detecting when we need to force-recompose
   // ---------------------------------------------------------
   // Flag for tracking whether we need to compose. Initialized to false, but
   // gets flipped to true if we detect that something has changed.
   bool mForceCompositing;
 
   // Cached base value, so we can detect & force-recompose when it changes
-  // from one sample to the next. (nsSMILAnimationController copies this
-  // forward from the previous sample's compositor.)
-  nsAutoPtr<nsSMILValue> mCachedBaseValue;
+  // from one sample to the next. (nsSMILAnimationController moves this
+  // forward from the previous sample's compositor by calling
+  // StealCachedBaseValue.)
+  nsSMILValue mCachedBaseValue;
 };
 
 #endif // NS_SMILCOMPOSITOR_H_