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/sha1.h"
+#include "gpu/command_buffer/service/gl_utils.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;

[greggman, 2012/06/26 23:00:27]

Sorry I didn't make myself clearer last time. It seems like the point of making
a typedef is not to save typing, it's to make it so you can change the type in
one place and have it effect all the places that use it. So either put this in
the header as a type exposed by MemoryProgramCache or remove it all together.
The  former seems better since it's a arguably more useful 

[dmurph, 2012/07/04 00:01:29]

Done.

+}  // anonymous namespace
+
+MemoryProgramCache::~MemoryProgramCache() {
+}
+
+void MemoryProgramCache::ClearBackend() {
+  curr_size_bytes_ = 0;
+  store_.clear();
+  eviction_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 sha_string(sha, kHashLength);
+
+  StoreMap::const_iterator found = store_.find(sha_string);
+  if (found == store_.end()) {
+    return false;
+  }
+  const scoped_refptr<ProgramCacheValue> value = found->second;

[greggman, 2012/06/26 23:00:27]

I think you can just use

const ProgramCacheValue* value = found->second;

or if that doesn't compile you can definitely use

const ProgramCacheValue* value = found->second.get()

There's no reason to keep a scoped_refptr unless you need to keep a reference.

[dmurph, 2012/07/04 00:01:29]

Done.

+  glProgramBinary(program,
+                  value->format,

[apatrick_chromium, 2012/06/28 21:54:31]

Is the format guaranteed to be understood by the current GL? If it's not, this
will generate GL_INVALID_ENUM which is something glLinkProgram never generates.

[dmurph, 2012/06/28 22:49:54]

Well, the format was populated by a corresponding glGetProgramBinary.  Is there
a chance that it will write a format that it can't read?

[apatrick_chromium, 2012/06/28 23:22:33]

I think only on mac, though so far I haven't found a mac that actually supports
program binary. Going forward, is this class going to get populated with program
binaries read from a file? A GL driver change could render a binary format
invalid as well.

[dmurph, 2012/07/04 00:01:29]

Just to document in the comment thread, this shouldn't be an issue for
in-memory, because we live in the lifetime of the gpu process.  Also, the cache
isn't even created if we don't have the glGetProgramBinary and glProgramBinary
calls available.

+                  static_cast<const GLvoid*>(value->data.get()),

[greggman, 2012/06/26 23:00:27]

Did the compiler complain without the cast? I would have expected cashing to
const GLvoid* requires no cast.

[dmurph, 2012/07/04 00:01:29]

Done.

+                  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 buffer_length = 0;
+  glGetProgramiv(program, GL_PROGRAM_BINARY_LENGTH_OES, &buffer_length);
+  scoped_array<char> binary(new char[buffer_length]);
+  glGetProgramBinary(program,
+                     buffer_length,
+                     &length,
+                     &format,
+                     static_cast<GLvoid*>(binary.get()));
+  if (length == 0) {
+    return;
+  }
+
+  char a_sha[kHashLength];
+  char b_sha[kHashLength];
+  ComputeShaderHash(*shader_a->source(), a_sha);
+  ComputeShaderHash(*shader_b->source(), b_sha);
+
+  scoped_refptr<ProgramCacheValue> value(
+      new ProgramCacheValue(length,
+                            format,
+                            binary.release(),
+                            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 sha_string(sha, kHashLength);
+
+  if (store_.find(sha_string) != store_.end()) {
+    return;
+  }
+
+  while (curr_size_bytes_ + length > max_size_bytes_) {
+    if (eviction_helper_.IsEmpty()) {

[greggman, 2012/06/26 23:00:27]

If length is > max_size_bytes it so that no matter what you won't put it in the
cache it seem silly to remove all the other programs from the cache

Maybe check that first, then change this check to a DCHECK?

[dmurph, 2012/07/04 00:01:29]

Done.

+      return;
+    }
+    const std::string& program = eviction_helper_.PeekKey();
+    const StoreMap::iterator found = store_.find(program);
+    const scoped_refptr<ProgramCacheValue>& evicting = found->second;

[greggman, 2012/06/26 23:00:27]

here too it's safe to just use 
const ProgramCacheValue* evicting = found->second 
as long as you don't use evicting after store_.erase(found)

[dmurph, 2012/07/04 00:01:29]

Done.

+    curr_size_bytes_ -= evicting->length;
+    Evict(program, evicting->shader_0_hash, evicting->shader_1_hash);
+    store_.erase(found);
+    eviction_helper_.PopKey();
+  }
+  store_[sha_string] = value;
+  curr_size_bytes_ += length;
+  eviction_helper_.KeyUsed(sha_string);
+  LinkedProgramCacheSuccess(sha_string,
+                            std::string(a_sha, kHashLength),
+                            std::string(b_sha, kHashLength));
+}
+

[greggman, 2012/06/26 23:00:27]

You need a memory_program_cache_unittest.cc

+}  // namespace gles2
+}  // namespace gpu