Bug 1296762 (Part 1) - Remove SurfaceCache::InsertPlaceholder(). r=dholbert
authorSeth Fowler <mark.seth.fowler@gmail.com>
Fri, 19 Aug 2016 15:04:34 -0700
changeset 354794 6c7a27f37849f3accdf56b33ea82c6efb1faf995
parent 354793 72e542f76ef6bd1d16d7a7ee45d10d9e515bffca
child 354795 ef60a93ad25aa956e3ff35b32478d31dd748556e
push id1324
push usermtabara@mozilla.com
push dateMon, 16 Jan 2017 13:07:44 +0000
treeherdermozilla-release@a01c49833940 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1296762
milestone51.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 1296762 (Part 1) - Remove SurfaceCache::InsertPlaceholder(). r=dholbert
image/SurfaceCache.cpp
image/SurfaceCache.h
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -62,22 +62,16 @@ 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.
-// XXX(seth): This is only true of old-style placeholders inserted via
-// InsertPlaceholder().
-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;
 }
 
 /**
@@ -136,18 +130,17 @@ public:
                 const Cost         aCost,
                 const ImageKey     aImageKey,
                 const SurfaceKey&  aSurfaceKey)
     : mProvider(aProvider)
     , mCost(aCost)
     , mImageKey(aImageKey)
     , mSurfaceKey(aSurfaceKey)
   {
-    MOZ_ASSERT(aProvider || mCost == sPlaceholderCost,
-               "Old-style placeholders should have trivial cost");
+    MOZ_ASSERT(aProvider);
     MOZ_ASSERT(mImageKey, "Must have a valid image key");
   }
 
   DrawableSurface GetDrawableSurface() const
   {
     if (MOZ_UNLIKELY(IsPlaceholder())) {
       MOZ_ASSERT_UNREACHABLE("Called GetDrawableSurface() on a placeholder");
       return DrawableSurface();
@@ -362,17 +355,17 @@ public:
         // 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", it must have been a placeholder.
+        // We found an "exact match"; 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;
       }
     }
 
@@ -1060,29 +1053,16 @@ SurfaceCache::Insert(NotNull<ISurfacePro
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
   Cost cost = aProvider->LogicalSizeInBytes();
   return sInstance->Insert(aProvider.get(), cost, aImageKey, aSurfaceKey,
                            /* aSetAvailable = */ false);
 }
 
-/* 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,
-                           /* aSetAvailable = */ false);
-}
-
 /* 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
@@ -235,18 +235,18 @@ struct SurfaceCache
    *                        returned surface exactly matches @aSurfaceKey.
    */
   static LookupResult LookupBestMatch(const ImageKey    aImageKey,
                                       const SurfaceKey& aSurfaceKey);
 
   /**
    * Insert an ISurfaceProvider into the cache. If an entry 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.
+   * FAILURE_ALREADY_PRESENT. If a matching placeholder is already present, it
+   * is replaced.
    *
    * Cache entries will never expire as long as they remain locked, but if they
    * become unlocked, they can expire either because the SurfaceCache runs out
    * of capacity or because they've gone too long without being used.  When it
    * is first inserted, a cache entry is locked if its associated image is
    * locked.  When that image is later unlocked, the cache entry becomes
    * unlocked too. To become locked again at that point, two things must happen:
    * the image must become locked again (via LockImage()), and the cache entry
@@ -281,63 +281,28 @@ struct SurfaceCache
    *         FAILURE_ALREADY_PRESENT if an entry with the same ImageKey and
    *           SurfaceKey already exists in the cache.
    */
   static InsertOutcome Insert(NotNull<ISurfaceProvider*> aProvider,
                               const ImageKey    aImageKey,
                               const SurfaceKey& aSurfaceKey);
 
   /**
-   * Insert a placeholder entry into the cache. If an entry 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 cache
-   * entry 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.)
-   *
-   * Placeholders are automatically removed when a real entry that matches the
-   * placeholder is inserted with Insert(), or when RemoveImage() is called.
-   *
-   * @param aImageKey       Key data identifying which image the cache entry
-   *                        belongs to.
-   * @param aSurfaceKey     Key data which uniquely identifies the requested
-   *                        cache entry.
-   * @return SUCCESS if the placeholder was inserted successfully.
-   *         FAILURE if the placeholder could not be inserted for some reason.
-   *         FAILURE_ALREADY_PRESENT if an entry with the same ImageKey and
-   *           SurfaceKey already exists in the cache.
-   */
-  static InsertOutcome InsertPlaceholder(const ImageKey    aImageKey,
-                                         const SurfaceKey& aSurfaceKey);
-
-  /**
    * Mark the cache entry @aProvider as having an available surface. This turns
    * a placeholder cache entry into a normal cache entry. The cache entry
    * becomes locked if the associated image is locked; otherwise, it starts in
    * the unlocked state.
    *
    * If the cache entry containing @aProvider has already been evicted from the
    * surface cache, this function has no effect.
    *
    * It's illegal to call this function if @aProvider is not a placeholder; by
    * definition, non-placeholder ISurfaceProviders should have a surface
    * available already.
    *
-   * XXX(seth): We're currently in a transitional state where two notions of
-   * placeholder exist: the old one (placeholders are an "empty" cache entry
-   * inserted via InsertPlaceholder(), which then gets replaced by inserting a
-   * real cache entry with the same keys via Insert()) and the new one (where
-   * the same cache entry, inserted via Insert(), starts in a placeholder state
-   * and then transitions to being a normal cache entry via this function). The
-   * old mechanism will be removed in bug 1292392.
-   *
    * @param aProvider       The cache entry that now has a surface available.
    * @param aImageKey       Key data identifying which image the cache entry
    *                        belongs to.
    * @param aSurfaceKey     Key data which uniquely identifies the requested
    *                        cache entry.
    */
   static void SurfaceAvailable(NotNull<ISurfaceProvider*> aProvider,
                                const ImageKey    aImageKey,
@@ -418,17 +383,17 @@ struct SurfaceCache
    * If the image is unlocked, this has no effect.
    *
    * @param aImageKey    The image which should have its existing cache entries
    *                     unlocked.
    */
   static void UnlockEntries(const ImageKey aImageKey);
 
   /**
-   * Removes all cache entries (both real and placeholder) associated with the
+   * Removes all cache entries (including 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
    * entries 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.