Bug 1397304 Avoid searching the image cache queue for an entry after we just popped it off the queue. r=tnikkel
authorBen Kelly <ben@wanderview.com>
Wed, 06 Sep 2017 13:12:05 -0700
changeset 379239 615eb9ac1663b2689cde20321ad46a8d984897e3
parent 379238 23b8b4b23566594fcde199bbd64e8f8a07b9e220
child 379240 9018e3691e117111fa75907f5fe4481c3db37244
push id94623
push userbkelly@mozilla.com
push dateWed, 06 Sep 2017 20:12:12 +0000
treeherdermozilla-inbound@615eb9ac1663 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1397304
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 1397304 Avoid searching the image cache queue for an entry after we just popped it off the queue. r=tnikkel
image/imgLoader.cpp
image/imgLoader.h
--- a/image/imgLoader.cpp
+++ b/image/imgLoader.cpp
@@ -1063,16 +1063,22 @@ imgCacheQueue::IsDirty()
 }
 
 uint32_t
 imgCacheQueue::GetNumElements() const
 {
   return mQueue.Length();
 }
 
+bool
+imgCacheQueue::Contains(imgCacheEntry* aEntry) const
+{
+  return mQueue.Contains(aEntry);
+}
+
 imgCacheQueue::iterator
 imgCacheQueue::begin()
 {
   return mQueue.begin();
 }
 
 imgCacheQueue::const_iterator
 imgCacheQueue::begin() const
@@ -1675,17 +1681,19 @@ imgLoader::CheckCacheLimits(imgCacheTabl
       if (req) {
         LOG_STATIC_FUNC_WITH_PARAM(gImgLog,
                                    "imgLoader::CheckCacheLimits",
                                    "entry", req->CacheKey().Spec());
       }
     }
 
     if (entry) {
-      RemoveFromCache(entry);
+      // We just popped this entry from the queue, so pass AlreadyRemoved
+      // to avoid searching the queue again in RemoveFromCache.
+      RemoveFromCache(entry, QueueState::AlreadyRemoved);
     }
   }
 }
 
 bool
 imgLoader::ValidateRequestWithNewChannel(imgRequest* request,
                                          nsIURI* aURI,
                                          nsIURI* aInitialDocumentURI,
@@ -1974,17 +1982,17 @@ imgLoader::RemoveFromCache(const ImageCa
     AddToUncachedImages(request);
 
     return true;
   }
   return false;
 }
 
 bool
-imgLoader::RemoveFromCache(imgCacheEntry* entry)
+imgLoader::RemoveFromCache(imgCacheEntry* entry, QueueState aQueueState)
 {
   LOG_STATIC_FUNC(gImgLog, "imgLoader::RemoveFromCache entry");
 
   RefPtr<imgRequest> request = entry->GetRequest();
   if (request) {
     const ImageCacheKey& key = request->CacheKey();
     imgCacheTable& cache = GetCache(key);
     imgCacheQueue& queue = GetCacheQueue(key);
@@ -1996,17 +2004,24 @@ imgLoader::RemoveFromCache(imgCacheEntry
     cache.Remove(key);
 
     if (entry->HasNoProxies()) {
       LOG_STATIC_FUNC(gImgLog,
                       "imgLoader::RemoveFromCache removing from tracker");
       if (mCacheTracker) {
         mCacheTracker->RemoveObject(entry);
       }
-      queue.Remove(entry);
+      // Only search the queue to remove the entry if its possible it might
+      // be in the queue.  If we know its not in the queue this would be
+      // wasted work.
+      MOZ_ASSERT_IF(aQueueState == QueueState::AlreadyRemoved,
+                    !queue.Contains(entry));
+      if (aQueueState == QueueState::MaybeExists) {
+        queue.Remove(entry);
+      }
     }
 
     entry->SetEvicted(true);
     request->SetIsInCache(false);
     AddToUncachedImages(request);
 
     return true;
   }
--- a/image/imgLoader.h
+++ b/image/imgLoader.h
@@ -196,16 +196,17 @@ public:
   void Push(imgCacheEntry*);
   void MarkDirty();
   bool IsDirty();
   already_AddRefed<imgCacheEntry> Pop();
   void Refresh();
   uint32_t GetSize() const;
   void UpdateSize(int32_t diff);
   uint32_t GetNumElements() const;
+  bool Contains(imgCacheEntry* aEntry) const;
   typedef nsTArray<RefPtr<imgCacheEntry> > queueContainer;
   typedef queueContainer::iterator iterator;
   typedef queueContainer::const_iterator const_iterator;
 
   iterator begin();
   const_iterator begin() const;
   iterator end();
   const_iterator end() const;
@@ -335,17 +336,26 @@ public:
 
   nsresult ClearChromeImageCache();
   nsresult ClearImageCache();
   void MinimizeCaches();
 
   nsresult InitCache();
 
   bool RemoveFromCache(const ImageCacheKey& aKey);
-  bool RemoveFromCache(imgCacheEntry* entry);
+
+  // Enumeration describing if a given entry is in the cache queue or not.
+  // There are some cases we know the entry is definitely not in the queue.
+  enum class QueueState {
+    MaybeExists,
+    AlreadyRemoved
+  };
+
+  bool RemoveFromCache(imgCacheEntry* entry,
+                       QueueState aQueueState = QueueState::MaybeExists);
 
   bool PutIntoCache(const ImageCacheKey& aKey, imgCacheEntry* aEntry);
 
   void AddToUncachedImages(imgRequest* aRequest);
   void RemoveFromUncachedImages(imgRequest* aRequest);
 
   // Returns true if we should prefer evicting cache entry |two| over cache
   // entry |one|.