Bug 1195180 part 7 - Store animations in an array; r=heycam
authorBrian Birtles <birtles@gmail.com>
Mon, 28 Sep 2015 12:38:41 +0900
changeset 264671 3ef6210305ae5d7a3aad1d9c0dff2ca23410e3f6
parent 264670 8406c5300ab7051ae6fe9bf41a1d30261cf70a4a
child 264672 05dfb6716db36f5f6045264d243eb24c595288c1
push id29444
push usercbook@mozilla.com
push dateMon, 28 Sep 2015 12:17:21 +0000
treeherdermozilla-central@031db40e2b55 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1195180
milestone44.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 1195180 part 7 - Store animations in an array; r=heycam Currently AnimationTimeline stores animations in a hashmap which means that when we go to iterate over those animations to tick them we will visit them in an order that is non-deterministic. Although many of the observable effects of ticking an animation (e.g. CSS animation/transition events, mutation observer events) are later sorted so that the result does not depend on the order in which animations are ticked, this is not true for in all cases. In particular, the order in which Animation.finished promises are resolved will vary depending on the order in which animations are ticked. Likewise, for Animation finish events. Furthermore, it seems generally desirable to have a deterministic order for visiting animations in order to aid reproducing bugs. To achieve this, this patch switches the storage of animations in AnimationTimeline to use an array instead. However, when adding animations we need to determine if the animation to add already exists. To this end we also maintain a hashmap of the animations so we can quickly determine if the animation to add is a duplicate or not.
dom/animation/AnimationTimeline.cpp
dom/animation/AnimationTimeline.h
dom/animation/DocumentTimeline.cpp
--- a/dom/animation/AnimationTimeline.cpp
+++ b/dom/animation/AnimationTimeline.cpp
@@ -5,17 +5,18 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "AnimationTimeline.h"
 #include "mozilla/AnimationComparator.h"
 
 namespace mozilla {
 namespace dom {
 
-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnimationTimeline, mWindow, mAnimations)
+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(AnimationTimeline, mWindow,
+                                      mAnimationOrder)
 
 NS_IMPL_CYCLE_COLLECTING_ADDREF(AnimationTimeline)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(AnimationTimeline)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AnimationTimeline)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
@@ -26,18 +27,19 @@ AnimationTimeline::GetAnimations(Animati
   nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mWindow);
   if (mWindow) {
     nsIDocument* doc = window->GetDoc();
     if (doc) {
       doc->FlushPendingNotifications(Flush_Style);
     }
   }
 
-  for (auto iter = mAnimations.Iter(); !iter.Done(); iter.Next()) {
-    Animation* animation = iter.Get()->GetKey();
+  aAnimations.SetCapacity(mAnimationOrder.Length());
+
+  for (Animation* animation : mAnimationOrder) {
 
     // Skip animations which are no longer relevant or which have been
     // associated with another timeline. These animations will be removed
     // on the next tick.
     if (!animation->IsRelevant() || animation->GetTimeline() != this) {
       continue;
     }
 
@@ -58,13 +60,18 @@ AnimationTimeline::GetAnimations(Animati
 
   // Sort animations by priority
   aAnimations.Sort(AnimationPtrComparator<nsRefPtr<Animation>>());
 }
 
 void
 AnimationTimeline::NotifyAnimationUpdated(Animation& aAnimation)
 {
+  if (mAnimations.Contains(&aAnimation)) {
+    return;
+  }
+
   mAnimations.PutEntry(&aAnimation);
+  mAnimationOrder.AppendElement(&aAnimation);
 }
 
 } // namespace dom
 } // namespace mozilla
--- a/dom/animation/AnimationTimeline.h
+++ b/dom/animation/AnimationTimeline.h
@@ -90,16 +90,25 @@ public:
    * determine if it needs to begin, continue or stop tracking this animation.
    */
   virtual void NotifyAnimationUpdated(Animation& aAnimation);
 
 protected:
   nsCOMPtr<nsIGlobalObject> mWindow;
 
   // Animations observing this timeline
-  typedef nsTHashtable<nsRefPtrHashKey<dom::Animation>> AnimationSet;
-  AnimationSet mAnimations;
+  //
+  // We store them in (a) a hashset for quick lookup, and (b) an array
+  // to maintain a fixed sampling order.
+  //
+  // The array keeps a strong reference to each animation in order
+  // to save some addref/release traffic and because we never dereference
+  // the pointers in the hashset.
+  typedef nsTHashtable<nsPtrHashKey<dom::Animation>> AnimationSet;
+  typedef nsTArray<nsRefPtr<dom::Animation>>         AnimationArray;
+  AnimationSet   mAnimations;
+  AnimationArray mAnimationOrder;
 };
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_AnimationTimeline_h
--- a/dom/animation/DocumentTimeline.cpp
+++ b/dom/animation/DocumentTimeline.cpp
@@ -106,30 +106,31 @@ DocumentTimeline::NotifyAnimationUpdated
 }
 
 void
 DocumentTimeline::WillRefresh(mozilla::TimeStamp aTime)
 {
   MOZ_ASSERT(mIsObservingRefreshDriver);
 
   bool needsTicks = false;
+  AnimationArray animationsToKeep(mAnimationOrder.Length());
 
-  for (auto iter = mAnimations.Iter(); !iter.Done(); iter.Next()) {
-    Animation* animation = iter.Get()->GetKey();
-
-    // Drop any animations which no longer need to be tracked by this timeline.
+  for (Animation* animation : mAnimationOrder) {
     if (animation->GetTimeline() != this ||
         (!animation->IsRelevant() && !animation->NeedsTicks())) {
-      iter.Remove();
+      mAnimations.RemoveEntry(animation);
       continue;
     }
 
     needsTicks |= animation->NeedsTicks();
+    animationsToKeep.AppendElement(animation);
   }
 
+  mAnimationOrder.SwapElements(animationsToKeep);
+
   if (!needsTicks) {
     // If another refresh driver observer destroys the nsPresContext,
     // nsRefreshDriver will detect it and we won't be called.
     MOZ_ASSERT(GetRefreshDriver(),
                "Refresh driver should still be valid inside WillRefresh");
     GetRefreshDriver()->RemoveRefreshObserver(this, Flush_Style);
     mIsObservingRefreshDriver = false;
   }
@@ -137,17 +138,17 @@ DocumentTimeline::WillRefresh(mozilla::T
 
 void
 DocumentTimeline::NotifyRefreshDriverCreated(nsRefreshDriver* aDriver)
 {
   MOZ_ASSERT(!mIsObservingRefreshDriver,
              "Timeline should not be observing the refresh driver before"
              " it is created");
 
-  if (mAnimations.Count()) {
+  if (!mAnimationOrder.IsEmpty()) {
     aDriver->AddRefreshObserver(this, Flush_Style);
     mIsObservingRefreshDriver = true;
   }
 }
 
 void
 DocumentTimeline::NotifyRefreshDriverDestroying(nsRefreshDriver* aDriver)
 {