Limit GPU Image Decode Cache Working Set by Count

Currently we limit GPU Image Decode Cache working set by bytes, but
this allows the cache to be filled with thousands of working set
entries if sufficiently small. This ends up causing negative
performance and memory spikes vs flushing more regularly.

This change adds a 256 item limit to the GPU IDC working set, avoiding
the worst of these issues.

Bug: 870455
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I0e6f27d3cb070ea3c29940ce9aad61224a3120cf
Reviewed-on: https://chromium-review.googlesource.com/1179378
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584286}

cc/tiles/gpu_image_decode_cache.cc

@@ -44,6 +44,12 @@ static const int kNormalMaxItemsInCacheForGpu = 2000;
 static const int kThrottledMaxItemsInCacheForGpu = 100;
 static const int kSuspendedMaxItemsInCacheForGpu = 0;
 
+// The maximum number of images that we can lock simultaneously in our working
+// set. This is separate from the memory limit, as keeping very large numbers
+// of small images simultaneously locked can lead to performance issues and
+// memory spikes.
+static const int kMaxItemsInWorkingSet = 256;
+
 // lock_count │ used  │ result state
 // ═══════════╪═══════╪══════════════════
 //  1         │ false │ WASTED_ONCE
@@ -674,7 +680,8 @@ GpuImageDecodeCache::GpuImageDecodeCache(viz::RasterContextProvider* context,
       context_(context),
       max_texture_size_(max_texture_size),
       persistent_cache_(PersistentCache::NO_AUTO_EVICT),
-      max_working_set_bytes_(max_working_set_bytes) {
+      max_working_set_bytes_(max_working_set_bytes),
+      max_working_set_items_(kMaxItemsInWorkingSet) {
   // In certain cases, ThreadTaskRunnerHandle isn't set (Android Webview).
   // Don't register a dump provider in these cases.
   if (base::ThreadTaskRunnerHandle::IsSet()) {
@@ -1290,7 +1297,9 @@ void GpuImageDecodeCache::OwnershipChanged(const DrawImage& draw_image,
   // If we have no image refs on an image, we should unbudget it.
   if (!has_any_refs && image_data->is_budgeted) {
     DCHECK_GE(working_set_bytes_, image_data->size);
+    DCHECK_GE(working_set_items_, 1u);
     working_set_bytes_ -= image_data->size;
+    working_set_items_ -= 1;
     image_data->is_budgeted = false;
   }
 
@@ -1329,6 +1338,7 @@ void GpuImageDecodeCache::OwnershipChanged(const DrawImage& draw_image,
   if (has_any_refs && !image_data->is_budgeted &&
       CanFitInWorkingSet(image_data->size)) {
     working_set_bytes_ += image_data->size;
+    working_set_items_ += 1;
     image_data->is_budgeted = true;
   }
 
@@ -1393,9 +1403,15 @@ bool GpuImageDecodeCache::EnsureCapacity(size_t required_size) {
 bool GpuImageDecodeCache::CanFitInWorkingSet(size_t size) const {
   lock_.AssertAcquired();
 
+  if (working_set_items_ >= max_working_set_items_)
+    return false;
+
   base::CheckedNumeric<uint32_t> new_size(working_set_bytes_);
   new_size += size;
-  return new_size.IsValid() && new_size.ValueOrDie() <= max_working_set_bytes_;
+  if (!new_size.IsValid() || new_size.ValueOrDie() > max_working_set_bytes_)
+    return false;
+
+  return true;
 }
 
 bool GpuImageDecodeCache::ExceedsPreferredCount() const {

cc/tiles/gpu_image_decode_cache.h

@@ -157,8 +157,9 @@ class CC_EXPORT GpuImageDecodeCache
   bool SupportsColorSpaceConversion() const;
 
   // For testing only.
-  void SetWorkingSetLimitForTesting(size_t limit) {
-    max_working_set_bytes_ = limit;
+  void SetWorkingSetLimitsForTesting(size_t bytes_limit, size_t items_limit) {
+    max_working_set_bytes_ = bytes_limit;
+    max_working_set_items_ = items_limit;
   }
   size_t GetWorkingSetBytesForTesting() const { return working_set_bytes_; }
   size_t GetNumCacheEntriesForTesting() const {
@@ -505,7 +506,9 @@ class CC_EXPORT GpuImageDecodeCache
   InUseCache in_use_cache_;
 
   size_t max_working_set_bytes_ = 0;
+  size_t max_working_set_items_ = 0;
   size_t working_set_bytes_ = 0;
+  size_t working_set_items_ = 0;
   base::MemoryState memory_state_ = base::MemoryState::NORMAL;
   bool aggressively_freeing_resources_ = false;
 

cc/tiles/gpu_image_decode_cache_unittest.cc

@@ -922,7 +922,7 @@ TEST_P(GpuImageDecodeCacheTest, GetDecodedImageForDrawAtRasterDecode) {
   bool is_decomposable = true;
   SkFilterQuality quality = kHigh_SkFilterQuality;
 
-  cache->SetWorkingSetLimitForTesting(0);
+  cache->SetWorkingSetLimitsForTesting(0 /* max_bytes */, 0 /* max_items */);
 
   PaintImage image = CreateDiscardablePaintImage(gfx::Size(100, 100));
   DrawImage draw_image(image, SkIRect::MakeWH(image.width(), image.height()),
@@ -1139,7 +1139,7 @@ TEST_P(GpuImageDecodeCacheTest, AtRasterUsedDirectlyIfSpaceAllows) {
   bool is_decomposable = true;
   SkFilterQuality quality = kHigh_SkFilterQuality;
 
-  cache->SetWorkingSetLimitForTesting(0);
+  cache->SetWorkingSetLimitsForTesting(0 /* max_bytes */, 0 /* max_items */);
 
   PaintImage image = CreateDiscardablePaintImage(gfx::Size(100, 100));
   DrawImage draw_image(image, SkIRect::MakeWH(image.width(), image.height()),
@@ -1165,7 +1165,8 @@ TEST_P(GpuImageDecodeCacheTest, AtRasterUsedDirectlyIfSpaceAllows) {
 
   // Increase memory limit and attempt to use the same image. It should be in
   // available for ref.
-  cache->SetWorkingSetLimitForTesting(96 * 1024 * 1024);
+  cache->SetWorkingSetLimitsForTesting(96 * 1024 * 1024 /* max_bytes */,
+                                       256 /* max_items */);
   ImageDecodeCache::TaskResult another_result =
       cache->GetTaskForImageAndRef(draw_image, ImageDecodeCache::TracingInfo());
   EXPECT_TRUE(another_result.need_unref);
@@ -1179,7 +1180,7 @@ TEST_P(GpuImageDecodeCacheTest,
   bool is_decomposable = true;
   SkFilterQuality quality = kHigh_SkFilterQuality;
 
-  cache->SetWorkingSetLimitForTesting(0);
+  cache->SetWorkingSetLimitsForTesting(0 /* max_bytes */, 0 /* max_items */);
 
   PaintImage image = CreateDiscardablePaintImage(gfx::Size(100, 100));
   DrawImage draw_image(image, SkIRect::MakeWH(image.width(), image.height()),
@@ -1217,7 +1218,7 @@ TEST_P(GpuImageDecodeCacheTest,
   bool is_decomposable = true;
   SkFilterQuality quality = kHigh_SkFilterQuality;
 
-  cache->SetWorkingSetLimitForTesting(0);
+  cache->SetWorkingSetLimitsForTesting(0 /* max_bytes */, 0 /* max_items */);
   PaintImage image = CreateDiscardablePaintImage(gfx::Size(1, 24000));
   DrawImage draw_image(image, SkIRect::MakeWH(image.width(), image.height()),
                        quality,
@@ -2044,9 +2045,6 @@ TEST_P(GpuImageDecodeCacheTest, EvictDueToCachedItemsLimit) {
   bool is_decomposable = true;
   SkFilterQuality quality = kHigh_SkFilterQuality;
 
-  // Allow a single 10x10 images.
-  cache->SetWorkingSetLimitForTesting(
-      SkColorTypeBytesPerPixel(GetParam().first) * 10 * 10 * 10);
   // Set the THROTTLED state, which limits our cache to 100 entries.
   cache->OnMemoryStateChange(base::MemoryState::THROTTLED);
 
@@ -2088,8 +2086,9 @@ TEST_P(GpuImageDecodeCacheTest, AlreadyBudgetedImagesAreNotAtRaster) {
   SkFilterQuality quality = kHigh_SkFilterQuality;
 
   // Allow a single 10x10 image and lock it.
-  cache->SetWorkingSetLimitForTesting(
-      SkColorTypeBytesPerPixel(GetParam().first) * 10 * 10 * 10);
+  cache->SetWorkingSetLimitsForTesting(
+      SkColorTypeBytesPerPixel(GetParam().first) * 10 * 10 * 10 /* max_bytes */,
+      1 /* max_items */);
   PaintImage image = CreateDiscardablePaintImage(gfx::Size(10, 10));
   DrawImage draw_image(image, SkIRect::MakeWH(image.width(), image.height()),
                        quality,
@@ -2118,14 +2117,15 @@ TEST_P(GpuImageDecodeCacheTest, AlreadyBudgetedImagesAreNotAtRaster) {
   cache->UnrefImage(draw_image);
 }
 
-TEST_P(GpuImageDecodeCacheTest, ImagesUsedDuringDrawAreBudgeted) {
+TEST_P(GpuImageDecodeCacheTest, ImageBudgetingByCount) {
   auto cache = CreateCache();
   bool is_decomposable = true;
   SkFilterQuality quality = kHigh_SkFilterQuality;
 
-  // Allow a single 10x10 image.
-  cache->SetWorkingSetLimitForTesting(
-      SkColorTypeBytesPerPixel(GetParam().first) * 10 * 10 * 10);
+  // Allow a single image by count. Use a high byte limit as we want to test the
+  // count restriction.
+  cache->SetWorkingSetLimitsForTesting(96 * 1024 * 1024 /* max_bytes */,
+                                       1 /* max_items */);
   PaintImage image = CreateDiscardablePaintImage(gfx::Size(10, 10));
   DrawImage draw_image(image, SkIRect::MakeWH(image.width(), image.height()),
                        quality,
@@ -2139,12 +2139,62 @@ TEST_P(GpuImageDecodeCacheTest, ImagesUsedDuringDrawAreBudgeted) {
   EXPECT_TRUE(decoded_draw_image.image());
   EXPECT_TRUE(decoded_draw_image.is_budgeted());
 
-  // Try another image, it shouldn't be budgeted.
+  // Try another image, it shouldn't be budgeted and should be at-raster.
   DrawImage second_draw_image(
       CreateDiscardablePaintImage(gfx::Size(100, 100)),
       SkIRect::MakeWH(image.width(), image.height()), quality,
       CreateMatrix(SkSize::Make(1.0f, 1.0f), is_decomposable),
       PaintImage::kDefaultFrameIndex, DefaultColorSpace());
+  // Should be at raster.
+  ImageDecodeCache::TaskResult result = cache->GetTaskForImageAndRef(
+      second_draw_image, ImageDecodeCache::TracingInfo());
+  EXPECT_FALSE(result.need_unref);
+  EXPECT_FALSE(result.task);
+  // Image retrieved from at-raster decode should not be budgeted.
+  DecodedDrawImage second_decoded_draw_image =
+      EnsureImageBacked(cache->GetDecodedImageForDraw(second_draw_image));
+  EXPECT_TRUE(second_decoded_draw_image.image());
+  EXPECT_FALSE(second_decoded_draw_image.is_budgeted());
+
+  cache->DrawWithImageFinished(draw_image, decoded_draw_image);
+  cache->DrawWithImageFinished(second_draw_image, second_decoded_draw_image);
+}
+
+TEST_P(GpuImageDecodeCacheTest, ImageBudgetingBySize) {
+  auto cache = CreateCache();
+  bool is_decomposable = true;
+  SkFilterQuality quality = kHigh_SkFilterQuality;
+
+  // Allow a single 10x10 image by size. Don't restrict the items limit as we
+  // want to test the size limit.
+  cache->SetWorkingSetLimitsForTesting(
+      SkColorTypeBytesPerPixel(GetParam().first) * 10 * 10 * 10 /* max_bytes */,
+      256 /* max_items */);
+  PaintImage image = CreateDiscardablePaintImage(gfx::Size(10, 10));
+  DrawImage draw_image(image, SkIRect::MakeWH(image.width(), image.height()),
+                       quality,
+                       CreateMatrix(SkSize::Make(1.0f, 1.0f), is_decomposable),
+                       PaintImage::kDefaultFrameIndex, DefaultColorSpace());
+
+  // The image counts against our budget.
+  viz::ContextProvider::ScopedContextLock context_lock(context_provider());
+  DecodedDrawImage decoded_draw_image =
+      EnsureImageBacked(cache->GetDecodedImageForDraw(draw_image));
+  EXPECT_TRUE(decoded_draw_image.image());
+  EXPECT_TRUE(decoded_draw_image.is_budgeted());
+
+  // Try another image, it shouldn't be budgeted and should be at-raster.
+  DrawImage second_draw_image(
+      CreateDiscardablePaintImage(gfx::Size(100, 100)),
+      SkIRect::MakeWH(image.width(), image.height()), quality,
+      CreateMatrix(SkSize::Make(1.0f, 1.0f), is_decomposable),
+      PaintImage::kDefaultFrameIndex, DefaultColorSpace());
+  // Should be at raster.
+  ImageDecodeCache::TaskResult result = cache->GetTaskForImageAndRef(
+      second_draw_image, ImageDecodeCache::TracingInfo());
+  EXPECT_FALSE(result.need_unref);
+  EXPECT_FALSE(result.task);
+  // Image retrieved from at-raster decode should not be budgeted.
   DecodedDrawImage second_decoded_draw_image =
       EnsureImageBacked(cache->GetDecodedImageForDraw(second_draw_image));
   EXPECT_TRUE(second_decoded_draw_image.image());