GPU Program Caching

gpu/command_buffer/service/memory_program_cache.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "gpu/command_buffer/service/memory_program_cache.h"
+#include "base/bind.h"
+#include "base/file_path.h"
+#include "base/sha1.h"
+#include "net/disk_cache/disk_cache.h"
+#include "ui/gl/gl_bindings.h"
+
+namespace gpu {
+namespace gles2 {
+
+namespace {
+typedef std::map<std::string, GLint> BindAttribMap;
+} // anonymous namespace
+
+MemoryProgramCache::~MemoryProgramCache() {
+  StoreMap::const_iterator it;
+  for(it = store_.begin(); it != store_.end(); ++it) {
+    delete[] (char*)it->second.data;
+  }
+}
+
+void MemoryProgramCache::ClearBackend() {
+  curr_size_bytes_ = 0;
+  store_.clear();
+  lru_helper_.Clear();
+}
+
+bool MemoryProgramCache::LoadLinkedProgram(
+    const GLuint program,
+    ShaderManager::ShaderInfo* shader_a,
+    ShaderManager::ShaderInfo* shader_b,
+    const BindAttribMap* bind_attrib_location_map) const {
+  char a_sha[kHashLength];
+  char b_sha[kHashLength];
+  ComputeShaderHash(*shader_a->source(), a_sha);
+  ComputeShaderHash(*shader_b->source(), b_sha);
+
+  char sha[kHashLength];
+  ComputeProgramHash(a_sha,
+                     b_sha,
+                     bind_attrib_location_map,
+                     sha);
+  const std::string shaString(sha, kHashLength);
+
+  StoreMap::const_iterator found = store_.find(shaString);
+  if(found == store_.end()) {
+    return false;
+  }
+  StoreValue value = found->second;
+  glProgramBinary(program, value.format, value.data, value.length);
+  shader_a->SetAttribMap(value.attrib_map_0);
+  shader_a->SetUniformMap(value.uniform_map_0);
+  shader_b->SetAttribMap(value.attrib_map_1);
+  shader_b->SetUniformMap(value.uniform_map_1);
+  return true;
+}
+
+void MemoryProgramCache::SaveLinkedProgram(
+    const GLuint program,
+    const ShaderManager::ShaderInfo* shader_a,
+    const ShaderManager::ShaderInfo* shader_b,
+    const BindAttribMap* bind_attrib_location_map) {
+  GLsizei length;
+  GLenum format;
+  GLsizei bufferlength = 0;
+  glGetProgramiv(program, 0x8741, &bufferlength);
+  GLvoid* binary = new char[bufferlength];

[greggman, 2012/06/25 18:53:03]

binary will be leaked if length == 0 below

I think the best way to handle this would be as follows

scoped_array<char> binary(new char[buffer_length]);

Then change StoreValue to have a scoped_array<char> data

When you call StoreValue's constructor you can call it like this

StoreValue value(length,
                 format,
                 binary.release(),
                 ..

This will make it safe here and it will mean no need to manually delete that
memory in the 2 places in the code that's there now.

[dmurph, 2012/06/26 02:32:56]

Done.

+  glGetProgramBinary(program,
+                     bufferlength,
+                     &length,
+                     &format,
+                     binary);
+  if(length == 0) {
+    return;
+  }
+
+  char a_sha[kHashLength];
+  char b_sha[kHashLength];
+  ComputeShaderHash(*shader_a->source(), a_sha);
+  ComputeShaderHash(*shader_b->source(), b_sha);
+
+  StoreValue value(length,
+                   format,
+                   binary,
+                   a_sha,
+                   shader_a->attrib_map(),
+                   shader_a->uniform_map(),
+                   b_sha,
+                   shader_b->attrib_map(),
+                   shader_b->uniform_map());
+
+  char sha[kHashLength];
+  ComputeProgramHash(a_sha,
+                     b_sha,
+                     bind_attrib_location_map,
+                     sha);
+  const std::string shaString(sha, kHashLength);
+
+  if(store_.find(shaString) != store_.end()) {
+    return;
+  }
+
+  while(curr_size_bytes_ + length > max_size_bytes_) {
+    if(lru_helper_.IsEmpty()) {
+      return;
+    }
+    std::string program = lru_helper_.EvictKey();

[greggman, 2012/06/25 18:53:03]

could this be const str::string& program? Then no copy will be made

[dmurph, 2012/06/26 02:32:56]

Done.

+    StoreValue evictedValue = store_[program];

[greggman, 2012/06/25 18:53:03]

style: variables use under_score

[greggman, 2012/06/25 18:53:03]

This line copies relatively large structure just for the point of deleting it.
You could use const StoreValue& value_to_evict = store_[program]

Or, better yet, you could use find. As is is store_[program] and store_.erase
both have to do  a search where as 

iterator = store_.find()

...

store_.erase(iterator)

only searches once.

[dmurph, 2012/06/26 02:32:56]

Done.

+    store_.erase(program);
+    delete[] (char*)evictedValue.data;
+    curr_size_bytes_ -= evictedValue.length;
+    Evict(program, evictedValue.shader_0_hash, evictedValue.shader_1_hash);
+  }
+  store_[shaString] = value;
+  curr_size_bytes_ += length;
+  lru_helper_.KeyUsed(shaString);
+  LinkedProgramCacheSuccess(shaString,
+                         std::string(a_sha, kHashLength),
+                         std::string(b_sha, kHashLength));
+}
+
+} // namespace gles2
+} // namespace gpu