Limit ServiceTransferCache to 2k entries

Currently, the transfer cache has a large (128MB) memory limit, but no
limit on individual entries. As we have the potential to flood the
cache with thousands of small entries, causing an explosion of handle
allocations, I've added a 2000 element cap.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I1245220bca1ea47546be7d28eaea2e494e3468db
Reviewed-on: https://chromium-review.googlesource.com/1147311Reviewed-by: Khushal <khushalsagar@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577312}

cc/paint/transfer_cache_unittest.cc

@@ -122,7 +122,7 @@ TEST_F(TransferCacheTest, Basic) {
                 decoder_id(), entry.Type(), entry.Id())));
 }
 
-TEST_F(TransferCacheTest, Eviction) {
+TEST_F(TransferCacheTest, MemoryEviction) {
   auto* service_cache = ServiceTransferCache();
   auto* context_support = ContextSupport();
 
@@ -152,6 +152,31 @@ TEST_F(TransferCacheTest, Eviction) {
       entry.UnsafeType(), entry.Id()));
 }
 
+TEST_F(TransferCacheTest, CountEviction) {
+  auto* service_cache = ServiceTransferCache();
+  auto* context_support = ContextSupport();
+
+  // Create 10 entries and leave them all unlocked.
+  std::vector<std::unique_ptr<ClientRawMemoryTransferCacheEntry>> entries;
+  for (int i = 0; i < 10; ++i) {
+    entries.emplace_back(std::make_unique<ClientRawMemoryTransferCacheEntry>(
+        std::vector<uint8_t>(4)));
+    CreateEntry(*entries[i]);
+    context_support->UnlockTransferCacheEntries(
+        {{entries[i]->UnsafeType(), entries[i]->Id()}});
+    ri()->Finish();
+  }
+
+  // These entries should be using up space.
+  EXPECT_EQ(service_cache->cache_size_for_testing(), 40u);
+
+  // Evict on the service side.
+  service_cache->SetMaxCacheEntriesForTesting(5);
+
+  // Half the entries should be evicted.
+  EXPECT_EQ(service_cache->cache_size_for_testing(), 20u);
+}
+
 TEST_F(TransferCacheTest, RawMemoryTransfer) {
   auto* service_cache = ServiceTransferCache();
 

gpu/command_buffer/service/service_transfer_cache.cc

@@ -18,6 +18,10 @@
 namespace gpu {
 namespace {
 
+// Put an arbitrary (high) limit on number of cache entries to prevent
+// unbounded handle growth with tiny entries.
+static size_t kMaxCacheEntries = 2000;
+
 size_t CacheSizeLimit() {
   size_t memory_usage = 128 * 1024 * 1024;
   if (base::SysInfo::IsLowEndDevice())
@@ -70,7 +74,9 @@ ServiceTransferCache::CacheEntryInternal::operator=(
     CacheEntryInternal&& other) = default;
 
 ServiceTransferCache::ServiceTransferCache()
-    : entries_(EntryCache::NO_AUTO_EVICT), cache_size_limit_(CacheSizeLimit()) {
+    : entries_(EntryCache::NO_AUTO_EVICT),
+      cache_size_limit_(CacheSizeLimit()),
+      max_cache_entries_(kMaxCacheEntries) {
   // In certain cases, ThreadTaskRunnerHandle isn't set (Android Webview).
   // Don't register a dump provider in these cases.
   if (base::ThreadTaskRunnerHandle::IsSet()) {
@@ -154,7 +160,8 @@ cc::ServiceTransferCacheEntry* ServiceTransferCache::GetEntry(
 
 void ServiceTransferCache::EnforceLimits() {
   for (auto it = entries_.rbegin(); it != entries_.rend();) {
-    if (total_size_ <= cache_size_limit_) {
+    if (total_size_ <= cache_size_limit_ &&
+        entries_.size() <= max_cache_entries_) {
       return;
     }
     if (it->second.handle && !it->second.handle->Delete()) {

gpu/command_buffer/service/service_transfer_cache.h

@@ -62,6 +62,10 @@ class GPU_GLES2_EXPORT ServiceTransferCache
     cache_size_limit_ = cache_size_limit;
     EnforceLimits();
   }
+  void SetMaxCacheEntriesForTesting(size_t max_cache_entries) {
+    max_cache_entries_ = max_cache_entries;
+    EnforceLimits();
+  }
   size_t cache_size_for_testing() const { return total_size_; }
 
  private:
@@ -97,6 +101,9 @@ class GPU_GLES2_EXPORT ServiceTransferCache
   // The limit above which the cache will start evicting resources.
   size_t cache_size_limit_ = 0;
 
+  // The max number of entries we will hold in the cache.
+  size_t max_cache_entries_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(ServiceTransferCache);
 };