Bug 1118105 - Make SurfaceCache::Insert let you know if you try to insert a duplicate surface. r=dholbert
☠☠ backed out by 98ca4954995f ☠ ☠
authorSeth Fowler <seth@mozilla.com>
Fri, 09 Jan 2015 05:14:30 -0800
changeset 248828 c5c63ad85aa6e3a568bf9d4eaae6e254942a3d29
parent 248827 bb2775fb362c04a626aeae82a6538c939e0f6533
child 248829 905090be811685b764845341ddb13be614595773
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1118105
milestone37.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 1118105 - Make SurfaceCache::Insert let you know if you try to insert a duplicate surface. r=dholbert
image/src/Decoder.cpp
image/src/SurfaceCache.cpp
image/src/SurfaceCache.h
--- a/image/src/Decoder.cpp
+++ b/image/src/Decoder.cpp
@@ -241,23 +241,23 @@ Decoder::AllocateFrameInternal(uint32_t 
   }
 
   RawAccessFrameRef ref = frame->RawAccessRef();
   if (!ref) {
     frame->Abort();
     return RawAccessFrameRef();
   }
 
-  bool succeeded =
+  InsertOutcome outcome =
     SurfaceCache::Insert(frame, ImageKey(&mImage),
                          RasterSurfaceKey(imageSize.ToIntSize(),
                                           aDecodeFlags,
                                           aFrameNum),
                          Lifetime::Persistent);
-  if (!succeeded) {
+  if (outcome != InsertOutcome::SUCCESS) {
     ref->Abort();
     return RawAccessFrameRef();
   }
 
   nsIntRect refreshArea;
 
   if (aFrameNum == 1) {
     MOZ_ASSERT(aPreviousFrame, "Must provide a previous frame when animated");
--- a/image/src/SurfaceCache.cpp
+++ b/image/src/SurfaceCache.cpp
@@ -301,29 +301,32 @@ private:
     UnregisterWeakMemoryReporter(this);
   }
 
 public:
   void InitMemoryReporter() { RegisterWeakMemoryReporter(this); }
 
   Mutex& GetMutex() { return mMutex; }
 
-  bool Insert(imgFrame*         aSurface,
-              const Cost        aCost,
-              const ImageKey    aImageKey,
-              const SurfaceKey& aSurfaceKey,
-              Lifetime          aLifetime)
+  InsertOutcome Insert(imgFrame*         aSurface,
+                       const Cost        aCost,
+                       const ImageKey    aImageKey,
+                       const SurfaceKey& aSurfaceKey,
+                       Lifetime          aLifetime)
   {
-    MOZ_ASSERT(!Lookup(aImageKey, aSurfaceKey),
-               "Inserting a duplicate surface into the SurfaceCache");
+    // If this is a duplicate surface, refuse to replace the original.
+    if (MOZ_UNLIKELY(Lookup(aImageKey, aSurfaceKey))) {
+      return InsertOutcome::FAILURE_ALREADY_PRESENT;
+    }
 
     // If this is bigger than we can hold after discarding everything we can,
     // refuse to cache it.
-    if (!CanHoldAfterDiscarding(aCost))
-      return false;
+    if (MOZ_UNLIKELY(!CanHoldAfterDiscarding(aCost))) {
+      return InsertOutcome::FAILURE;
+    }
 
     // Remove elements in order of cost until we can fit this in the cache. Note
     // that locked surfaces aren't in mCosts, so we never remove them here.
     while (aCost > mAvailableCost) {
       MOZ_ASSERT(!mCosts.IsEmpty(), "Removed everything and it still won't fit");
       Remove(mCosts.LastElement().GetSurface());
     }
 
@@ -338,26 +341,26 @@ public:
     nsRefPtr<CachedSurface> surface =
       new CachedSurface(aSurface, aCost, aImageKey, aSurfaceKey, aLifetime);
 
     // We require that locking succeed if the image is locked and the surface is
     // persistent; the caller may need to know this to handle errors correctly.
     if (cache->IsLocked() && aLifetime == Lifetime::Persistent) {
       surface->SetLocked(true);
       if (!surface->IsLocked()) {
-        return false;
+        return InsertOutcome::FAILURE;
       }
     }
 
     // Insert.
     MOZ_ASSERT(aCost <= mAvailableCost, "Inserting despite too large a cost");
     cache->Insert(aSurfaceKey, surface);
     StartTracking(surface);
 
-    return true;
+    return InsertOutcome::SUCCESS;
   }
 
   void Remove(CachedSurface* aSurface)
   {
     MOZ_ASSERT(aSurface, "Should have a surface");
     ImageKey imageKey = aSurface->GetImageKey();
 
     nsRefPtr<ImageSurfaceCache> cache = GetImageCache(imageKey);
@@ -793,24 +796,24 @@ SurfaceCache::Lookup(const ImageKey    a
   if (!sInstance) {
     return DrawableFrameRef();
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
   return sInstance->Lookup(aImageKey, aSurfaceKey);
 }
 
-/* static */ bool
+/* static */ InsertOutcome
 SurfaceCache::Insert(imgFrame*         aSurface,
                      const ImageKey    aImageKey,
                      const SurfaceKey& aSurfaceKey,
                      Lifetime          aLifetime)
 {
   if (!sInstance) {
-    return false;
+    return InsertOutcome::FAILURE;
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
   Cost cost = ComputeCost(aSurfaceKey.Size());
   return sInstance->Insert(aSurface, cost, aImageKey, aSurfaceKey, aLifetime);
 }
 
 /* static */ bool
--- a/image/src/SurfaceCache.h
+++ b/image/src/SurfaceCache.h
@@ -109,16 +109,22 @@ VectorSurfaceKey(const gfx::IntSize& aSi
   return SurfaceKey(aSize, aSVGContext, aAnimationTime, 0);
 }
 
 MOZ_BEGIN_ENUM_CLASS(Lifetime, uint8_t)
   Transient,
   Persistent
 MOZ_END_ENUM_CLASS(Lifetime)
 
+MOZ_BEGIN_ENUM_CLASS(InsertOutcome, uint8_t)
+  SUCCESS,                 // Success (but see Insert documentation).
+  FAILURE,                 // Couldn't insert (e.g., for capacity reasons).
+  FAILURE_ALREADY_PRESENT  // A surface with the same key is already present.
+MOZ_END_ENUM_CLASS(InsertOutcome)
+
 /**
  * SurfaceCache is an imagelib-global service that allows caching of temporary
  * surfaces. Surfaces normally expire from the cache automatically if they go
  * too long without being accessed.
  *
  * SurfaceCache does not hold surfaces directly; instead, it holds imgFrame
  * objects, which hold surfaces but also layer on additional features specific
  * to imagelib's needs like animation, padding support, and transparent support
@@ -168,53 +174,57 @@ struct SurfaceCache
    *
    * @return a DrawableFrameRef to the imgFrame wrapping the requested surface,
    *         or an empty DrawableFrameRef if not found.
    */
   static DrawableFrameRef Lookup(const ImageKey    aImageKey,
                                  const SurfaceKey& aSurfaceKey);
 
   /**
-   * Insert a surface into the cache. It is an error to call this function
-   * without first calling Lookup to verify that the surface is not already in
-   * the cache.
+   * Insert a surface into the cache. If a surface with the same ImageKey and
+   * SurfaceKey is already in the cache, Insert returns FAILURE_ALREADY_PRESENT.
    *
    * Each surface in the cache has a lifetime, either Transient or Persistent.
    * Transient surfaces can expire from the cache at any time. Persistent
    * surfaces can ordinarily also expire from the cache at any time, but if the
    * image they're associated with is locked, then these surfaces will never
    * expire. This means that surfaces which cannot be rematerialized should be
    * inserted with a persistent lifetime *after* the image is locked with
    * LockImage(); if you use the other order, the surfaces might expire before
    * LockImage() gets called.
    *
    * If a surface cannot be rematerialized, it may be important to know whether
-   * it was inserted into the cache successfully. Insert() returns false if it
+   * it was inserted into the cache successfully. Insert() returns FAILURE if it
    * failed to insert the surface, which could happen because of capacity
    * reasons, or because it was already freed by the OS. If you aren't inserting
    * a surface with persistent lifetime, or if the surface isn't associated with
-   * a locked image, the return value is useless: the surface might expire
-   * immediately after being inserted, even though Insert() returned true. Thus,
-   * most callers do not need to check the return value.
+   * a locked image, checking for SUCCESS or FAILURE is useless: the surface
+   * might expire immediately after being inserted, even though Insert()
+   * returned SUCCESS. Thus, many callers do not need to check the result of
+   * Insert() at all.
    *
    * @param aTarget      The new surface (wrapped in an imgFrame) to insert into
    *                     the cache.
    * @param aImageKey    Key data identifying which image the surface belongs to.
    * @param aSurfaceKey  Key data which uniquely identifies the requested surface.
    * @param aLifetime    Whether this is a transient surface that can always be
    *                     allowed to expire, or a persistent surface that
    *                     shouldn't expire if the image is locked.
-   * @return false if the surface could not be inserted. Only check this if
-   *         inserting a persistent surface associated with a locked image (see
-   *         above for more information).
+   * @return SUCCESS if the surface was inserted successfully. (But see above
+   *           for more information about when you should check this.)
+   *         FAILURE if the surface could not be inserted, e.g. for capacity
+   *           reasons. (But see above for more information about when you
+   *           should check this.)
+   *         FAILURE_ALREADY_PRESENT if a surface with the same ImageKey and
+   *           SurfaceKey already exists in the cache.
    */
-  static bool Insert(imgFrame*         aSurface,
-                     const ImageKey    aImageKey,
-                     const SurfaceKey& aSurfaceKey,
-                     Lifetime          aLifetime);
+  static InsertOutcome Insert(imgFrame*         aSurface,
+                              const ImageKey    aImageKey,
+                              const SurfaceKey& aSurfaceKey,
+                              Lifetime          aLifetime);
 
   /**
    * Checks if a surface of a given size could possibly be stored in the cache.
    * If CanHold() returns false, Insert() will always fail to insert the
    * surface, but the inverse is not true: Insert() may take more information
    * into account than just image size when deciding whether to cache the
    * surface, so Insert() may still fail even if CanHold() returns true.
    *