GPU Program Caching

gpu/command_buffer/service/shader_manager.h

Index: gpu/command_buffer/service/shader_manager.h
diff --git a/gpu/command_buffer/service/shader_manager.h b/gpu/command_buffer/service/shader_manager.h
index 9b6fad2ce2d7afee87fc901d37eeca31ee012887..1785e373069edae8648fc096a4e10aa1cb91d4f4 100644
--- a/gpu/command_buffer/service/shader_manager.h
+++ b/gpu/command_buffer/service/shader_manager.h
@@ -34,6 +34,10 @@ class GPU_EXPORT ShaderManager {
     typedef ShaderTranslator::VariableInfo VariableInfo;
 
     void UpdateSource(const char* source) {
+      // prevent the compiled source from getting deleted when source_ is reset

[greggman, 2012/07/16 19:41:24]

I'm confused by this implementation.

It seems like all you want is

1) When glCompileShader is called and shader compilation is put off (marked as
pending) copy source to compilation_source
2) When doing a pending compile, use compilation_source

the end. Not sure why all this other complication is needed which swapping
strings, calling release, extra checks in compilation_source or some of the
other code here. 

Am I missing something?

[dmurph, 2012/07/16 20:00:37]

I'm doing it slightly differently, and I think it ends up being much simpler
along with less memory usage.

1) Watch for the event where we update the source after we have already
compiled, and store source_ the compiled_source_ pointer so it's not destructed
when we update source_.

2) Reset compiled_source_ when we compile.

When we want to grab our compiled source, if compiled_source_ is populated then
we know to use that (source changed after compile), otherwise we use source_
(source not changed after compile)

+      if(!compilation_source_.get() && source_.get()) {

[greggman, 2012/07/16 21:57:26]

Okay then maybe a bunch of comments need to change? See other comments.

[dmurph, 2012/07/17 18:17:13]

Updated comment, simplified this statement

+        compilation_source_.reset(source_.release());
+      }
       source_.reset(source ? new std::string(source) : NULL);
       translated_source_.reset(NULL);
     }
@@ -63,6 +67,23 @@ class GPU_EXPORT ShaderManager {
         bool valid, const char* log,
         ShaderTranslatorInterface* translator);
 
+    bool pending_compilation() const {
+      return pending_cache_miss_compilation_;
+    }
+
+    // The source that was compiled for this shader

[greggman, 2012/07/16 21:57:26]

How about

// The source that was used when the user called CompileShader. 
// This is only used for a deferred compile.

And how about renaming it something like 
source_for_deferred_compile?

[dmurph, 2012/07/17 18:17:13]

How about deferred_compilation_source?

+    const std::string* compilation_source() const {
+      return compilation_source_.get() != NULL ?
+          compilation_source_.get() :
+          source_.get();
+    }
+
+    // Stores the current source as the compiled source

[greggman, 2012/07/16 21:57:26]

This comment does not match what this function does.

Also, the function name doesn't seem to describe what this function does. It
says it's Setting the source as it was compiled. But (a) it's not storing any
source and (b) there is no store on compile. There is store on deferred (or
rather store when compile is requested) but there is no store on true compile
which makes this comment confusing, at least to me. :-p

[dmurph, 2012/07/17 18:17:13]

Did some refactoring to try and help clear all of this up.  Let me know what you
think.

+    void SetSourceAsCompiled(bool pending_cache_miss_compilation) {

[greggman, 2012/07/16 21:57:26]

Also, there needs to be unit tests for these new functions in
shader_manager_unittest.cc

[dmurph, 2012/07/17 18:17:13]

Done.

+      pending_cache_miss_compilation_ = pending_cache_miss_compilation;
+      compilation_source_.reset();
+    }
+
     const VariableInfo* GetAttribInfo(const std::string& name) const;
     const VariableInfo* GetUniformInfo(const std::string& name) const;
 
@@ -87,6 +108,28 @@ class GPU_EXPORT ShaderManager {
       return use_count_ != 0;
     }
 
+    // Used by program cache.
+    const ShaderTranslator::VariableMap& attrib_map() const {
+      return attrib_map_;
+    }
+
+    // Used by program cache.
+    const ShaderTranslator::VariableMap& uniform_map() const {
+      return uniform_map_;
+    }
+
+    // Used by program cache.
+    void set_attrib_map(const ShaderTranslator::VariableMap& attrib_map) {
+      // copied because cache might be cleared
+      attrib_map_ = ShaderTranslator::VariableMap(attrib_map);
+    }
+
+    // Used by program cache.
+    void set_uniform_map(const ShaderTranslator::VariableMap& uniform_map) {
+      // copied because cache might be cleared
+      uniform_map_ = ShaderTranslator::VariableMap(uniform_map);
+    }
+
    private:
     typedef ShaderTranslator::VariableMap VariableMap;
 
@@ -122,6 +165,12 @@ class GPU_EXPORT ShaderManager {
     // The type info when the shader was last compiled.
     VariableMap attrib_map_;
     VariableMap uniform_map_;
+
+    // If compilation was delayed for possible cache hit
+    bool pending_cache_miss_compilation_;
+
+    // Holds on to the source that was supposed to be compiled

[greggman, 2012/07/16 21:57:26]

This comment makes sense but I might be better to match it if you accept the
other suggested changes to something like
// Holds on to the source for a deferred compile.

[dmurph, 2012/07/17 18:17:13]

Done.

+    scoped_ptr<std::string> compilation_source_;
   };
 
   ShaderManager();