Issue #2928 - Improve imgLoader cache queue handling.

- Convert the imgCacheQueue to nsTArray from Vector
- Avoid marking the queue dirty when element operations are performed
that don't upset the sort order. This should improve performance as well.

image/imgLoader.cpp

@@ -952,12 +952,38 @@ using namespace std;
 void
 imgCacheQueue::Remove(imgCacheEntry* entry)
 {
-  queueContainer::iterator it = find(mQueue.begin(), mQueue.end(), entry);
+  uint64_t index = mQueue.IndexOf(entry);
-  if (it != mQueue.end()) {
+  if (index == queueContainer::NoIndex) {
-    mSize -= (*it)->GetDataSize();
+    return;
-    mQueue.erase(it);
-    MarkDirty();
   }
+
+  mSize -= mQueue[index]->GetDataSize();
+
+  // If the queue is clean and this is the first entry,
+  // then we can efficiently remove the entry without
+  // dirtying the sort order.
+  if (!IsDirty() && index == 0) {
+    std::pop_heap(mQueue.begin(), mQueue.end(),
+                  imgLoader::CompareCacheEntries);
+    mQueue.RemoveElementAt(mQueue.Length() - 1);
+    return;
+  }
+
+  // Remove from the middle of the list. This potentially
+  // breaks the binary heap sort order.
+  mQueue.RemoveElementAt(index);
+
+  // If we only have one entry or the queue is empty, though,
+  // then the sort order is still effectively good.
+  // Simply refresh the list to clear the dirty flag.
+  if (mQueue.Length() <= 1) {
+    Refresh();
+    return;
+  }
+
+  // Otherwise we must mark the queue dirty and potentially
+  // trigger an expensive sort later.
+  MarkDirty();
 }
 
 void
@@ -966,23 +992,26 @@ imgCacheQueue::Push(imgCacheEntry* entry)
   mSize += entry->GetDataSize();
 
   RefPtr<imgCacheEntry> refptr(entry);
-  mQueue.push_back(refptr);
+  mQueue.AppendElement(Move(refptr));
-  MarkDirty();
+  // If we're not dirty already, then we can efficiently add this to the binary heap immediately. 
+  if (!IsDirty()) {
+    std::push_heap(mQueue.begin(), mQueue.end(), imgLoader::CompareCacheEntries);
+  }
 }
 
 already_AddRefed<imgCacheEntry>
 imgCacheQueue::Pop()
 {
-  if (mQueue.empty()) {
+  if (mQueue.IsEmpty()) {
     return nullptr;
   }
   if (IsDirty()) {
     Refresh();
   }
 
-  RefPtr<imgCacheEntry> entry = mQueue[0];
   std::pop_heap(mQueue.begin(), mQueue.end(), imgLoader::CompareCacheEntries);
-  mQueue.pop_back();
+  RefPtr<imgCacheEntry> entry = Move(mQueue.LastElement());
+  mQueue.RemoveElementAt(mQueue.Length() - 1);
 
   mSize -= entry->GetDataSize();
   return entry.forget();
@@ -991,6 +1020,7 @@ imgCacheQueue::Pop()
 void
 imgCacheQueue::Refresh()
 {
+  // Re-heap the list. This is an O(3 * n) operation and best avoided if possible.
   std::make_heap(mQueue.begin(), mQueue.end(), imgLoader::CompareCacheEntries);
   mDirty = false;
 }
@@ -1010,7 +1040,7 @@ imgCacheQueue::IsDirty()
 uint32_t
 imgCacheQueue::GetNumElements() const
 {
-  return mQueue.size();
+  return mQueue.Length();
 }
 
 bool
@@ -1608,7 +1638,11 @@ void
 imgLoader::CacheEntriesChanged(bool aForChrome, int32_t aSizeDiff /* = 0 */)
 {
   imgCacheQueue& queue = GetCacheQueue(aForChrome);
+  // We only need to dirty the queue if there is any sorting taking place.
+  // Empty or single-entry lists can't become dirty.
+  if (queue.GetNumElements() > 1) {
     queue.MarkDirty();
+  }
   queue.UpdateSize(aSizeDiff);
 }
 
@@ -2016,13 +2050,13 @@ imgLoader::EvictEntries(imgCacheQueue& aQueueToClear)
   // We have to make a temporary, since RemoveFromCache removes the element
   // from the queue, invalidating iterators.
   nsTArray<RefPtr<imgCacheEntry> > entries(aQueueToClear.GetNumElements());
-  for (imgCacheQueue::const_iterator i = aQueueToClear.begin();
+  for (auto i = aQueueToClear.begin(); i != aQueueToClear.end(); ++i) {
-       i != aQueueToClear.end(); ++i) {
     entries.AppendElement(*i);
   }
 
-  for (uint32_t i = 0; i < entries.Length(); ++i) {
+  // Iterate in reverse order to minimize array copying.
-    if (!RemoveFromCache(entries[i])) {
+  for (auto& entry : entries) {
+    if (!RemoveFromCache(entry)) {
       return NS_ERROR_FAILURE;
     }
   }

image/imgLoader.h

@@ -178,7 +178,8 @@ public:
   uint32_t GetSize() const;
   void UpdateSize(int32_t diff);
   uint32_t GetNumElements() const;
-  typedef std::vector<RefPtr<imgCacheEntry> > queueContainer;
+  bool Contains(imgCacheEntry* aEntry) const;
+  typedef nsTArray<RefPtr<imgCacheEntry> > queueContainer;
   typedef queueContainer::iterator iterator;
   typedef queueContainer::const_iterator const_iterator;