gpu: Fix GrShaderCache accounting for storing duplicate keys.

The GrShaderCache currently assumes that it can't be populated with an
entry having the same key twice. This is incorrect in 2 scenarios.

1) If skia fails to link the cached binary, it will re-generate and
store a new copy.
2) If skia uses a shader before it is loaded off the disk cache, we will
find an existing entry in the memory cache when the disk cache entry is
being populated.

The above scenarios can result in errors in the internal accounting for
memory used. Fix them.

R=ericrk@chromium.org

Bug: 868121
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
Change-Id: Ifd4c1e872379b54d7c659d62ff6af09eeaad2b46
Reviewed-on: https://chromium-review.googlesource.com/1152552
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578730}

gpu/command_buffer/service/gr_shader_cache.cc

@@ -49,9 +49,15 @@ void GrShaderCache::store(const SkData& key, const SkData& data) {
   EnforceLimits(data.size());
 
   CacheKey cache_key(SkData::MakeWithCopy(key.data(), key.size()));
+  auto existing_it = store_.Get(cache_key);
+  if (existing_it != store_.end()) {
+    // Skia may ignore the cached entry and regenerate a shader if it fails to
+    // link, in which case replace the current version with the latest one.
+    EraseFromCache(existing_it);
+  }
+
   CacheData cache_data(SkData::MakeWithCopy(data.data(), data.size()));
-  auto it = store_.Put(std::move(cache_key), std::move(cache_data));
-  curr_size_bytes_ += it->second.data->size();
+  auto it = AddToCache(cache_key, std::move(cache_data));
 
   WriteToDisk(it->first, &it->second);
 }
@@ -63,16 +69,36 @@ void GrShaderCache::PopulateCache(const std::string& key,
 
   EnforceLimits(data.size());
 
+  // If we already have this in the cache, skia may have populated it before it
+  // was loaded off the disk cache. Its better to keep the latest version
+  // generated version than overwriting it here.
   CacheKey cache_key(MakeData(key));
+  if (store_.Get(cache_key) != store_.end())
+    return;
+
   CacheData cache_data(MakeData(data));
-  auto it = store_.Put(std::move(cache_key), std::move(cache_data));
-  curr_size_bytes_ += it->second.data->size();
+  auto it = AddToCache(cache_key, std::move(cache_data));
 
   // This was loaded off the disk cache, no need to push this back for disk
   // write.
   it->second.pending_disk_write = false;
 }
 
+GrShaderCache::Store::iterator GrShaderCache::AddToCache(CacheKey key,
+                                                         CacheData data) {
+  auto it = store_.Put(key, std::move(data));
+  curr_size_bytes_ += it->second.data->size();
+  return it;
+}
+
+template <typename Iterator>
+void GrShaderCache::EraseFromCache(Iterator it) {
+  DCHECK_GE(curr_size_bytes_, it->second.data->size());
+
+  curr_size_bytes_ -= it->second.data->size();
+  store_.Erase(it);
+}
+
 void GrShaderCache::CacheClientIdOnDisk(int32_t client_id) {
   client_ids_to_cache_on_disk_.insert(client_id);
 }
@@ -116,12 +142,8 @@ void GrShaderCache::WriteToDisk(const CacheKey& key, CacheData* data) {
 void GrShaderCache::EnforceLimits(size_t size_needed) {
   DCHECK_LE(size_needed, cache_size_limit_);
 
-  while (size_needed + curr_size_bytes_ > cache_size_limit_) {
-    auto it = store_.rbegin();
-    DCHECK_GE(curr_size_bytes_, it->second.data->size());
-    curr_size_bytes_ -= it->second.data->size();
-    store_.Erase(it);
-  }
+  while (size_needed + curr_size_bytes_ > cache_size_limit_)
+    EraseFromCache(store_.rbegin());
 }
 
 GrShaderCache::ScopedCacheUse::ScopedCacheUse(GrShaderCache* cache,

gpu/command_buffer/service/gr_shader_cache.h

@@ -48,6 +48,7 @@ class GPU_GLES2_EXPORT GrShaderCache
       base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level);
 
   size_t num_cache_entries() const { return store_.size(); }
+  size_t curr_size_bytes_for_testing() const { return curr_size_bytes_; }
 
  private:
   static constexpr int32_t kInvalidClientId = 0;
@@ -83,12 +84,18 @@ class GPU_GLES2_EXPORT GrShaderCache
     size_t operator()(const CacheKey& key) const { return key.hash; }
   };
 
+  using Store = base::HashingMRUCache<CacheKey, CacheData, CacheKeyHash>;
+
   void EnforceLimits(size_t size_needed);
+
+  Store::iterator AddToCache(CacheKey key, CacheData data);
+  template <typename Iterator>
+  void EraseFromCache(Iterator it);
+
   void WriteToDisk(const CacheKey& key, CacheData* data);
 
   size_t cache_size_limit_;
   size_t curr_size_bytes_ = 0u;
-  using Store = base::HashingMRUCache<CacheKey, CacheData, CacheKeyHash>;
   Store store_;
 
   Client* const client_;

gpu/command_buffer/service/gr_shader_cache_unittest.cc

@@ -20,7 +20,6 @@ class GrShaderCacheTest : public GrShaderCache::Client, public testing::Test {
   GrShaderCacheTest() : cache_(kCacheLimit, this) {}
 
   void StoreShader(const std::string& key, const std::string& shader) override {
-    CHECK_EQ(disk_cache_.count(key), 0u);
     disk_cache_[key] = shader;
   }
 
@@ -129,5 +128,57 @@ TEST_F(GrShaderCacheTest, MemoryPressure) {
   EXPECT_EQ(cache_.num_cache_entries(), 0u);
 }
 
+TEST_F(GrShaderCacheTest, StoringSameEntry) {
+  int32_t regular_client_id = 3;
+  cache_.CacheClientIdOnDisk(regular_client_id);
+
+  auto key = SkData::MakeWithCopy(kShaderKey, strlen(kShaderKey));
+  auto shader = SkData::MakeUninitialized(kCacheLimit);
+  {
+    GrShaderCache::ScopedCacheUse cache_use(&cache_, regular_client_id);
+    EXPECT_EQ(cache_.load(*key), nullptr);
+    cache_.store(*key, *shader);
+  }
+  EXPECT_EQ(cache_.num_cache_entries(), 1u);
+  EXPECT_EQ(cache_.curr_size_bytes_for_testing(), shader->size());
+
+  auto shader2 = SkData::MakeUninitialized(kCacheLimit / 2);
+  {
+    GrShaderCache::ScopedCacheUse cache_use(&cache_, regular_client_id);
+    EXPECT_NE(cache_.load(*key), nullptr);
+    cache_.store(*key, *shader2);
+  }
+  EXPECT_EQ(cache_.num_cache_entries(), 1u);
+  EXPECT_EQ(cache_.curr_size_bytes_for_testing(), shader2->size());
+}
+
+TEST_F(GrShaderCacheTest, PopulateFromDiskAfterStoring) {
+  int32_t regular_client_id = 3;
+  cache_.CacheClientIdOnDisk(regular_client_id);
+
+  auto key = SkData::MakeWithCopy(kShaderKey, strlen(kShaderKey));
+  auto shader = SkData::MakeWithCString(kShader);
+  {
+    GrShaderCache::ScopedCacheUse cache_use(&cache_, regular_client_id);
+    EXPECT_EQ(cache_.load(*key), nullptr);
+    cache_.store(*key, *shader);
+  }
+  EXPECT_EQ(cache_.num_cache_entries(), 1u);
+  EXPECT_EQ(cache_.curr_size_bytes_for_testing(), shader->size());
+
+  // Try storing a different shader with the same key.
+  std::string key_str(static_cast<const char*>(key->data()), key->size());
+  std::string shader_str(static_cast<const char*>(shader->data()),
+                         shader->size() / 2);
+  cache_.PopulateCache(key_str, shader_str);
+  {
+    GrShaderCache::ScopedCacheUse cache_use(&cache_, regular_client_id);
+    auto cached_shader = cache_.load(*key);
+    ASSERT_TRUE(cached_shader);
+    EXPECT_TRUE(cached_shader->equals(shader.get()));
+  }
+  EXPECT_EQ(disk_cache_.size(), 1u);
+}
+
 }  // namespace raster
 }  // namespace gpu