Bug 1176124 (Part 2) - Add placeholder support to the SurfaceCache so we can avoid launching redundant decoders. r=dholbert
authorSeth Fowler <mark.seth.fowler@gmail.com>
Sun, 19 Jul 2015 18:39:44 -0700
changeset 253635 f52a8f3b15ed32a3c9084133b0fe44374effeaf7
parent 253634 6747b7f35dcb1e2ce70251e31080a3abba2115bd
child 253636 5834e85fc660b4fb6e466fe13fcead5b8d733545
push id29072
push usercbook@mozilla.com
push dateMon, 20 Jul 2015 09:36:58 +0000
treeherderautoland@5df788c56ae7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1176124
milestone42.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 1176124 (Part 2) - Add placeholder support to the SurfaceCache so we can avoid launching redundant decoders. r=dholbert
image/RasterImage.cpp
image/SurfaceCache.cpp
image/SurfaceCache.h
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -521,19 +521,22 @@ RasterImage::LookupFrame(uint32_t aFrame
 
   LookupResult result = LookupFrameInternal(aFrameNum, requestedSize, aFlags);
 
   if (!result && !mHasSize) {
     // We can't request a decode without knowing our intrinsic size. Give up.
     return DrawableFrameRef();
   }
 
-  if (!result || result.Type() == MatchType::SUBSTITUTE_BECAUSE_NOT_FOUND) {
+  if (result.Type() == MatchType::NOT_FOUND ||
+      result.Type() == MatchType::SUBSTITUTE_BECAUSE_NOT_FOUND ||
+      ((aFlags & FLAG_SYNC_DECODE) && !result)) {
     // We don't have a copy of this frame, and there's no decoder working on
-    // one. Trigger decoding so it'll be available next time.
+    // one. (Or we're sync decoding and the existing decoder hasn't even started
+    // yet.) Trigger decoding so it'll be available next time.
     MOZ_ASSERT(!mAnim, "Animated frames should be locked");
 
     Decode(Some(requestedSize), aFlags);
 
     // If we can sync decode, we should already have the frame.
     if (aFlags & FLAG_SYNC_DECODE) {
       result = LookupFrameInternal(aFrameNum, requestedSize, aFlags);
     }
@@ -1495,16 +1498,29 @@ RasterImage::CreateDecoder(const Maybe<I
   }
 
   decoder->Init();
 
   if (NS_FAILED(decoder->GetDecoderError())) {
     return nullptr;
   }
 
+  if (aSize) {
+    // Add a placeholder for the first frame to the SurfaceCache so we won't
+    // trigger any more decoders with the same parameters.
+    InsertOutcome outcome =
+      SurfaceCache::InsertPlaceholder(ImageKey(this),
+                                      RasterSurfaceKey(*aSize,
+                                                       decoder->GetDecodeFlags(),
+                                                       /* aFrameNum = */ 0));
+    if (outcome != InsertOutcome::SUCCESS) {
+      return nullptr;
+    }
+  }
+
   if (!aSize) {
     Telemetry::GetHistogramById(
       Telemetry::IMAGE_DECODE_COUNT)->Subtract(mDecodeCount);
     mDecodeCount++;
     Telemetry::GetHistogramById(
       Telemetry::IMAGE_DECODE_COUNT)->Add(mDecodeCount);
 
     if (mDecodeCount > sMaxDecodeCount) {
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -11,16 +11,17 @@
 
 #include <algorithm>
 #include "mozilla/Assertions.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Likely.h"
 #include "mozilla/Move.h"
 #include "mozilla/Mutex.h"
+#include "mozilla/Pair.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/StaticPtr.h"
 #include "nsIMemoryReporter.h"
 #include "gfx2DGlue.h"
 #include "gfxPattern.h"  // Workaround for flaw in bug 921753 part 2.
 #include "gfxPlatform.h"
 #include "gfxPrefs.h"
 #include "imgFrame.h"
@@ -62,16 +63,20 @@ static StaticRefPtr<SurfaceCacheImpl> sI
 /**
  * Cost models the cost of storing a surface in the cache. Right now, this is
  * simply an estimate of the size of the surface in bytes, but in the future it
  * may be worth taking into account the cost of rematerializing the surface as
  * well.
  */
 typedef size_t Cost;
 
+// Placeholders do not have surfaces, but need to be given a trivial cost for
+// our invariants to hold.
+static const Cost sPlaceholderCost = 1;
+
 static Cost
 ComputeCost(const IntSize& aSize, uint32_t aBytesPerPixel)
 {
   MOZ_ASSERT(aBytesPerPixel == 1 || aBytesPerPixel == 4);
   return aSize.width * aSize.height * aBytesPerPixel;
 }
 
 /**
@@ -132,45 +137,61 @@ public:
                 const SurfaceKey&  aSurfaceKey,
                 const Lifetime     aLifetime)
     : mSurface(aSurface)
     , mCost(aCost)
     , mImageKey(aImageKey)
     , mSurfaceKey(aSurfaceKey)
     , mLifetime(aLifetime)
   {
-    MOZ_ASSERT(mSurface, "Must have a valid surface");
+    MOZ_ASSERT(!IsPlaceholder() ||
+               (mCost == sPlaceholderCost && mLifetime == Lifetime::Transient),
+               "Placeholder should have trivial cost and transient lifetime");
     MOZ_ASSERT(mImageKey, "Must have a valid image key");
   }
 
   DrawableFrameRef DrawableRef() const
   {
+    if (MOZ_UNLIKELY(IsPlaceholder())) {
+      MOZ_ASSERT_UNREACHABLE("Shouldn't call DrawableRef() on a placeholder");
+      return DrawableFrameRef();
+    }
+
     return mSurface->DrawableRef();
   }
 
   void SetLocked(bool aLocked)
   {
+    if (IsPlaceholder()) {
+      return;  // Can't lock a placeholder.
+    }
+
     if (aLocked && mLifetime == Lifetime::Persistent) {
       // This may fail, and that's OK. We make no guarantees about whether
       // locking is successful if you call SurfaceCache::LockImage() after
       // SurfaceCache::Insert().
       mDrawableRef = mSurface->DrawableRef();
     } else {
       mDrawableRef.reset();
     }
   }
 
+  bool IsPlaceholder() const { return !bool(mSurface); }
   bool IsLocked() const { return bool(mDrawableRef); }
 
   ImageKey GetImageKey() const { return mImageKey; }
   SurfaceKey GetSurfaceKey() const { return mSurfaceKey; }
   CostEntry GetCostEntry() { return image::CostEntry(this, mCost); }
   nsExpirationState* GetExpirationState() { return &mExpirationState; }
   Lifetime GetLifetime() const { return mLifetime; }
-  bool IsDecoded() const { return mSurface->IsImageComplete(); }
+
+  bool IsDecoded() const
+  {
+    return !IsPlaceholder() && mSurface->IsImageComplete();
+  }
 
   // A helper type used by SurfaceCacheImpl::CollectSizeOfSurfaces.
   struct MOZ_STACK_CLASS SurfaceMemoryReport
   {
     SurfaceMemoryReport(nsTArray<SurfaceMemoryCounter>& aCounters,
                         MallocSizeOf                    aMallocSizeOf)
       : mCounters(aCounters)
       , mMallocSizeOf(aMallocSizeOf)
@@ -257,31 +278,57 @@ public:
 
   already_AddRefed<CachedSurface> Lookup(const SurfaceKey& aSurfaceKey)
   {
     nsRefPtr<CachedSurface> surface;
     mSurfaces.Get(aSurfaceKey, getter_AddRefs(surface));
     return surface.forget();
   }
 
-  already_AddRefed<CachedSurface>
+  MOZ_WARN_UNUSED_RESULT  // See bug 1185044.
+  Pair<already_AddRefed<CachedSurface>, MatchType>
   LookupBestMatch(const SurfaceKey&      aSurfaceKey,
                   const Maybe<uint32_t>& aAlternateFlags)
   {
-    // Try for a perfect match first.
-    nsRefPtr<CachedSurface> surface;
-    mSurfaces.Get(aSurfaceKey, getter_AddRefs(surface));
-    if (surface && surface->IsDecoded()) {
-      return surface.forget();
+    // Try for an exact match first.
+    nsRefPtr<CachedSurface> exactMatch;
+    mSurfaces.Get(aSurfaceKey, getter_AddRefs(exactMatch));
+    if (exactMatch && exactMatch->IsDecoded()) {
+      return MakePair(exactMatch.forget(), MatchType::EXACT);
     }
 
     // There's no perfect match, so find the best match we can.
     MatchContext matchContext(aSurfaceKey, aAlternateFlags);
     ForEach(TryToImproveMatch, &matchContext);
-    return matchContext.mBestMatch.forget();
+
+    MatchType matchType;
+    if (matchContext.mBestMatch) {
+      if (!exactMatch) {
+        // No exact match, but we found a substitute.
+        matchType = MatchType::SUBSTITUTE_BECAUSE_NOT_FOUND;
+      } else if (exactMatch != matchContext.mBestMatch) {
+        // The exact match is still decoding, but we found a substitute.
+        matchType = MatchType::SUBSTITUTE_BECAUSE_PENDING;
+      } else {
+        // The exact match is still decoding, but it's the best we've got.
+        matchType = MatchType::EXACT;
+      }
+    } else {
+      if (exactMatch) {
+        // We found an "exact match", but since TryToImproveMatch didn't return
+        // it, it must have been a placeholder.
+        MOZ_ASSERT(exactMatch->IsPlaceholder());
+        matchType = MatchType::PENDING;
+      } else {
+        // We couldn't find an exact match *or* a substitute.
+        matchType = MatchType::NOT_FOUND;
+      }
+    }
+
+    return MakePair(matchContext.mBestMatch.forget(), matchType);
   }
 
   void ForEach(SurfaceTable::EnumReadFunction aFunction, void* aData)
   {
     mSurfaces.EnumerateRead(aFunction, aData);
   }
 
   void SetLocked(bool aLocked) { mLocked = aLocked; }
@@ -303,16 +350,21 @@ private:
 
   static PLDHashOperator TryToImproveMatch(const SurfaceKey& aSurfaceKey,
                                            CachedSurface*    aSurface,
                                            void*             aContext)
   {
     auto context = static_cast<MatchContext*>(aContext);
     const SurfaceKey& idealKey = context->mIdealKey;
 
+    // We never match a placeholder.
+    if (aSurface->IsPlaceholder()) {
+      return PL_DHASH_NEXT;
+    }
+
     // Matching the animation time and SVG context is required.
     if (aSurfaceKey.AnimationTime() != idealKey.AnimationTime() ||
         aSurfaceKey.SVGContext() != idealKey.SVGContext()) {
       return PL_DHASH_NEXT;
     }
 
     // Matching the flags is required, but we can match the alternate flags as
     // well if some were provided.
@@ -420,20 +472,31 @@ public:
 
   InsertOutcome Insert(imgFrame*         aSurface,
                        const Cost        aCost,
                        const ImageKey    aImageKey,
                        const SurfaceKey& aSurfaceKey,
                        Lifetime          aLifetime)
   {
     // If this is a duplicate surface, refuse to replace the original.
-    if (MOZ_UNLIKELY(Lookup(aImageKey, aSurfaceKey, /* aMarkUsed = */ false))) {
+    // XXX(seth): Calling Lookup() and then RemoveSurface() does the lookup
+    // twice. We'll make this more efficient in bug 1185137.
+    LookupResult result = Lookup(aImageKey, aSurfaceKey, /* aMarkUsed = */ false);
+    if (MOZ_UNLIKELY(result)) {
       return InsertOutcome::FAILURE_ALREADY_PRESENT;
     }
 
+    if (result.Type() == MatchType::PENDING) {
+      RemoveSurface(aImageKey, aSurfaceKey);
+    }
+
+    MOZ_ASSERT(result.Type() == MatchType::NOT_FOUND ||
+               result.Type() == MatchType::PENDING,
+               "A LookupResult with no surface should be NOT_FOUND or PENDING");
+
     // If this is bigger than we can hold after discarding everything we can,
     // refuse to cache it.
     if (MOZ_UNLIKELY(!CanHoldAfterDiscarding(aCost))) {
       mOverflowCount++;
       return InsertOutcome::FAILURE;
     }
 
     // Remove elements in order of cost until we can fit this in the cache. Note
@@ -453,16 +516,17 @@ 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) {
+      MOZ_ASSERT(!surface->IsPlaceholder(), "Placeholders should be transient");
       surface->SetLocked(true);
       if (!surface->IsLocked()) {
         return InsertOutcome::FAILURE;
       }
     }
 
     // Insert.
     MOZ_ASSERT(aCost <= mAvailableCost, "Inserting despite too large a cost");
@@ -555,16 +619,20 @@ public:
     }
 
     nsRefPtr<CachedSurface> surface = cache->Lookup(aSurfaceKey);
     if (!surface) {
       // Lookup in the per-image cache missed.
       return LookupResult(MatchType::NOT_FOUND);
     }
 
+    if (surface->IsPlaceholder()) {
+      return LookupResult(MatchType::PENDING);
+    }
+
     DrawableFrameRef ref = surface->DrawableRef();
     if (!ref) {
       // The surface was released by the operating system. Remove the cache
       // entry as well.
       Remove(surface);
       return LookupResult(MatchType::NOT_FOUND);
     }
 
@@ -590,48 +658,49 @@ public:
     // Repeatedly look up the best match, trying again if the resulting surface
     // has been freed by the operating system, until we can either lock a
     // surface for drawing or there are no matching surfaces left.
     // XXX(seth): This is O(N^2), but N is expected to be very small. If we
     // encounter a performance problem here we can revisit this.
 
     nsRefPtr<CachedSurface> surface;
     DrawableFrameRef ref;
+    MatchType matchType = MatchType::NOT_FOUND;
     while (true) {
-      surface = cache->LookupBestMatch(aSurfaceKey, aAlternateFlags);
+      // XXX(seth): This code is begging for std::tie. See bug 1184385.
+      Pair<already_AddRefed<CachedSurface>, MatchType> lookupResult =
+        cache->LookupBestMatch(aSurfaceKey, aAlternateFlags);
+      surface = lookupResult.first();
+      matchType = lookupResult.second();
+
       if (!surface) {
-        // Lookup in the per-image cache missed.
-        return LookupResult(MatchType::NOT_FOUND);
+        return LookupResult(matchType);  // Lookup in the per-image cache missed.
       }
 
       ref = surface->DrawableRef();
       if (ref) {
         break;
       }
 
       // The surface was released by the operating system. Remove the cache
       // entry as well.
       Remove(surface);
     }
 
-    SurfaceKey key = surface->GetSurfaceKey();
-    const bool isExactMatch = key.Size() == aSurfaceKey.Size();
-
-    MOZ_ASSERT(isExactMatch ==
-      (key == aSurfaceKey ||
-         (aAlternateFlags && key == aSurfaceKey.WithNewFlags(*aAlternateFlags))),
+    MOZ_ASSERT((matchType == MatchType::EXACT) ==
+      (surface->GetSurfaceKey() == aSurfaceKey ||
+         (aAlternateFlags &&
+          surface->GetSurfaceKey() ==
+            aSurfaceKey.WithNewFlags(*aAlternateFlags))),
       "Result differs in a way other than size or alternate flags");
 
-
-    if (isExactMatch) {
+    if (matchType == MatchType::EXACT) {
       MarkUsed(surface, cache);
     }
 
-    MatchType matchType = isExactMatch ? MatchType::EXACT
-                                       : MatchType::SUBSTITUTE_BECAUSE_NOT_FOUND;
     return LookupResult(Move(ref), matchType);
   }
 
   void RemoveSurface(const ImageKey    aImageKey,
                      const SurfaceKey& aSurfaceKey)
   {
     nsRefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache) {
@@ -1043,16 +1112,29 @@ SurfaceCache::Insert(imgFrame*         a
     MOZ_CRASH("Don't pass null surfaces to SurfaceCache::Insert");
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
   Cost cost = ComputeCost(aSurface->GetSize(), aSurface->GetBytesPerPixel());
   return sInstance->Insert(aSurface, cost, aImageKey, aSurfaceKey, aLifetime);
 }
 
+/* static */ InsertOutcome
+SurfaceCache::InsertPlaceholder(const ImageKey    aImageKey,
+                                const SurfaceKey& aSurfaceKey)
+{
+  if (!sInstance) {
+    return InsertOutcome::FAILURE;
+  }
+
+  MutexAutoLock lock(sInstance->GetMutex());
+  return sInstance->Insert(nullptr, sPlaceholderCost, aImageKey, aSurfaceKey,
+                           Lifetime::Transient);
+}
+
 /* static */ bool
 SurfaceCache::CanHold(const IntSize& aSize, uint32_t aBytesPerPixel /* = 4 */)
 {
   if (!sInstance) {
     return false;
   }
 
   Cost cost = ComputeCost(aSize, aBytesPerPixel);
--- a/image/SurfaceCache.h
+++ b/image/SurfaceCache.h
@@ -225,16 +225,17 @@ struct SurfaceCache
   static LookupResult LookupBestMatch(const ImageKey    aImageKey,
                                       const SurfaceKey& aSurfaceKey,
                                       const Maybe<uint32_t>& aAlternateFlags
                                         = Nothing());
 
   /**
    * 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.
+   * If a matching placeholder is already present, the placeholder is removed.
    *
    * Each surface in the cache has a lifetime, either Transient or Persistent.
    * Transient surfaces can expire from the cache at any time. Persistent
    * surfaces, on the other hand, will never expire as long as they remain
    * locked, but if they become unlocked, can expire just like transient
    * surfaces. When it is first inserted, a persistent surface is locked if its
    * associated image is locked. When that image is later unlocked, the surface
    * becomes unlocked too. To become locked again at that point, two things must
@@ -276,16 +277,43 @@ struct SurfaceCache
    *           SurfaceKey already exists in the cache.
    */
   static InsertOutcome Insert(imgFrame*         aSurface,
                               const ImageKey    aImageKey,
                               const SurfaceKey& aSurfaceKey,
                               Lifetime          aLifetime);
 
   /**
+   * Insert a placeholder for a surface into the cache. If a surface with the
+   * same ImageKey and SurfaceKey is already in the cache, InsertPlaceholder()
+   * returns FAILURE_ALREADY_PRESENT.
+   *
+   * Placeholders exist to allow lazy allocation of surfaces. The Lookup*()
+   * methods will report whether a placeholder for an exactly matching surface
+   * existed by returning a MatchType of PENDING or SUBSTITUTE_BECAUSE_PENDING,
+   * but they will never return a placeholder directly. (They couldn't, since
+   * placeholders don't have an associated surface.)
+   *
+   * Once inserted, placeholders can be removed using RemoveSurface() or
+   * RemoveImage(), just like a surface.  They're automatically removed when a
+   * real surface that matches the placeholder is inserted with Insert().
+   *
+   * @param aImageKey    Key data identifying which image the placeholder
+   *                     belongs to.
+   * @param aSurfaceKey  Key data which uniquely identifies the surface the
+   *                     placeholder stands in for.
+   * @return SUCCESS if the placeholder was inserted successfully.
+   *         FAILURE if the placeholder could not be inserted for some reason.
+   *         FAILURE_ALREADY_PRESENT if a surface with the same ImageKey and
+   *           SurfaceKey already exists in the cache.
+   */
+  static InsertOutcome InsertPlaceholder(const ImageKey    aImageKey,
+                                         const SurfaceKey& aSurfaceKey);
+
+  /**
    * 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.
    *
    * Use CanHold() to avoid the need to create a temporary surface when we know
    * for sure the cache can't hold it.
@@ -357,34 +385,35 @@ struct SurfaceCache
    * If the image is unlocked, this has no effect.
    *
    * @param aImageKey    The image which should have its existing surfaces
    *                     unlocked.
    */
   static void UnlockSurfaces(const ImageKey aImageKey);
 
   /**
-   * Removes a surface from the cache, if it's present. If it's not present,
-   * RemoveSurface() has no effect.
+   * Removes a surface or placeholder from the cache, if it's present. If it's
+   * not present, RemoveSurface() has no effect.
    *
    * Use this function to remove individual surfaces that have become invalid.
    * Prefer RemoveImage() or DiscardAll() when they're applicable, as they have
    * much better performance than calling this function repeatedly.
    *
    * @param aImageKey    Key data identifying which image the surface belongs
                          to.
    * @param aSurfaceKey  Key data which uniquely identifies the requested
                          surface.
    */
   static void RemoveSurface(const ImageKey    aImageKey,
                             const SurfaceKey& aSurfaceKey);
 
   /**
-   * Removes all cached surfaces associated with the given image from the cache.
-   * If the image is locked, it is automatically unlocked.
+   * Removes all cached surfaces and placeholders associated with the given
+   * image from the cache.  If the image is locked, it is automatically
+   * unlocked.
    *
    * This MUST be called, at a minimum, when an Image which could be storing
    * surfaces in the surface cache is destroyed. If another image were allocated
    * at the same address it could result in subtle, difficult-to-reproduce bugs.
    *
    * @param aImageKey  The image which should be removed from the cache.
    */
   static void RemoveImage(const ImageKey aImageKey);