From e4de973baa8edf32cf672cd5414d9154c393b8df Mon Sep 17 00:00:00 2001 From: Seth Fowler Date: Tue, 25 Aug 2015 16:26:43 -0700 Subject: [PATCH] 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 | 158 ++++++++++++++++++++++++++++---------- image/ProgressTracker.h | 54 +++++++++---- 2 files changed, 155 insertions(+), 57 deletions(-) diff --git a/image/ProgressTracker.cpp b/image/ProgressTracker.cpp index 9d12762bf77e..b0a76a762ece 100644 --- a/image/ProgressTracker.cpp +++ b/image/ProgressTracker.cpp @@ -243,33 +243,76 @@ ProgressTracker::NotifyCurrentState(IProgressObserver* aObserver) NS_DispatchToCurrentThread(ev); } -#define NOTIFY_IMAGE_OBSERVERS(OBSERVERS, FUNC) \ - do { \ - ObserverArray::ForwardIterator iter(OBSERVERS); \ - while (iter.HasMore()) { \ - nsRefPtr 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 struct ImageObserverNotifier; -/* static */ void -ProgressTracker::SyncNotifyInternal(ObserverArray& aObservers, - bool aHasImage, - Progress aProgress, - const nsIntRect& aDirtyRect) +template <> +struct MOZ_STACK_CLASS ImageObserverNotifier +{ + explicit ImageObserverNotifier(const ObserverTable* aObservers, + bool aIgnoreDeferral = false) + : mObservers(aObservers) + , mIgnoreDeferral(aIgnoreDeferral) + { } + + template + void operator()(Lambda aFunc) + { + for (auto iter = mObservers->ConstIter(); !iter.Done(); iter.Next()) { + nsRefPtr observer = iter.Data().get(); + if (observer && + (mIgnoreDeferral || !observer->NotificationsDeferred())) { + aFunc(observer); + } + } + } + +private: + const ObserverTable* mObservers; + const bool mIgnoreDeferral; +}; + +template <> +struct MOZ_STACK_CLASS ImageObserverNotifier +{ + explicit ImageObserverNotifier(IProgressObserver* aObserver) + : mObserver(aObserver) + { } + + template + void operator()(Lambda aFunc) + { + if (mObserver && !mObserver->NotificationsDeferred()) { + aFunc(mObserver); + } + } + +private: + IProgressObserver* mObserver; +}; + +template void +SyncNotifyInternal(const T& aObservers, + bool aHasImage, + Progress aProgress, + const nsIntRect& aDirtyRect) { MOZ_ASSERT(NS_IsMainThread()); typedef imgINotificationObserver I; + ImageObserverNotifier 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) { @@ -278,19 +321,21 @@ ProgressTracker::SyncNotifyInternal(ObserverArray& aObservers, // 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); }); } } @@ -298,17 +343,18 @@ ProgressTracker::SyncNotifyInternal(ObserverArray& aObservers, // 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); + }); } } @@ -340,7 +386,9 @@ ProgressTracker::SyncNotifyProgress(Progress aProgress, 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(); @@ -370,9 +418,7 @@ ProgressTracker::SyncNotify(IProgressObserver* aObserver) } } - ObserverArray array; - array.AppendElement(aObserver); - SyncNotifyInternal(array, !!image, mProgress, rect); + SyncNotifyInternal(aObserver, !!image, mProgress, rect); } void @@ -395,7 +441,14 @@ 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 weakPtr = aObserver; + aTable->Put(aObserver, weakPtr); + }); } bool @@ -404,7 +457,11 @@ 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. @@ -425,12 +482,25 @@ ProgressTracker::RemoveObserver(IProgressObserver* aObserver) 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 notify(aTable); + notify([](IProgressObserver* aObs) { + aObs->Notify(imgINotificationObserver::UNLOCKED_DRAW); + }); + }); } void @@ -445,8 +515,12 @@ void ProgressTracker::OnDiscard() { MOZ_ASSERT(NS_IsMainThread()); - NOTIFY_IMAGE_OBSERVERS(mObservers, - Notify(imgINotificationObserver::DISCARD)); + mObservers.Read([](const ObserverTable* aTable) { + ImageObserverNotifier notify(aTable); + notify([](IProgressObserver* aObs) { + aObs->Notify(imgINotificationObserver::DISCARD); + }); + }); } void @@ -454,13 +528,13 @@ 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 observer = iter.GetNext().get(); - if (observer) { - observer->SetHasImage(); - } - } + mObservers.Read([](const ObserverTable* aTable) { + ImageObserverNotifier + notify(aTable, /* aIgnoreDeferral = */ true); + notify([](IProgressObserver* aObs) { + aObs->SetHasImage(); + }); + }); } void diff --git a/image/ProgressTracker.h b/image/ProgressTracker.h index b511afd21646..1cb0c116d848 100644 --- a/image/ProgressTracker.h +++ b/image/ProgressTracker.h @@ -7,9 +7,11 @@ #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" @@ -57,6 +59,37 @@ inline Progress LoadCompleteProgress(bool aLastPart, 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. + */ +class ObserverTable + : public nsDataHashtable, + WeakPtr> +{ +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 @@ -78,6 +111,7 @@ public: ProgressTracker() : mImageMutex("ProgressTracker::mImage") , mImage(nullptr) + , mObservers(new ObserverTable) , mProgress(NoProgress) { } @@ -150,17 +184,13 @@ public: // 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> ObserverArray; friend class AsyncNotifyRunnable; friend class AsyncNotifyCurrentStateRunnable; friend class ImageFactory; @@ -178,12 +208,7 @@ private: // 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 mRunnable; // mImage is a weak ref; it should be set to null when the image goes out of @@ -191,10 +216,9 @@ private: 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 mObservers; Progress mProgress; };