Bug 1389479 - Part 2. Make the SurfaceCache free ImageSurfaceCache objects outside of the lock. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 15 Aug 2017 15:02:14 -0400
changeset 374900 3f2e443dd48ce187ba6098e2a72fd5804f94c6f3
parent 374899 635b60924015ec2379e3e4f092f2cd54b587fd17
child 374901 9ddc28603bea7b9916093490359863be852a7358
push id32340
push userkwierso@gmail.com
push dateWed, 16 Aug 2017 02:03:08 +0000
treeherdermozilla-central@4e93516e92e5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1389479
milestone57.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 1389479 - Part 2. Make the SurfaceCache free ImageSurfaceCache objects outside of the lock. r=tnikkel
image/SurfaceCache.cpp
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -248,22 +248,24 @@ public:
 
   MOZ_MUST_USE bool Insert(NotNull<CachedSurface*> aSurface)
   {
     MOZ_ASSERT(!mLocked || aSurface->IsPlaceholder() || aSurface->IsLocked(),
                "Inserting an unlocked surface for a locked image");
     return mSurfaces.Put(aSurface->GetSurfaceKey(), aSurface, fallible);
   }
 
-  void Remove(NotNull<CachedSurface*> aSurface)
+  already_AddRefed<CachedSurface> Remove(NotNull<CachedSurface*> aSurface)
   {
     MOZ_ASSERT(mSurfaces.GetWeak(aSurface->GetSurfaceKey()),
         "Should not be removing a surface we don't have");
 
-    mSurfaces.Remove(aSurface->GetSurfaceKey());
+    RefPtr<CachedSurface> surface;
+    mSurfaces.Remove(aSurface->GetSurfaceKey(), getter_AddRefs(surface));
+    return surface.forget();
   }
 
   already_AddRefed<CachedSurface> Lookup(const SurfaceKey& aSurfaceKey)
   {
     RefPtr<CachedSurface> surface;
     mSurfaces.Get(aSurfaceKey, getter_AddRefs(surface));
     return surface.forget();
   }
@@ -510,20 +512,24 @@ public:
     MOZ_ASSERT(cache, "Shouldn't try to remove a surface with no image cache");
 
     // If the surface was not a placeholder, tell its image that we discarded it.
     if (!aSurface->IsPlaceholder()) {
       static_cast<Image*>(imageKey)->OnSurfaceDiscarded(aSurface->GetSurfaceKey());
     }
 
     StopTracking(aSurface, aAutoLock);
-    cache->Remove(aSurface);
+
+    // Individual surfaces must be freed outside the lock.
+    mCachedSurfacesDiscard.AppendElement(cache->Remove(aSurface));
 
-    // Remove the per-image cache if it's unneeded now. (Keep it if the image is
-    // locked, since the per-image cache is where we store that state.)
+    // Remove the per-image cache if it's unneeded now. Keep it if the image is
+    // locked, since the per-image cache is where we store that state. Note that
+    // we don't push it into mImageCachesDiscard because all of its surfaces
+    // have been removed, so it is safe to free while holding the lock.
     if (cache->IsEmpty() && !cache->IsLocked()) {
       mImageCaches.Remove(imageKey);
     }
   }
 
   void StartTracking(NotNull<CachedSurface*> aSurface,
                      const StaticMutexAutoLock& aAutoLock)
   {
@@ -723,35 +729,40 @@ public:
     }
 
     // (Note that we *don't* unlock the per-image cache here; that's the
     // difference between this and UnlockImage.)
     DoUnlockSurfaces(WrapNotNull(cache),
       /* aStaticOnly = */ !gfxPrefs::ImageMemAnimatedDiscardable(), aAutoLock);
   }
 
-  void RemoveImage(const ImageKey aImageKey, const StaticMutexAutoLock& aAutoLock)
+  already_AddRefed<ImageSurfaceCache>
+  RemoveImage(const ImageKey aImageKey, const StaticMutexAutoLock& aAutoLock)
   {
     RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache) {
-      return;  // No cached surfaces for this image, so nothing to do.
+      return nullptr;  // No cached surfaces for this image, so nothing to do.
     }
 
     // Discard all of the cached surfaces for this image.
     // XXX(seth): This is O(n^2) since for each item in the cache we are
     // removing an element from the costs array. Since n is expected to be
     // small, performance should be good, but if usage patterns change we should
     // change the data structure used for mCosts.
     for (auto iter = cache->ConstIter(); !iter.Done(); iter.Next()) {
       StopTracking(WrapNotNull(iter.UserData()), aAutoLock);
     }
 
     // The per-image cache isn't needed anymore, so remove it as well.
     // This implicitly unlocks the image if it was locked.
     mImageCaches.Remove(aImageKey);
+
+    // Since we did not actually remove any of the surfaces from the cache
+    // itself, only stopped tracking them, we should free it outside the lock.
+    return cache.forget();
   }
 
   void DiscardAll(const StaticMutexAutoLock& aAutoLock)
   {
     // Remove in order of cost because mCosts is an array and the other data
     // structures are all hash tables. Note that locked surfaces are not
     // removed, since they aren't present in mCosts.
     while (!mCosts.IsEmpty()) {
@@ -780,16 +791,23 @@ public:
 
     // Discard surfaces until we've reduced our cost to our target cost.
     while (mAvailableCost < targetCost) {
       MOZ_ASSERT(!mCosts.IsEmpty(), "Removed everything and still not done");
       Remove(mCosts.LastElement().Surface(), aAutoLock);
     }
   }
 
+  void TakeDiscard(nsTArray<RefPtr<CachedSurface>>& aDiscard,
+                   const StaticMutexAutoLock& aAutoLock)
+  {
+    MOZ_ASSERT(aDiscard.IsEmpty());
+    aDiscard = Move(mCachedSurfacesDiscard);
+  }
+
   void LockSurface(NotNull<CachedSurface*> aSurface,
                    const StaticMutexAutoLock& aAutoLock)
   {
     if (aSurface->IsPlaceholder() || aSurface->IsLocked()) {
       return;
     }
 
     StopTracking(aSurface, aAutoLock);
@@ -903,62 +921,82 @@ private:
     RefPtr<CachedSurface> surface = cache->Lookup(aSurfaceKey);
     if (!surface) {
       return;  // Lookup in the per-image cache missed.
     }
 
     Remove(WrapNotNull(surface), aAutoLock);
   }
 
-  struct SurfaceTracker : public ExpirationTrackerImpl<CachedSurface, 2,
-                                                       StaticMutex,
-                                                       StaticMutexAutoLock>
+  class SurfaceTracker final :
+    public ExpirationTrackerImpl<CachedSurface, 2,
+                                 StaticMutex,
+                                 StaticMutexAutoLock>
   {
+  public:
     explicit SurfaceTracker(uint32_t aSurfaceCacheExpirationTimeMS)
       : ExpirationTrackerImpl<CachedSurface, 2,
                               StaticMutex, StaticMutexAutoLock>(
           aSurfaceCacheExpirationTimeMS, "SurfaceTracker",
           SystemGroup::EventTargetFor(TaskCategory::Other))
     { }
 
   protected:
     void NotifyExpiredLocked(CachedSurface* aSurface,
                              const StaticMutexAutoLock& aAutoLock) override
     {
       sInstance->Remove(WrapNotNull(aSurface), aAutoLock);
     }
 
+    void NotifyHandlerEndLocked(const StaticMutexAutoLock& aAutoLock) override
+    {
+      sInstance->TakeDiscard(mDiscard, aAutoLock);
+    }
+
+    void NotifyHandlerEnd() override
+    {
+      nsTArray<RefPtr<CachedSurface>> discard(Move(mDiscard));
+    }
+
     StaticMutex& GetMutex() override
     {
       return sInstanceMutex;
     }
+
+    nsTArray<RefPtr<CachedSurface>> mDiscard;
   };
 
-  struct MemoryPressureObserver : public nsIObserver
+  class MemoryPressureObserver final : public nsIObserver
   {
+  public:
     NS_DECL_ISUPPORTS
 
     NS_IMETHOD Observe(nsISupports*,
                        const char* aTopic,
                        const char16_t*) override
     {
-      StaticMutexAutoLock lock(sInstanceMutex);
-      if (sInstance && strcmp(aTopic, "memory-pressure") == 0) {
-        sInstance->DiscardForMemoryPressure(lock);
+      nsTArray<RefPtr<CachedSurface>> discard;
+      {
+        StaticMutexAutoLock lock(sInstanceMutex);
+        if (sInstance && strcmp(aTopic, "memory-pressure") == 0) {
+          sInstance->DiscardForMemoryPressure(lock);
+          sInstance->TakeDiscard(discard, lock);
+        }
       }
       return NS_OK;
     }
 
   private:
     virtual ~MemoryPressureObserver() { }
   };
 
   nsTArray<CostEntry>                     mCosts;
   nsRefPtrHashtable<nsPtrHashKey<Image>,
     ImageSurfaceCache> mImageCaches;
+  nsTArray<RefPtr<CachedSurface>>         mCachedSurfacesDiscard;
   SurfaceTracker                          mExpirationTracker;
   RefPtr<MemoryPressureObserver>        mMemoryPressureObserver;
   const uint32_t                          mDiscardFactor;
   const Cost                              mMaxCost;
   Cost                                    mAvailableCost;
   Cost                                    mLockedCost;
   size_t                                  mOverflowCount;
 };
@@ -1024,55 +1062,82 @@ SurfaceCache::Initialize()
                                    surfaceCacheDiscardFactor,
                                    finalSurfaceCacheSizeBytes);
   sInstance->InitMemoryReporter();
 }
 
 /* static */ void
 SurfaceCache::Shutdown()
 {
-  StaticMutexAutoLock lock(sInstanceMutex);
-  MOZ_ASSERT(NS_IsMainThread());
-  MOZ_ASSERT(sInstance, "No singleton - was Shutdown() called twice?");
-  sInstance = nullptr;
+  RefPtr<SurfaceCacheImpl> cache;
+  {
+    StaticMutexAutoLock lock(sInstanceMutex);
+    MOZ_ASSERT(NS_IsMainThread());
+    MOZ_ASSERT(sInstance, "No singleton - was Shutdown() called twice?");
+    cache = sInstance.forget();
+  }
 }
 
 /* static */ LookupResult
 SurfaceCache::Lookup(const ImageKey         aImageKey,
                      const SurfaceKey&      aSurfaceKey)
 {
-  StaticMutexAutoLock lock(sInstanceMutex);
-  if (!sInstance) {
-    return LookupResult(MatchType::NOT_FOUND);
+  nsTArray<RefPtr<CachedSurface>> discard;
+  LookupResult rv(MatchType::NOT_FOUND);
+
+  {
+    StaticMutexAutoLock lock(sInstanceMutex);
+    if (!sInstance) {
+      return rv;
+    }
+
+    rv = sInstance->Lookup(aImageKey, aSurfaceKey, lock);
+    sInstance->TakeDiscard(discard, lock);
   }
 
-  return sInstance->Lookup(aImageKey, aSurfaceKey, lock);
+  return rv;
 }
 
 /* static */ LookupResult
 SurfaceCache::LookupBestMatch(const ImageKey         aImageKey,
                               const SurfaceKey&      aSurfaceKey)
 {
-  StaticMutexAutoLock lock(sInstanceMutex);
-  if (!sInstance) {
-    return LookupResult(MatchType::NOT_FOUND);
+  nsTArray<RefPtr<CachedSurface>> discard;
+  LookupResult rv(MatchType::NOT_FOUND);
+
+  {
+    StaticMutexAutoLock lock(sInstanceMutex);
+    if (!sInstance) {
+      return rv;
+    }
+
+    rv = sInstance->LookupBestMatch(aImageKey, aSurfaceKey, lock);
+    sInstance->TakeDiscard(discard, lock);
   }
 
-  return sInstance->LookupBestMatch(aImageKey, aSurfaceKey, lock);
+  return rv;
 }
 
 /* static */ InsertOutcome
 SurfaceCache::Insert(NotNull<ISurfaceProvider*> aProvider)
 {
-  StaticMutexAutoLock lock(sInstanceMutex);
-  if (!sInstance) {
-    return InsertOutcome::FAILURE;
+  nsTArray<RefPtr<CachedSurface>> discard;
+  InsertOutcome rv(InsertOutcome::FAILURE);
+
+  {
+    StaticMutexAutoLock lock(sInstanceMutex);
+    if (!sInstance) {
+      return rv;
+    }
+
+    rv = sInstance->Insert(aProvider, /* aSetAvailable = */ false, lock);
+    sInstance->TakeDiscard(discard, lock);
   }
 
-  return sInstance->Insert(aProvider, /* aSetAvailable = */ false, lock);
+  return rv;
 }
 
 /* static */ bool
 SurfaceCache::CanHold(const IntSize& aSize, uint32_t aBytesPerPixel /* = 4 */)
 {
   StaticMutexAutoLock lock(sInstanceMutex);
   if (!sInstance) {
     return false;
@@ -1129,28 +1194,35 @@ SurfaceCache::UnlockEntries(const ImageK
   if (sInstance) {
     return sInstance->UnlockEntries(aImageKey, lock);
   }
 }
 
 /* static */ void
 SurfaceCache::RemoveImage(const ImageKey aImageKey)
 {
-  StaticMutexAutoLock lock(sInstanceMutex);
-  if (sInstance) {
-    sInstance->RemoveImage(aImageKey, lock);
+  RefPtr<ImageSurfaceCache> discard;
+  {
+    StaticMutexAutoLock lock(sInstanceMutex);
+    if (sInstance) {
+      discard = sInstance->RemoveImage(aImageKey, lock);
+    }
   }
 }
 
 /* static */ void
 SurfaceCache::DiscardAll()
 {
-  StaticMutexAutoLock lock(sInstanceMutex);
-  if (sInstance) {
-    sInstance->DiscardAll(lock);
+  nsTArray<RefPtr<CachedSurface>> discard;
+  {
+    StaticMutexAutoLock lock(sInstanceMutex);
+    if (sInstance) {
+      sInstance->DiscardAll(lock);
+      sInstance->TakeDiscard(discard, lock);
+    }
   }
 }
 
 /* static */ void
 SurfaceCache::CollectSizeOfSurfaces(const ImageKey                  aImageKey,
                                     nsTArray<SurfaceMemoryCounter>& aCounters,
                                     MallocSizeOf                    aMallocSizeOf)
 {