Bug 1296828 (Part 3) - Update SurfaceCache API to rely on ImageKeys and SurfaceKeys stored on ISurfaceProviders. r=dholbert
authorSeth Fowler <mark.seth.fowler@gmail.com>
Fri, 19 Aug 2016 20:26:59 -0700
changeset 355066 d28b09df98ff4a1fa02b37542b99f7a6b5191071
parent 355065 8f8d3c106410aed2252be6180c542aef3b8ad738
child 355067 90c8439299ea297d780b0a5ec2d81468520437d0
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
bugs1296828
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 1296828 (Part 3) - Update SurfaceCache API to rely on ImageKeys and SurfaceKeys stored on ISurfaceProviders. r=dholbert
image/AnimationSurfaceProvider.cpp
image/DecodedSurfaceProvider.cpp
image/DecoderFactory.cpp
image/ISurfaceProvider.h
image/SurfaceCache.cpp
image/SurfaceCache.h
image/VectorImage.cpp
--- a/image/AnimationSurfaceProvider.cpp
+++ b/image/AnimationSurfaceProvider.cpp
@@ -248,19 +248,17 @@ AnimationSurfaceProvider::AnnounceSurfac
   mFramesMutex.AssertNotCurrentThreadOwns();
   MOZ_ASSERT(mImage);
 
   // We just got the first frame; let the surface cache know. We deliberately do
   // this outside of mFramesMutex to avoid a potential deadlock with
   // AddSizeOfExcludingThis(), since otherwise we'd be acquiring mFramesMutex
   // and then the surface cache lock, while the memory reporting code would
   // acquire the surface cache lock and then mFramesMutex.
-  SurfaceCache::SurfaceAvailable(WrapNotNull(this),
-                                 GetImageKey(),
-                                 GetSurfaceKey());
+  SurfaceCache::SurfaceAvailable(WrapNotNull(this));
 }
 
 void
 AnimationSurfaceProvider::FinishDecoding()
 {
   mDecodingMutex.AssertCurrentThreadOwns();
   MOZ_ASSERT(mImage);
   MOZ_ASSERT(mDecoder);
--- a/image/DecodedSurfaceProvider.cpp
+++ b/image/DecodedSurfaceProvider.cpp
@@ -182,19 +182,17 @@ DecodedSurfaceProvider::CheckForNewSurfa
   // We don't have a surface yet; try to get one from the decoder.
   mSurface = mDecoder->GetCurrentFrameRef().get();
   if (!mSurface) {
     return;  // No surface yet.
   }
 
   // We just got a surface for the first time; let the surface cache know.
   MOZ_ASSERT(mImage);
-  SurfaceCache::SurfaceAvailable(WrapNotNull(this),
-                                 GetImageKey(),
-                                 GetSurfaceKey());
+  SurfaceCache::SurfaceAvailable(WrapNotNull(this));
 }
 
 void
 DecodedSurfaceProvider::FinishDecoding()
 {
   mMutex.AssertCurrentThreadOwns();
   MOZ_ASSERT(mImage);
   MOZ_ASSERT(mDecoder);
--- a/image/DecoderFactory.cpp
+++ b/image/DecoderFactory.cpp
@@ -145,19 +145,17 @@ DecoderFactory::CreateDecoder(DecoderTyp
     RasterSurfaceKey(aOutputSize, aSurfaceFlags, PlaybackType::eStatic);
   NotNull<RefPtr<DecodedSurfaceProvider>> provider =
     WrapNotNull(new DecodedSurfaceProvider(aImage,
                                            surfaceKey,
                                            WrapNotNull(decoder)));
 
   // Attempt to insert the surface provider into the surface cache right away so
   // we won't trigger any more decoders with the same parameters.
-  InsertOutcome outcome =
-    SurfaceCache::Insert(provider, ImageKey(aImage.get()), surfaceKey);
-  if (outcome != InsertOutcome::SUCCESS) {
+  if (SurfaceCache::Insert(provider) != InsertOutcome::SUCCESS) {
     return nullptr;
   }
 
   // Return the surface provider in its IDecodingTask guise.
   RefPtr<IDecodingTask> task = provider.get();
   return task.forget();
 }
 
@@ -197,19 +195,17 @@ DecoderFactory::CreateAnimationDecoder(D
     RasterSurfaceKey(aIntrinsicSize, aSurfaceFlags, PlaybackType::eAnimated);
   NotNull<RefPtr<AnimationSurfaceProvider>> provider =
     WrapNotNull(new AnimationSurfaceProvider(aImage,
                                              surfaceKey,
                                              WrapNotNull(decoder)));
 
   // Attempt to insert the surface provider into the surface cache right away so
   // we won't trigger any more decoders with the same parameters.
-  InsertOutcome outcome =
-    SurfaceCache::Insert(provider, ImageKey(aImage.get()), surfaceKey);
-  if (outcome != InsertOutcome::SUCCESS) {
+  if (SurfaceCache::Insert(provider) != InsertOutcome::SUCCESS) {
     return nullptr;
   }
 
   // Return the surface provider in its IDecodingTask guise.
   RefPtr<IDecodingTask> task = provider.get();
   return task.forget();
 }
 
--- a/image/ISurfaceProvider.h
+++ b/image/ISurfaceProvider.h
@@ -51,17 +51,18 @@ public:
   /// @return a (potentially lazily computed) drawable reference to a surface.
   virtual DrawableSurface Surface();
 
   /// @return true if DrawableRef() will return a completely decoded surface.
   virtual bool IsFinished() const = 0;
 
   /// @return the number of bytes of memory this ISurfaceProvider is expected to
   /// require. Optimizations may result in lower real memory usage. Trivial
-  /// overhead is ignored.
+  /// overhead is ignored. Because this value is used in bookkeeping, it's
+  /// important that it be constant over the lifetime of this object.
   virtual size_t LogicalSizeInBytes() const = 0;
 
   /// @return the actual number of bytes of memory this ISurfaceProvider is
   /// using. May vary over the lifetime of the ISurfaceProvider. The default
   /// implementation is appropriate for static ISurfaceProviders.
   virtual void AddSizeOfExcludingThis(MallocSizeOf aMallocSizeOf,
                                       size_t& aHeapSizeOut,
                                       size_t& aNonHeapSizeOut)
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -119,27 +119,19 @@ private:
  */
 class CachedSurface
 {
   ~CachedSurface() { }
 public:
   MOZ_DECLARE_REFCOUNTED_TYPENAME(CachedSurface)
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CachedSurface)
 
-  CachedSurface(NotNull<ISurfaceProvider*> aProvider,
-                const Cost                 aCost,
-                const ImageKey             aImageKey,
-                const SurfaceKey&          aSurfaceKey)
+  explicit CachedSurface(NotNull<ISurfaceProvider*> aProvider)
     : mProvider(aProvider)
-    , mCost(aCost)
-    , mImageKey(aImageKey)
-    , mSurfaceKey(aSurfaceKey)
-  {
-    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();
     }
 
@@ -154,21 +146,25 @@ public:
 
     mProvider->SetLocked(aLocked);
   }
 
   bool IsLocked() const { return !IsPlaceholder() && mProvider->IsLocked(); }
   bool IsPlaceholder() const { return mProvider->Availability().IsPlaceholder(); }
   bool IsDecoded() const { return !IsPlaceholder() && mProvider->IsFinished(); }
 
-  ImageKey GetImageKey() const { return mImageKey; }
-  SurfaceKey GetSurfaceKey() const { return mSurfaceKey; }
-  CostEntry GetCostEntry() { return image::CostEntry(WrapNotNull(this), mCost); }
+  ImageKey GetImageKey() const { return mProvider->GetImageKey(); }
+  SurfaceKey GetSurfaceKey() const { return mProvider->GetSurfaceKey(); }
   nsExpirationState* GetExpirationState() { return &mExpirationState; }
 
+  CostEntry GetCostEntry()
+  {
+    return image::CostEntry(WrapNotNull(this), mProvider->LogicalSizeInBytes());
+  }
+
   // A helper type used by SurfaceCacheImpl::CollectSizeOfSurfaces.
   struct MOZ_STACK_CLASS SurfaceMemoryReport
   {
     SurfaceMemoryReport(nsTArray<SurfaceMemoryCounter>& aCounters,
                         MallocSizeOf                    aMallocSizeOf)
       : mCounters(aCounters)
       , mMallocSizeOf(aMallocSizeOf)
     { }
@@ -199,19 +195,16 @@ public:
   private:
     nsTArray<SurfaceMemoryCounter>& mCounters;
     MallocSizeOf                    mMallocSizeOf;
   };
 
 private:
   nsExpirationState                 mExpirationState;
   NotNull<RefPtr<ISurfaceProvider>> mProvider;
-  const Cost                        mCost;
-  const ImageKey                    mImageKey;
-  const SurfaceKey                  mSurfaceKey;
 };
 
 static int64_t
 AreaOfIntSize(const IntSize& aSize) {
   return static_cast<int64_t>(aSize.width) * static_cast<int64_t>(aSize.height);
 }
 
 /**
@@ -232,21 +225,21 @@ public:
   MOZ_DECLARE_REFCOUNTED_TYPENAME(ImageSurfaceCache)
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ImageSurfaceCache)
 
   typedef
     nsRefPtrHashtable<nsGenericHashKey<SurfaceKey>, CachedSurface> SurfaceTable;
 
   bool IsEmpty() const { return mSurfaces.Count() == 0; }
 
-  void Insert(const SurfaceKey& aKey, NotNull<CachedSurface*> aSurface)
+  void Insert(NotNull<CachedSurface*> aSurface)
   {
     MOZ_ASSERT(!mLocked || aSurface->IsPlaceholder() || aSurface->IsLocked(),
                "Inserting an unlocked surface for a locked image");
-    mSurfaces.Put(aKey, aSurface);
+    mSurfaces.Put(aSurface->GetSurfaceKey(), aSurface);
   }
 
   void Remove(NotNull<CachedSurface*> aSurface)
   {
     MOZ_ASSERT(mSurfaces.GetWeak(aSurface->GetSurfaceKey()),
         "Should not be removing a surface we don't have");
 
     mSurfaces.Remove(aSurface->GetSurfaceKey());
@@ -412,81 +405,81 @@ private:
   }
 
 public:
   void InitMemoryReporter() { RegisterWeakMemoryReporter(this); }
 
   Mutex& GetMutex() { return mMutex; }
 
   InsertOutcome Insert(NotNull<ISurfaceProvider*> aProvider,
-                       const Cost                 aCost,
-                       const ImageKey             aImageKey,
-                       const SurfaceKey&          aSurfaceKey,
                        bool                       aSetAvailable)
   {
     // If this is a duplicate surface, refuse to replace the original.
     // XXX(seth): Calling Lookup() and then RemoveEntry() does the lookup
     // twice. We'll make this more efficient in bug 1185137.
-    LookupResult result = Lookup(aImageKey, aSurfaceKey, /* aMarkUsed = */ false);
+    LookupResult result = Lookup(aProvider->GetImageKey(),
+                                 aProvider->GetSurfaceKey(),
+                                 /* aMarkUsed = */ false);
     if (MOZ_UNLIKELY(result)) {
       return InsertOutcome::FAILURE_ALREADY_PRESENT;
     }
 
     if (result.Type() == MatchType::PENDING) {
-      RemoveEntry(aImageKey, aSurfaceKey);
+      RemoveEntry(aProvider->GetImageKey(), aProvider->GetSurfaceKey());
     }
 
     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))) {
+    Cost cost = aProvider->LogicalSizeInBytes();
+    if (MOZ_UNLIKELY(!CanHoldAfterDiscarding(cost))) {
       mOverflowCount++;
       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) {
+    while (cost > mAvailableCost) {
       MOZ_ASSERT(!mCosts.IsEmpty(),
                  "Removed everything and it still won't fit");
       Remove(mCosts.LastElement().Surface());
     }
 
     // Locate the appropriate per-image cache. If there's not an existing cache
     // for this image, create it.
-    RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
+    RefPtr<ImageSurfaceCache> cache = GetImageCache(aProvider->GetImageKey());
     if (!cache) {
       cache = new ImageSurfaceCache;
-      mImageCaches.Put(aImageKey, cache);
+      mImageCaches.Put(aProvider->GetImageKey(), cache);
     }
 
     // If we were asked to mark the cache entry available, do so.
     if (aSetAvailable) {
       aProvider->Availability().SetAvailable();
     }
 
     NotNull<RefPtr<CachedSurface>> surface =
-      WrapNotNull(new CachedSurface(aProvider, aCost, aImageKey, aSurfaceKey));
+      WrapNotNull(new CachedSurface(aProvider));
 
     // We require that locking succeed if the image is locked and we're not
     // inserting a placeholder; the caller may need to know this to handle
     // errors correctly.
     if (cache->IsLocked() && !surface->IsPlaceholder()) {
       surface->SetLocked(true);
       if (!surface->IsLocked()) {
         return InsertOutcome::FAILURE;
       }
     }
 
     // Insert.
-    MOZ_ASSERT(aCost <= mAvailableCost, "Inserting despite too large a cost");
-    cache->Insert(aSurfaceKey, surface);
+    MOZ_ASSERT(cost <= mAvailableCost, "Inserting despite too large a cost");
+    cache->Insert(surface);
     StartTracking(surface);
 
     return InsertOutcome::SUCCESS;
   }
 
   void Remove(NotNull<CachedSurface*> aSurface)
   {
     ImageKey imageKey = aSurface->GetImageKey();
@@ -649,33 +642,30 @@ public:
     return aCost <= mMaxCost;
   }
 
   size_t MaximumCapacity() const
   {
     return size_t(mMaxCost);
   }
 
-  void SurfaceAvailable(NotNull<ISurfaceProvider*> aProvider,
-                        const ImageKey             aImageKey,
-                        const SurfaceKey&          aSurfaceKey)
+  void SurfaceAvailable(NotNull<ISurfaceProvider*> aProvider)
   {
     if (!aProvider->Availability().IsPlaceholder()) {
       MOZ_ASSERT_UNREACHABLE("Calling SurfaceAvailable on non-placeholder");
       return;
     }
 
     // Reinsert the provider, requesting that Insert() mark it available. This
     // may or may not succeed, depending on whether some other decoder has
     // beaten us to the punch and inserted a non-placeholder version of this
     // surface first, but it's fine either way.
     // XXX(seth): This could be implemented more efficiently; we should be able
     // to just update our data structures without reinserting.
-    Cost cost = aProvider->LogicalSizeInBytes();
-    Insert(aProvider, cost, aImageKey, aSurfaceKey, /* aSetAvailable = */ true);
+    Insert(aProvider, /* aSetAvailable = */ true);
   }
 
   void LockImage(const ImageKey aImageKey)
   {
     RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache) {
       cache = new ImageSurfaceCache;
       mImageCaches.Put(aImageKey, cache);
@@ -1023,28 +1013,24 @@ SurfaceCache::LookupBestMatch(const Imag
     return LookupResult(MatchType::NOT_FOUND);
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
   return sInstance->LookupBestMatch(aImageKey, aSurfaceKey);
 }
 
 /* static */ InsertOutcome
-SurfaceCache::Insert(NotNull<ISurfaceProvider*> aProvider,
-                     const ImageKey             aImageKey,
-                     const SurfaceKey&          aSurfaceKey)
+SurfaceCache::Insert(NotNull<ISurfaceProvider*> aProvider)
 {
   if (!sInstance) {
     return InsertOutcome::FAILURE;
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
-  Cost cost = aProvider->LogicalSizeInBytes();
-  return sInstance->Insert(aProvider, cost, aImageKey, aSurfaceKey,
-                           /* aSetAvailable = */ false);
+  return sInstance->Insert(aProvider, /* aSetAvailable = */ false);
 }
 
 /* static */ bool
 SurfaceCache::CanHold(const IntSize& aSize, uint32_t aBytesPerPixel /* = 4 */)
 {
   if (!sInstance) {
     return false;
   }
@@ -1059,26 +1045,24 @@ SurfaceCache::CanHold(size_t aSize)
   if (!sInstance) {
     return false;
   }
 
   return sInstance->CanHold(aSize);
 }
 
 /* static */ void
-SurfaceCache::SurfaceAvailable(NotNull<ISurfaceProvider*> aProvider,
-                               const ImageKey             aImageKey,
-                               const SurfaceKey&          aSurfaceKey)
+SurfaceCache::SurfaceAvailable(NotNull<ISurfaceProvider*> aProvider)
 {
   if (!sInstance) {
     return;
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
-  sInstance->SurfaceAvailable(aProvider, aImageKey, aSurfaceKey);
+  sInstance->SurfaceAvailable(aProvider);
 }
 
 /* static */ void
 SurfaceCache::LockImage(const ImageKey aImageKey)
 {
   if (sInstance) {
     MutexAutoLock lock(sInstance->GetMutex());
     return sInstance->LockImage(aImageKey);
--- a/image/SurfaceCache.h
+++ b/image/SurfaceCache.h
@@ -264,54 +264,42 @@ struct SurfaceCache
    * FAILURE if it failed to insert the cache entry, which could happen because
    * of capacity reasons, or because it was already freed by the OS. If the
    * cache entry isn't associated with a locked image, checking for SUCCESS or
    * FAILURE is useless: the entry 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 aProvider    The new cache entry to insert into the cache.
-   * @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 cache entry was inserted successfully. (But see above
    *           for more information about when you should check this.)
    *         FAILURE if the cache entry 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 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);
+  static InsertOutcome Insert(NotNull<ISurfaceProvider*> aProvider);
 
   /**
    * 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.
    *
    * @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,
-                               const SurfaceKey& aSurfaceKey);
+  static void SurfaceAvailable(NotNull<ISurfaceProvider*> aProvider);
 
   /**
    * 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.
    *
--- a/image/VectorImage.cpp
+++ b/image/VectorImage.cpp
@@ -960,17 +960,17 @@ VectorImage::CreateSurfaceAndShow(const 
   if (!surface) {
     return Show(svgDrawable, aParams);
   }
 
   // Attempt to cache the frame.
   SurfaceKey surfaceKey = VectorSurfaceKey(aParams.size, aParams.svgContext);
   NotNull<RefPtr<ISurfaceProvider>> provider =
     WrapNotNull(new SimpleSurfaceProvider(ImageKey(this), surfaceKey, frame));
-  SurfaceCache::Insert(provider, ImageKey(this), surfaceKey);
+  SurfaceCache::Insert(provider);
 
   // Draw.
   RefPtr<gfxDrawable> drawable =
     new gfxSurfaceDrawable(surface, aParams.size);
   Show(drawable, aParams);
 
   // Send out an invalidation so that surfaces that are still in use get
   // re-locked. See the discussion of the UnlockSurfaces call above.