Bug 1194912 (Part 2) - Store ProgressTracker observers in a copy-on-write hash table, and dispatch notifications to them using a template. r=tn
authorSeth Fowler <mark.seth.fowler@gmail.com>
Tue, 25 Aug 2015 16:26:43 -0700
changeset 259337 4797d5683bf366faca78d415e7c7d1af83e3c0cc
parent 259336 c61c1035879cad1758b2e005bbdf646bccf5b58c
child 259338 7a1c73b41bb5dd5d9435b70b376257fc9ba3909f
push id64206
push usermfowler@mozilla.com
push dateTue, 25 Aug 2015 23:28:02 +0000
treeherdermozilla-inbound@4797d5683bf3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstn
bugs1194912
milestone43.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 1194912 (Part 2) - Store ProgressTracker observers in a copy-on-write hash table, and dispatch notifications to them using a template. r=tn
image/ProgressTracker.cpp
image/ProgressTracker.h
--- a/image/ProgressTracker.cpp
+++ b/image/ProgressTracker.cpp
@@ -238,82 +238,128 @@ ProgressTracker::NotifyCurrentState(IPro
 
   aObserver->SetNotificationsDeferred(true);
 
   nsCOMPtr<nsIRunnable> ev = new AsyncNotifyCurrentStateRunnable(this,
                                                                  aObserver);
   NS_DispatchToCurrentThread(ev);
 }
 
-#define NOTIFY_IMAGE_OBSERVERS(OBSERVERS, FUNC) \
-  do { \
-    ObserverArray::ForwardIterator iter(OBSERVERS); \
-    while (iter.HasMore()) { \
-      nsRefPtr<IProgressObserver> observer = iter.GetNext().get(); \
-      if (observer && !observer->NotificationsDeferred()) { \
-        observer->FUNC; \
-      } \
-    } \
-  } while (false);
+/**
+ * ImageObserverNotifier is a helper type that abstracts over the difference
+ * between sending notifications to all of the observers in an ObserverTable,
+ * and sending them to a single observer. This allows the same notification code
+ * to be used for both cases.
+ */
+template <typename T> struct ImageObserverNotifier;
+
+template <>
+struct MOZ_STACK_CLASS ImageObserverNotifier<const ObserverTable*>
+{
+  explicit ImageObserverNotifier(const ObserverTable* aObservers,
+                                 bool aIgnoreDeferral = false)
+    : mObservers(aObservers)
+    , mIgnoreDeferral(aIgnoreDeferral)
+  { }
+
+  template <typename Lambda>
+  void operator()(Lambda aFunc)
+  {
+    for (auto iter = mObservers->ConstIter(); !iter.Done(); iter.Next()) {
+      nsRefPtr<IProgressObserver> observer = iter.Data().get();
+      if (observer &&
+          (mIgnoreDeferral || !observer->NotificationsDeferred())) {
+        aFunc(observer);
+      }
+    }
+  }
 
-/* static */ void
-ProgressTracker::SyncNotifyInternal(ObserverArray& aObservers,
-                                    bool aHasImage,
-                                    Progress aProgress,
-                                    const nsIntRect& aDirtyRect)
+private:
+  const ObserverTable* mObservers;
+  const bool mIgnoreDeferral;
+};
+
+template <>
+struct MOZ_STACK_CLASS ImageObserverNotifier<IProgressObserver*>
+{
+  explicit ImageObserverNotifier(IProgressObserver* aObserver)
+    : mObserver(aObserver)
+  { }
+
+  template <typename Lambda>
+  void operator()(Lambda aFunc)
+  {
+    if (mObserver && !mObserver->NotificationsDeferred()) {
+      aFunc(mObserver);
+    }
+  }
+
+private:
+  IProgressObserver* mObserver;
+};
+
+template <typename T> void
+SyncNotifyInternal(const T& aObservers,
+                   bool aHasImage,
+                   Progress aProgress,
+                   const nsIntRect& aDirtyRect)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   typedef imgINotificationObserver I;
+  ImageObserverNotifier<T> notify(aObservers);
 
   if (aProgress & FLAG_SIZE_AVAILABLE) {
-    NOTIFY_IMAGE_OBSERVERS(aObservers, Notify(I::SIZE_AVAILABLE));
+    notify([](IProgressObserver* aObs) { aObs->Notify(I::SIZE_AVAILABLE); });
   }
 
   if (aProgress & FLAG_ONLOAD_BLOCKED) {
-    NOTIFY_IMAGE_OBSERVERS(aObservers, BlockOnload());
+    notify([](IProgressObserver* aObs) { aObs->BlockOnload(); });
   }
 
   if (aHasImage) {
     // OnFrameUpdate
     // If there's any content in this frame at all (always true for
     // vector images, true for raster images that have decoded at
     // least one frame) then send OnFrameUpdate.
     if (!aDirtyRect.IsEmpty()) {
-      NOTIFY_IMAGE_OBSERVERS(aObservers, Notify(I::FRAME_UPDATE, &aDirtyRect));
+      notify([&](IProgressObserver* aObs) {
+        aObs->Notify(I::FRAME_UPDATE, &aDirtyRect);
+      });
     }
 
     if (aProgress & FLAG_FRAME_COMPLETE) {
-      NOTIFY_IMAGE_OBSERVERS(aObservers, Notify(I::FRAME_COMPLETE));
+      notify([](IProgressObserver* aObs) { aObs->Notify(I::FRAME_COMPLETE); });
     }
 
     if (aProgress & FLAG_HAS_TRANSPARENCY) {
-      NOTIFY_IMAGE_OBSERVERS(aObservers, Notify(I::HAS_TRANSPARENCY));
+      notify([](IProgressObserver* aObs) { aObs->Notify(I::HAS_TRANSPARENCY); });
     }
 
     if (aProgress & FLAG_IS_ANIMATED) {
-      NOTIFY_IMAGE_OBSERVERS(aObservers, Notify(I::IS_ANIMATED));
+      notify([](IProgressObserver* aObs) { aObs->Notify(I::IS_ANIMATED); });
     }
   }
 
   // Send UnblockOnload before OnStopDecode and OnStopRequest. This allows
   // observers that can fire events when they receive those notifications to do
   // so then, instead of being forced to wait for UnblockOnload.
   if (aProgress & FLAG_ONLOAD_UNBLOCKED) {
-    NOTIFY_IMAGE_OBSERVERS(aObservers, UnblockOnload());
+    notify([](IProgressObserver* aObs) { aObs->UnblockOnload(); });
   }
 
   if (aProgress & FLAG_DECODE_COMPLETE) {
     MOZ_ASSERT(aHasImage, "Stopped decoding without ever having an image?");
-    NOTIFY_IMAGE_OBSERVERS(aObservers, Notify(I::DECODE_COMPLETE));
+    notify([](IProgressObserver* aObs) { aObs->Notify(I::DECODE_COMPLETE); });
   }
 
   if (aProgress & FLAG_LOAD_COMPLETE) {
-    NOTIFY_IMAGE_OBSERVERS(aObservers,
-                           OnLoadComplete(aProgress & FLAG_LAST_PART_COMPLETE));
+    notify([=](IProgressObserver* aObs) {
+      aObs->OnLoadComplete(aProgress & FLAG_LAST_PART_COMPLETE);
+    });
   }
 }
 
 void
 ProgressTracker::SyncNotifyProgress(Progress aProgress,
                                     const nsIntRect& aInvalidRect
                                                   /* = nsIntRect() */)
 {
@@ -335,17 +381,19 @@ ProgressTracker::SyncNotifyProgress(Prog
   }
 
   // Apply the changes.
   mProgress |= progress;
 
   CheckProgressConsistency(mProgress);
 
   // Send notifications.
-  SyncNotifyInternal(mObservers, HasImage(), progress, aInvalidRect);
+  mObservers.Read([&](const ObserverTable* aTable) {
+    SyncNotifyInternal(aTable, HasImage(), progress, aInvalidRect);
+  });
 
   if (progress & FLAG_HAS_ERROR) {
     FireFailureNotification();
   }
 }
 
 void
 ProgressTracker::SyncNotify(IProgressObserver* aObserver)
@@ -365,19 +413,17 @@ ProgressTracker::SyncNotify(IProgressObs
   if (image) {
     if (NS_FAILED(image->GetWidth(&rect.width)) ||
         NS_FAILED(image->GetHeight(&rect.height))) {
       // Either the image has no intrinsic size, or it has an error.
       rect = GetMaxSizedIntRect();
     }
   }
 
-  ObserverArray array;
-  array.AppendElement(aObserver);
-  SyncNotifyInternal(array, !!image, mProgress, rect);
+  SyncNotifyInternal(aObserver, !!image, mProgress, rect);
 }
 
 void
 ProgressTracker::EmulateRequestFinished(IProgressObserver* aObserver)
 {
   MOZ_ASSERT(NS_IsMainThread(),
              "SyncNotifyState and mObservers are not threadsafe");
   nsRefPtr<IProgressObserver> kungFuDeathGrip(aObserver);
@@ -390,26 +436,37 @@ ProgressTracker::EmulateRequestFinished(
     aObserver->OnLoadComplete(true);
   }
 }
 
 void
 ProgressTracker::AddObserver(IProgressObserver* aObserver)
 {
   MOZ_ASSERT(NS_IsMainThread());
-  mObservers.AppendElementUnlessExists(aObserver);
+
+  mObservers.Write([=](ObserverTable* aTable) {
+    MOZ_ASSERT(!aTable->Get(aObserver, nullptr),
+               "Adding duplicate entry for image observer");
+
+    WeakPtr<IProgressObserver> weakPtr = aObserver;
+    aTable->Put(aObserver, weakPtr);
+  });
 }
 
 bool
 ProgressTracker::RemoveObserver(IProgressObserver* aObserver)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   // Remove the observer from the list.
-  bool removed = mObservers.RemoveElement(aObserver);
+  bool removed = mObservers.Write([=](ObserverTable* aTable) {
+    bool removed = aTable->Get(aObserver, nullptr);
+    aTable->Remove(aObserver);
+    return removed;
+  });
 
   // Observers can get confused if they don't get all the proper teardown
   // notifications. Part ways on good terms.
   if (removed && !aObserver->NotificationsDeferred()) {
     EmulateRequestFinished(aObserver);
   }
 
   // Make sure we don't give callbacks to an observer that isn't interested in
@@ -420,52 +477,69 @@ ProgressTracker::RemoveObserver(IProgres
   if (aObserver->NotificationsDeferred() && runnable) {
     runnable->RemoveObserver(aObserver);
     aObserver->SetNotificationsDeferred(false);
   }
 
   return removed;
 }
 
+uint32_t
+ProgressTracker::ObserverCount() const
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  return mObservers.Read([](const ObserverTable* aTable) {
+    return aTable->Count();
+  });
+}
+
 void
 ProgressTracker::OnUnlockedDraw()
 {
   MOZ_ASSERT(NS_IsMainThread());
-  NOTIFY_IMAGE_OBSERVERS(mObservers,
-                         Notify(imgINotificationObserver::UNLOCKED_DRAW));
+  mObservers.Read([](const ObserverTable* aTable) {
+    ImageObserverNotifier<const ObserverTable*> notify(aTable);
+    notify([](IProgressObserver* aObs) {
+      aObs->Notify(imgINotificationObserver::UNLOCKED_DRAW);
+    });
+  });
 }
 
 void
 ProgressTracker::ResetForNewRequest()
 {
   MOZ_ASSERT(NS_IsMainThread());
   mProgress = NoProgress;
   CheckProgressConsistency(mProgress);
 }
 
 void
 ProgressTracker::OnDiscard()
 {
   MOZ_ASSERT(NS_IsMainThread());
-  NOTIFY_IMAGE_OBSERVERS(mObservers,
-                         Notify(imgINotificationObserver::DISCARD));
+  mObservers.Read([](const ObserverTable* aTable) {
+    ImageObserverNotifier<const ObserverTable*> notify(aTable);
+    notify([](IProgressObserver* aObs) {
+      aObs->Notify(imgINotificationObserver::DISCARD);
+    });
+  });
 }
 
 void
 ProgressTracker::OnImageAvailable()
 {
   MOZ_ASSERT(NS_IsMainThread());
   // Notify any imgRequestProxys that are observing us that we have an Image.
-  ObserverArray::ForwardIterator iter(mObservers);
-  while (iter.HasMore()) {
-    nsRefPtr<IProgressObserver> observer = iter.GetNext().get();
-    if (observer) {
-      observer->SetHasImage();
-    }
-  }
+  mObservers.Read([](const ObserverTable* aTable) {
+    ImageObserverNotifier<const ObserverTable*>
+      notify(aTable, /* aIgnoreDeferral = */ true);
+    notify([](IProgressObserver* aObs) {
+      aObs->SetHasImage();
+    });
+  });
 }
 
 void
 ProgressTracker::FireFailureNotification()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   // Some kind of problem has happened with image decoding.
--- a/image/ProgressTracker.h
+++ b/image/ProgressTracker.h
@@ -2,19 +2,21 @@
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * 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 mozilla_image_ProgressTracker_h
 #define mozilla_image_ProgressTracker_h
 
+#include "CopyOnWrite.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/WeakPtr.h"
+#include "nsDataHashtable.h"
 #include "nsCOMPtr.h"
 #include "nsTObserverArray.h"
 #include "nsThreadUtils.h"
 #include "nsRect.h"
 #include "IProgressObserver.h"
 
 class nsIRunnable;
 
@@ -53,16 +55,47 @@ inline Progress LoadCompleteProgress(boo
   }
   if (NS_FAILED(aStatus) || aError) {
     progress |= FLAG_HAS_ERROR;
   }
   return progress;
 }
 
 /**
+ * ProgressTracker stores its observers in an ObserverTable, which is a hash
+ * table mapping raw pointers to WeakPtr's to the same objects. This sounds like
+ * unnecessary duplication of information, but it's necessary for stable hash
+ * values since WeakPtr's lose the knowledge of which object they used to point
+ * to when that object is destroyed.
+ *
+ * ObserverTable subclasses nsDataHashtable to add reference counting support
+ * and a copy constructor, both of which are needed for use with CopyOnWrite<T>.
+ */
+class ObserverTable
+  : public nsDataHashtable<nsPtrHashKey<IProgressObserver>,
+                           WeakPtr<IProgressObserver>>
+{
+public:
+  NS_INLINE_DECL_REFCOUNTING(ObserverTable);
+
+  ObserverTable() = default;
+
+  ObserverTable(const ObserverTable& aOther)
+  {
+    NS_WARNING("Forced to copy ObserverTable due to nested notifications");
+    for (auto iter = aOther.ConstIter(); !iter.Done(); iter.Next()) {
+      this->Put(iter.Key(), iter.Data());
+    }
+  }
+
+private:
+  ~ObserverTable() { }
+};
+
+/**
  * ProgressTracker is a class that records an Image's progress through the
  * loading and decoding process, and makes it possible to send notifications to
  * IProgressObservers, both synchronously and asynchronously.
  *
  * When a new observer needs to be notified of the current progress of an image,
  * call the Notify() method on this class with the relevant observer as its
  * argument, and the notifications will be replayed to the observer
  * asynchronously.
@@ -73,16 +106,17 @@ class ProgressTracker : public mozilla::
 
 public:
   MOZ_DECLARE_WEAKREFERENCE_TYPENAME(ProgressTracker)
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ProgressTracker)
 
   ProgressTracker()
     : mImageMutex("ProgressTracker::mImage")
     , mImage(nullptr)
+    , mObservers(new ObserverTable)
     , mProgress(NoProgress)
   { }
 
   bool HasImage() const { MutexAutoLock lock(mImageMutex); return mImage; }
   already_AddRefed<Image> GetImage() const
   {
     MutexAutoLock lock(mImageMutex);
     nsRefPtr<Image> image = mImage;
@@ -145,27 +179,23 @@ public:
   // be held.  Called on the main thread only.
   void SyncNotifyProgress(Progress aProgress,
                           const nsIntRect& aInvalidRect = nsIntRect());
 
   // We manage a set of observers that are using an image and thus concerned
   // with its loading progress. Weak pointers.
   void AddObserver(IProgressObserver* aObserver);
   bool RemoveObserver(IProgressObserver* aObserver);
-  size_t ObserverCount() const {
-    MOZ_ASSERT(NS_IsMainThread(), "Use mObservers on main thread only");
-    return mObservers.Length();
-  }
+  uint32_t ObserverCount() const;
 
   // Resets our weak reference to our image. Image subclasses should call this
   // in their destructor.
   void ResetImage();
 
 private:
-  typedef nsTObserverArray<mozilla::WeakPtr<IProgressObserver>> ObserverArray;
   friend class AsyncNotifyRunnable;
   friend class AsyncNotifyCurrentStateRunnable;
   friend class ImageFactory;
 
   ProgressTracker(const ProgressTracker& aOther) = delete;
 
   // Sets our weak reference to our image. Only ImageFactory should call this.
   void SetImage(Image* aImage);
@@ -173,33 +203,27 @@ private:
   // Send some notifications that would be necessary to make |aObserver| believe
   // the request is finished downloading and decoding.  We only send
   // FLAG_LOAD_COMPLETE and FLAG_ONLOAD_UNBLOCKED, and only if necessary.
   void EmulateRequestFinished(IProgressObserver* aObserver);
 
   // Main thread only because it deals with the observer service.
   void FireFailureNotification();
 
-  // Main thread only, since notifications are expected on the main thread, and
-  // mObservers is not threadsafe.
-  static void SyncNotifyInternal(ObserverArray& aObservers,
-                                 bool aHasImage, Progress aProgress,
-                                 const nsIntRect& aInvalidRect);
-
+  // The runnable, if any, that we've scheduled to deliver async notifications.
   nsCOMPtr<nsIRunnable> mRunnable;
 
   // mImage is a weak ref; it should be set to null when the image goes out of
   // scope. mImageMutex protects mImage.
   mutable Mutex mImageMutex;
   Image* mImage;
 
-  // List of observers attached to the image. Each observer represents a
-  // consumer using the image. Array and/or individual elements should only be
-  // accessed on the main thread.
-  ObserverArray mObservers;
+  // Hashtable of observers attached to the image. Each observer represents a
+  // consumer using the image. Main thread only.
+  CopyOnWrite<ObserverTable> mObservers;
 
   Progress mProgress;
 };
 
 } // namespace image
 } // namespace mozilla
 
 #endif // mozilla_image_ProgressTracker_h