GPU Program Caching

in-memory program cache implementation with a memory limit + lru eviction.

Wiring:
- Added bindings for glProgramBinary, glGetProgramBinary, glProgramParameteri
- Plumbed the shader cache from gl_channel_manager to program_manager
- Program cache creation after first context is created

Refactoring:
- moved DoCompile to ProgramManager

New:
- added functionality to ShaderInfo to store if we have a possible pending cache compile
- exposed attrib_map and uniform_map in ShaderInfo for the cache
- program_cache base class with in-memory status storage
- Simple memory_program_cache implementation, stores programs with lru eviction
- Added caching logic to DoCompileShader and Link in ProgramMAnager
- MemoryProgramCache, the in-memory cache implementation
- ProgramCacheLruHelper, an O(1) lru implementation

Misc:
- A couple style fixes in modified files

Design doc: https://docs.google.com/document/d/1Vceem-nF4TCICoeGSh7OMXxfGuJEJYblGXRgN9V9hcE/edit

BUG=88572
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147328

[dmurph, 2012-06-15 01:46:56]

Just getting this up here for arch discussion and to have a working shader cache
patch for those who might desperately need it.

Probably the most important classes here are gles2_cmd_decoder, program_manager,
and shader_cache.

Next step for me is mocking the shader cache for tests.

[dmurph, 2012-06-15 08:06:32]

Points:
- shader string data needs to be copied into keys used for table insertion. 
This might warrant using std::string instead of char*?
- any issues with how DoCompileShader was broken up?
- before link-time shader compilation, checks for previous compilation need to
be done.

[greggman, 2012-06-15 08:10:09]

Sorry, I didn't notice your comment about using std::string until after I made
all my comments.

Looks good in general. Don't get frustrated at all the style comment's. It takes
a while to get used to Google's strict style

You'll need to add memory_shader_cache_unittests.cc that test MemoryShaderCache,
and shader_cache_unittests.cc that test the various key classes checking that if
any one of the many fields change they don't clash.

Also unit tests in program_manager_unittest.cc that use a Mock ShaderCache and
test that the right things are called at the right times for the various cases
(not in cache, in cache, added to cache, shader compile successful, etc..)

There's another wrinkle. You need to include the attribute bindings in the
program cache key. The reason is 2 programs with different bindings generate
different binaries. In other words

glBindAttribLocation(prg, "position", 0);
glLinkProgram(prg)

is different than

glBindAttribLocation(prg, "position", 3);
glLinkProgram(prg)

Those are stored in ProgramInfo::bind_attrib_location_map_

One more thing, It seems like the MemoryShaderCache needs a memory limit,
otherwise it will just grow in size forever. I'd leave that for a separate CL
though.

That also means you'll need some MRU accounting so that when it needs to purge
programs to stay under the limit it purges the oldest ones.

[greggman, 2012-06-15 08:10:24]

https://chromiumcodereview.appspot.com/10534173/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:38: class ShaderCache;
Should these be named ProgramCache and MemoryProgramCache? If we ever decide to
share shaders or if it's ever possible to cache shaders instead of programs it
will be awkward no?

https://chromiumcodereview.appspot.com/10534173/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:146: base::WeakPtr<gpu::ShaderCache>
shader_cache_;
Does this really need to be a weak ptr? Usually we'd just use a raw* (see
gpu_channel_manager_ above) or we'd use a scoped_refptr if we needed it to be
ref counted. But I suspect under the current design the GpuChannelManager always
out lives the channels so it seems like these should just be raw pointers?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.h:155: base::WeakPtr<ShaderCache>
shader_cache_;
same here. Does this need to be a weakptr?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5656: ProgramManager* manager =
GetContextGroup()->program_manager();
you can just use program_manager();  
No need to go through GetContextGroup()

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5658: if (cache &&
If you get rid of the WeakPtr there should ways be a cache.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5659:
cache->getShaderCompilationStatus(info->source()->c_str())
style: functions start with UpperCase

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5661: info->SetStatus(true,
"possible cache hit", translator);
I don't think it's valid to set the status string on success. The user can query
this string with glGetShaderInfoLog and that string shouldn't be accessible to
the user.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5662: return;
the 6 lines above seem like they belong in ProgramManager::DoCompileShader?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/memory_shader_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.cc:30: #if defined(OS_MACOSX)
You need to call glGetString and check for GL_ARB_get_progam_binary or
GL_OES_get_program_binary to decide if you can cache or not. Some non mac
platform might nt support program binary and mac might add it.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.cc:38: const char* shader_src) {
would it be better if all of these functions took 'const std::string&
shader_src' instead of 'const char* shaders_src'? It seems like almost
everywhere you are calling it you're taking it from a std::string and then later
you're going to have to store the string requiring a strlen on a relatively long
string.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.cc:40: return
shader_status_[key];
If you are just trying to find a key you should call find. using [] to access
stl based maps inserts the key if it doesn't exist. Or was that your intent?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/memory_shader_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:12: using disk_cache::Backend;
style: not allowed to use using in a header file.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:20: : length(_length),
format(_format), data(_data) { }
style: 4 spaces in front of :

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:23: GLvoid* data;
could all of these be marked as const?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:26: class GPU_EXPORT
MemoryShaderCache : public ShaderCache {
style: classes need a command description describing what they do

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:27: public:
style: 1 space before public:

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:30: 
style: All of these function names need to start with UpperCase

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:31: bool
isShaderCacheEnabled();
can this be a const function?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:32: CompiledShaderStatus
getShaderCompilationStatus(const char* shader_src);
const func?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:36: LinkedProgramStatus
getLinkedProgramStatus(const char* untranslated_a,
const func?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:44: void getLinkedProgram(const
char* untranslated_a,
const func?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:56: private:
style: 1 space before private

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:57: 
style: no blank lines after private

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:60: hash_map<CachedShaderKey,
Do you want to typedef these types?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:72: 
style: no blank lines at the end of a class

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:73: 
You probably want a DISALLOW_COPY_AND_ASSIGN(MemoryShaderCache);

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:260: FeatureInfo::Ref
feature_info) {
feature_info should just be a raw pointer.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:361: AttachShader(manager,
attached_shaders_[0]);
Why do you have to attach here? Shouldn't they already be attached?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:392:
glGetProgramBinary(service_id(),
This code makes no sense to me. What is cache->cache_buffer for? it just a temp
buffer to store the shader for a moment when you pull it out because you then
copy it a few lines below.

I'd do this

1) You need to call 

GLsize length = 0;
glGetProgramiv(service_id(), GL_PROGRAM_BINARY_LENGTH_OES, &length) 

To find out the size of the buffer you need. As it is now if the program happesn
to be huge it will just trash memory.

Then, I'd do one of the following

Either add std::vector<uint8> binary_program_buffer_ to ShaderCache and then
resize if it's not big enough as in

if (binary_program_buffer_.size() < 
    static_cast<size_t>(length)) {
   binary_program_buffer_.resize(length);
}

That way it will only grow to the size of the largest program but will rarely be
reallocated.

Otherwise I'd find some way of making it get gl to copy the shader directly into
the place you plan to store it. Then the code wouldn't be copying it twice as as
it is now. (once from GL and again from cache_buffer)

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:400: GLvoid* trimedBinary = new
char[length];
Related to the above comment where does this memory get freed? It seems like
since it's the responsibility of the cache to free this memory it should be the
responsibility of the cache to allocate it. 

Should the caching code be calling glGetProgramIv and glGetProgramBinary as in
maybe the chunk of code after line 388 should be moved into
ShaderCache::CacheProgram(GLuint program_id, ...). CacheProgram can do the
generic thing (making the GL calls) and call some virtual functions in
MemoryShaderCache to actually store them or something. I don't want to back seat
design, just trying to throw out ideas.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.h:15: #include "base/file_path.h"
do you need file_path.h?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/shader_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:16: enum CompiledShaderStatus {
move these inside ShaderCache so they aren't in the public namespace?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:28: ShaderCache() : cache_buffer_(new
char[kShaderCacheBufferSize]) { };
how does this work???

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:61: struct CachedShaderKey {
These keys probably need a version number or the chrome link number or
something. Otherwise if you change one of these structures they'll be invalid
when loaded from the cache

On the other hand, maybe these keys should be specific to the type of cache. You
don't need gl_vendor, gl_render, vendor_id_ or driver_id in the
MemoryShaderCache

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:68: CachedShaderKey(const char*
_untranslated_src,
given the use this seems like it should be 'const& std::string' ?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:78: const char* untranslated_src;
these strings need to be std::string or something that actually stores the
string. The strings you're passing in all the way from ShaderInfo.source are not
guaranteed to stick around.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:102: CachedProgramKey(const char*
_untranslated_src_0,
same here? should it be 'const& std::string' ?

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:114: const char* untranslated_src_0;
same here. these need to be something that stores the string.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:182: 
style: remove extra blank lines

[dmurph, 2012-06-15 16:40:23]

Comments that I didn't respond to are ones that I agree with and I'll mark with
'done' as I change them/upload the patch.

https://chromiumcodereview.appspot.com/10534173/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:146: base::WeakPtr<gpu::ShaderCache>
shader_cache_;
On 2012/06/15 08:10:24, greggman wrote:
> Does this really need to be a weak ptr? Usually we'd just use a raw* (see
> gpu_channel_manager_ above) or we'd use a scoped_refptr if we needed it to be
> ref counted. But I suspect under the current design the GpuChannelManager
always
> out lives the channels so it seems like these should just be raw pointers?

Yes, but I would argue that it's
1. Better design, shows that we don't own the cache
2. The 'if (cache)' checks allow us to also define the shader cache as NULL to
disable it.
So it has the side effect of facilitating a disabled cache (if the gpu doesn't
support the right extensions) in the fastest way possible

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_group.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/context_group.h:155: base::WeakPtr<ShaderCache>
shader_cache_;
On 2012/06/15 08:10:24, greggman wrote:
> same here. Does this need to be a weakptr?

see above

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5658: if (cache &&
On 2012/06/15 08:10:24, greggman wrote:
> If you get rid of the WeakPtr there should ways be a cache.

See above comment.  This would be the fastest option for disabling the cache

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/memory_shader_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.cc:30: #if defined(OS_MACOSX)
On 2012/06/15 08:10:24, greggman wrote:
> You need to call glGetString and check for GL_ARB_get_progam_binary or
> GL_OES_get_program_binary to decide if you can cache or not. Some non mac
> platform might nt support program binary and mac might add it.

Yeah, I ended up not using this method, I'll put those checks in
gpu_channel_manager so it can have the cache be NULL if it's not supported

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.cc:38: const char* shader_src) {
On 2012/06/15 08:10:24, greggman wrote:
> would it be better if all of these functions took 'const std::string&
> shader_src' instead of 'const char* shaders_src'? It seems like almost
> everywhere you are calling it you're taking it from a std::string and then
later
> you're going to have to store the string requiring a strlen on a relatively
long
> string.

Yeah, good idea :)

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.cc:40: return
shader_status_[key];
On 2012/06/15 08:10:24, greggman wrote:
> If you are just trying to find a key you should call find. using [] to access
> stl based maps inserts the key if it doesn't exist. Or was that your intent?

It wasn't, thanks for catching this, I'm not super familiar with c++ hash_map

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:392:
glGetProgramBinary(service_id(),
On 2012/06/15 08:10:24, greggman wrote:
> This code makes no sense to me. What is cache->cache_buffer for? it just a
temp
> buffer to store the shader for a moment when you pull it out because you then
> copy it a few lines below.
> 
> I'd do this
> 
> 1) You need to call 
> 
> GLsize length = 0;
> glGetProgramiv(service_id(), GL_PROGRAM_BINARY_LENGTH_OES, &length) 
> 
> To find out the size of the buffer you need. As it is now if the program
happesn
> to be huge it will just trash memory.
> 
> Then, I'd do one of the following
> 
> Either add std::vector<uint8> binary_program_buffer_ to ShaderCache and then
> resize if it's not big enough as in
> 
> if (binary_program_buffer_.size() < 
>     static_cast<size_t>(length)) {
>    binary_program_buffer_.resize(length);
> }
> 
> That way it will only grow to the size of the largest program but will rarely
be
> reallocated.
> 
> Otherwise I'd find some way of making it get gl to copy the shader directly
into
> the place you plan to store it. Then the code wouldn't be copying it twice as
as
> it is now. (once from GL and again from cache_buffer)
> 
> 
> 
> 

Sounds good, I didn't know I could query the size of the binary, that seems like
the best approach.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:400: GLvoid* trimedBinary = new
char[length];
On 2012/06/15 08:10:24, greggman wrote:
> Related to the above comment where does this memory get freed? It seems like
> since it's the responsibility of the cache to free this memory it should be
the
> responsibility of the cache to allocate it. 
> 
> Should the caching code be calling glGetProgramIv and glGetProgramBinary as in
> maybe the chunk of code after line 388 should be moved into
> ShaderCache::CacheProgram(GLuint program_id, ...). CacheProgram can do the
> generic thing (making the GL calls) and call some virtual functions in
> MemoryShaderCache to actually store them or something. I don't want to back
seat
> design, just trying to throw out ideas.

I like that, the memory issue was bothering me yesterday evening on the way
home, definitely can be cleaned up a lot by putting all of that in the cache

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/shader_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:28: ShaderCache() : cache_buffer_(new
char[kShaderCacheBufferSize]) { };
On 2012/06/15 08:10:24, greggman wrote:
> how does this work???

Trying to find a good place for the buffer for returning the cached program,
didn't realize you could query the size.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:61: struct CachedShaderKey {
On 2012/06/15 08:10:24, greggman wrote:
> These keys probably need a version number or the chrome link number or
> something. Otherwise if you change one of these structures they'll be invalid
> when loaded from the cache
> 
> On the other hand, maybe these keys should be specific to the type of cache.
You
> don't need gl_vendor, gl_render, vendor_id_ or driver_id in the
> MemoryShaderCache
> 

Yeah, so I'm thinking the shader_cache could be non-virtual and handle all the
in-memory caching (with memory limits, etc), and leave handles for the
disk-based portion.  I would have separate keys for the hash_maps that don't
include information that can't change during the lifespan of the cache
(basically the lifespan of the gpu process)

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:78: const char* untranslated_src;
On 2012/06/15 08:10:24, greggman wrote:
> these strings need to be std::string or something that actually stores the
> string. The strings you're passing in all the way from ShaderInfo.source are
not
> guaranteed to stick around.

Yeah

[dmurph, 2012-06-19 01:08:33]

Below are my second round of just 'Done' comments, see above for others.

Patch includes basic test of the shader cache.  More tests to come, with max
memory limit.

https://chromiumcodereview.appspot.com/10534173/diff/1/content/common/gpu/gpu...
File content/common/gpu/gpu_channel.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/content/common/gpu/gpu...
content/common/gpu/gpu_channel.h:38: class ShaderCache;
On 2012/06/15 08:10:24, greggman wrote:
> Should these be named ProgramCache and MemoryProgramCache? If we ever decide
to
> share shaders or if it's ever possible to cache shaders instead of programs it
> will be awkward no?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5656: ProgramManager* manager =
GetContextGroup()->program_manager();
On 2012/06/15 08:10:24, greggman wrote:
> you can just use program_manager();  
> No need to go through GetContextGroup()

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5661: info->SetStatus(true,
"possible cache hit", translator);
On 2012/06/15 08:10:24, greggman wrote:
> I don't think it's valid to set the status string on success. The user can
query
> this string with glGetShaderInfoLog and that string shouldn't be accessible to
> the user.

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:5662: return;
On 2012/06/15 08:10:24, greggman wrote:
> the 6 lines above seem like they belong in ProgramManager::DoCompileShader?

Well, we need separation between using the cache and actually compiling the
shader, so the Link() method can force compile the shaders if it needs to.  So
the program manager's DoCompileShader doesn't use the cache.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/memory_shader_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:12: using disk_cache::Backend;
On 2012/06/15 08:10:24, greggman wrote:
> style: not allowed to use using in a header file.

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:20: : length(_length),
format(_format), data(_data) { }
On 2012/06/15 08:10:24, greggman wrote:
> style: 4 spaces in front of :

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:23: GLvoid* data;
On 2012/06/15 08:10:24, greggman wrote:
> could all of these be marked as const?

It looks like I need assignment working for hash_map

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:26: class GPU_EXPORT
MemoryShaderCache : public ShaderCache {
On 2012/06/15 08:10:24, greggman wrote:
> style: classes need a command description describing what they do

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:27: public:
On 2012/06/15 08:10:24, greggman wrote:
> style: 1 space before public:

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:30: 
On 2012/06/15 08:10:24, greggman wrote:
> style: All of these function names need to start with UpperCase

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:31: bool
isShaderCacheEnabled();
On 2012/06/15 08:10:24, greggman wrote:
> can this be a const function? 

gone

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:32: CompiledShaderStatus
getShaderCompilationStatus(const char* shader_src);
On 2012/06/15 08:10:24, greggman wrote:
> const func?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:36: LinkedProgramStatus
getLinkedProgramStatus(const char* untranslated_a,
On 2012/06/15 08:10:24, greggman wrote:
> const func?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:44: void getLinkedProgram(const
char* untranslated_a,
On 2012/06/15 08:10:24, greggman wrote:
> const func?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:56: private:
On 2012/06/15 08:10:24, greggman wrote:
> style: 1 space before private

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:57: 
On 2012/06/15 08:10:24, greggman wrote:
> style: no blank lines after private

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:60: hash_map<CachedShaderKey,
On 2012/06/15 08:10:24, greggman wrote:
> Do you want to typedef these types?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:72: 
On 2012/06/15 08:10:24, greggman wrote:
> style: no blank lines at the end of a class

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_shader_cache.h:73: 
On 2012/06/15 08:10:24, greggman wrote:
> You probably want a DISALLOW_COPY_AND_ASSIGN(MemoryShaderCache);

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:260: FeatureInfo::Ref
feature_info) {
On 2012/06/15 08:10:24, greggman wrote:
> feature_info should just be a raw pointer.

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.cc:361: AttachShader(manager,
attached_shaders_[0]);
On 2012/06/15 08:10:24, greggman wrote:
> Why do you have to attach here? Shouldn't they already be attached?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/program_manager.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/program_manager.h:15: #include "base/file_path.h"
On 2012/06/15 08:10:24, greggman wrote:
> do you need file_path.h?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
File gpu/command_buffer/service/shader_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:16: enum CompiledShaderStatus {
On 2012/06/15 08:10:24, greggman wrote:
> move these inside ShaderCache so they aren't in the public namespace?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:68: CachedShaderKey(const char*
_untranslated_src,
On 2012/06/15 08:10:24, greggman wrote:
> given the use this seems like it should be 'const& std::string' ?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:102: CachedProgramKey(const char*
_untranslated_src_0,
On 2012/06/15 08:10:24, greggman wrote:
> same here? should it be 'const& std::string' ?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:114: const char* untranslated_src_0;
On 2012/06/15 08:10:24, greggman wrote:
> same here. these need to be something that stores the string. 

Done.

https://chromiumcodereview.appspot.com/10534173/diff/1/gpu/command_buffer/ser...
gpu/command_buffer/service/shader_cache.h:182: 
On 2012/06/15 08:10:24, greggman wrote:
> style: remove extra blank lines

Done.

[greggman, 2012-06-19 22:27:50]

I let a couple of comments but I'm just going to come over.

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:15: typedef
std::map<std::string, GLint> location_map;
style: types are CamelCase

Also, this should be in an anonymous namespace since as is makes it a global
type in gles2's namespace.

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:25: store_.erase(key);
I don't think you can delete things like this from a map while iterating?

You might try

while (!store_.empty()) {
   it = store_.begin();

Or this might work

for (it = store_.begin(), it != store.end();)
{
    it = store_.erase(key);
}

Though I don't think I've removed things from a hash_map before.

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.h (right):

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:20: GLvoid* _data,
const?

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:21:
ShaderTranslator::VariableMap& _attrib_map_0,
const?

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:47: CachedProgramKeyEquals>
store_map;
style: types are CamelCase

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:59: ShaderManager::ShaderInfo*
shader_a,
const?

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:61: std::map<std::string,
GLint>* bind_attrib_location_map);
const?

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache.h (right):

http://codereview.chromium.org/10534173/diff/11012/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:36: const unsigned char*
hashed_src_0;
what's the point in these being char* instead of std::string?

Also what's the point of having this at all? If the keys are SHA1s then
CachedProgramKey should just be std::string.


You'd make the key by copying all the data to a buffer and running SHA1 at the
buffer at the time you need the key.

size_t buffer_size = ComputeSizeNeededForKeyData(src0, src1, location_map,
etc...);
scoped_array(new uint8[buffer_size]);

Then fill out the buffer

uint8* p = buffer.get();
memcpy(p, src0.c_str(), src0.size());
p += src0.size();
p++ = '\0';
memcpy(p, src1.c_str(), src1.size());
p += src1.size();
p++ = '\0';
etc.

then finally 

unsigned char key_buffer[kSHA1Length]
base::SHA1HashBytes(buffer_.get(), buffer_size, key_buffer);
std::string key(key_buffer, key_buffer + kSHA1Length);

[dmurph, 2012-06-23 01:37:28]

New patch has more unit tests and supports a memory limit with lru eviction. 
Other misc cleanup and streamlining with the interface.

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:15: typedef
std::map<std::string, GLint> location_map;
On 2012/06/19 22:27:50, greggman wrote:
> style: types are CamelCase
> 
> Also, this should be in an anonymous namespace since as is makes it a global
> type in gles2's namespace.

Done.

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:25: store_.erase(key);
On 2012/06/19 22:27:50, greggman wrote:
> I don't think you can delete things like this from a map while iterating?
> 
> You might try
> 
> while (!store_.empty()) {
>    it = store_.begin();
> 
> Or this might work
> 
> for (it = store_.begin(), it != store.end();)
> {
>     it = store_.erase(key);
> }
> 
> Though I don't think I've removed things from a hash_map before.

This way works correctly.  Although I'm removing the erase, they should be
deleted anyways by the destructor of the hash_map

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.h:20: GLvoid* _data,
On 2012/06/19 22:27:50, greggman wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.h:21:
ShaderTranslator::VariableMap& _attrib_map_0,
On 2012/06/19 22:27:50, greggman wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.h:47: CachedProgramKeyEquals>
store_map;
On 2012/06/19 22:27:50, greggman wrote:
> style: types are CamelCase

Done.

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.h:59: ShaderManager::ShaderInfo*
shader_a,
On 2012/06/19 22:27:50, greggman wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.h:61: std::map<std::string,
GLint>* bind_attrib_location_map);
On 2012/06/19 22:27:50, greggman wrote:
> const?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
File gpu/command_buffer/service/program_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/11012/gpu/command_buffer...
gpu/command_buffer/service/program_cache.h:36: const unsigned char*
hashed_src_0;
On 2012/06/19 22:27:50, greggman wrote:
> what's the point in these being char* instead of std::string?
> 
> Also what's the point of having this at all? If the keys are SHA1s then
> CachedProgramKey should just be std::string.
> 
> 
> You'd make the key by copying all the data to a buffer and running SHA1 at the
> buffer at the time you need the key.
> 
> size_t buffer_size = ComputeSizeNeededForKeyData(src0, src1, location_map,
> etc...);
> scoped_array(new uint8[buffer_size]);
> 
> Then fill out the buffer
> 
> uint8* p = buffer.get();
> memcpy(p, src0.c_str(), src0.size());
> p += src0.size();
> p++ = '\0';
> memcpy(p, src1.c_str(), src1.size());
> p += src1.size();
> p++ = '\0';
> etc.
> 
> then finally 
> 
> unsigned char key_buffer[kSHA1Length]
> base::SHA1HashBytes(buffer_.get(), buffer_size, key_buffer);
> std::string key(key_buffer, key_buffer + kSHA1Length);
> 
> 

Done.

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

here's a few comments. I didn't review everything yet.

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
File gpu/command_buffer/service/context_group.h (right):

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/context_group.h:14: #include "base/memory/weak_ptr.h"
Is this needed?

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:71: GLvoid* binary = new
char[bufferlength];
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.

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:111: std::string program =
lru_helper_.EvictKey();
could this be const str::string& program? Then no copy will be made

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:112: StoreValue evictedValue
= store_[program];
style: variables use under_score

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:112: StoreValue evictedValue
= store_[program];
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.

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:145: buffer.get()[currPos++] =
(char) (value >> 12);
Is there a reason you're doing 4 bits at a time instead of 8?

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache.h (right):

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:12: #include "base/memory/weak_ptr.h"
is this still needed?

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:19: typedef std::map<std::string,
GLint> BindAttribMap;
I don't think it makes sense to have an anonymous namespace in a header. I
probably didn't notice this was a header before.

Should be moved this into ProgramCache?

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:64: static const uint32 kHashLength =
base::kSHA1Length;
Is there a reason to use uint32 everywhere? the style guide prefers 'size_t'
where appropriate and 'int' otherwise.

uint32 should only be used if it absolutely needs to be uint32 (4 bytes)

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:95: struct CompiledShaderInfo {
We don't usually allow structs to have methods that do anything. Should be a
class?

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:101: void usedInCachedProgram() {
If you're going to have a function to inc ref count should you have one to dec
ref count?

Better yet, DecerementRefCount() or UnuseInCachedProgram()  can do a
DCHECK_NE(0, ref_count); and ref_count_ can be private.

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:117: virtual void ClearBackend() = 0;
There's no such thing as a pure virtual private method is there?

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_lru_helper.h (right):

http://codereview.chromium.org/10534173/diff/18001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper.h:35: struct FastHash {
do you really need FastHash?

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

Style fixes + cleaner memory management

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
File gpu/command_buffer/service/context_group.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/context_group.h:14: #include "base/memory/weak_ptr.h"
On 2012/06/25 18:53:03, greggman wrote:
> Is this needed?
Nope, thought I had caught all of these haha.

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:71: GLvoid* binary = new
char[bufferlength];
On 2012/06/25 18:53:03, greggman wrote:
> 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.
> 
> 

Done.

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:111: std::string program =
lru_helper_.EvictKey();
On 2012/06/25 18:53:03, greggman wrote:
> could this be const str::string& program? Then no copy will be made

Done.

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:112: StoreValue evictedValue
= store_[program];
On 2012/06/25 18:53:03, greggman wrote:
> style: variables use under_score

Done.

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
File gpu/command_buffer/service/program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.cc:145: buffer.get()[currPos++] =
(char) (value >> 12);
On 2012/06/25 18:53:03, greggman wrote:
> Is there a reason you're doing 4 bits at a time instead of 8?

whoops

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
File gpu/command_buffer/service/program_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.h:12: #include "base/memory/weak_ptr.h"
On 2012/06/25 18:53:03, greggman wrote:
> is this still needed?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.h:19: typedef std::map<std::string,
GLint> BindAttribMap;
On 2012/06/25 18:53:03, greggman wrote:
> I don't think it makes sense to have an anonymous namespace in a header. I
> probably didn't notice this was a header before.
> 
> Should be moved this into ProgramCache?

It's just to shorten arguments for the map, I'll just expand them

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.h:64: static const uint32 kHashLength =
base::kSHA1Length;
On 2012/06/25 18:53:03, greggman wrote:
> Is there a reason to use uint32 everywhere? the style guide prefers 'size_t'
> where appropriate and 'int' otherwise.
> 
> uint32 should only be used if it absolutely needs to be uint32 (4 bytes)

Done.

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.h:95: struct CompiledShaderInfo {
On 2012/06/25 18:53:03, greggman wrote:
> We don't usually allow structs to have methods that do anything. Should be a
> class?

I'll remove the method

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.h:101: void usedInCachedProgram() {
On 2012/06/25 18:53:03, greggman wrote:
> If you're going to have a function to inc ref count should you have one to dec
> ref count?
> 
> Better yet, DecerementRefCount() or UnuseInCachedProgram()  can do a
> DCHECK_NE(0, ref_count); and ref_count_ can be private.

Ends up not being needed, that check is included on eviction

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.h:117: virtual void ClearBackend() = 0;
On 2012/06/25 18:53:03, greggman wrote:
> There's no such thing as a pure virtual private method is there?

Yeah, it's so the derived class can provide the implementation, but they can't
call the method.
http://stackoverflow.com/questions/2170688/private-virtual-method-in-c

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
File gpu/command_buffer/service/program_cache_lru_helper.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/18001/gpu/command_buffer...
gpu/command_buffer/service/program_cache_lru_helper.h:35: struct FastHash {
On 2012/06/25 18:53:03, greggman wrote:
> do you really need FastHash?
Hell yeah!
http://www.azillionmonkeys.com/qed/hash.html

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

It's looking pretty good. A few issues left.

http://codereview.chromium.org/10534173/diff/25001/content/common/gpu/gpu_cha...
File content/common/gpu/gpu_channel_manager.h (right):

http://codereview.chromium.org/10534173/diff/25001/content/common/gpu/gpu_cha...
content/common/gpu/gpu_channel_manager.h:19: #include
"gpu/command_buffer/service/program_cache.h"
forward declare instead?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:16: typedef
std::map<std::string, GLint> BindAttribMap;
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

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:49: const
scoped_refptr<ProgramCacheValue> value = found->second;
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.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:52: static_cast<const
GLvoid*>(value->data.get()),
Did the compiler complain without the cast? I would have expected cashing to
const GLvoid* requires no cast.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:107: if
(eviction_helper_.IsEmpty()) {
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?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:112: const
scoped_refptr<ProgramCacheValue>& evicting = found->second;
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)

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:125: 
You need a memory_program_cache_unittest.cc

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:12: #include
"base/memory/scoped_ptr.h"
style: alphabetize includes

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:31: bool LoadLinkedProgram(
style: need 'virtual' in front and OVERRIDE in back.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:36: void SaveLinkedProgram(
style: need 'virtual' in front and OVERRIDE in back.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:43: void ClearBackend();
style: need 'virtual' in front and OVERRIDE in back.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:48: GLenum _format,
style: indenting

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:65: const GLsizei length;
style: public members in classes are not allowed. Either add accessors or make
it a struct. It's fine to make this a struct AFAICT

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:81: FastHash> StoreMap;
I don't think you need FashHash. You're hashing std::string. If you FastHash is
so awesome for strings then add it to base in another CL so it speeds up all of
Chrome instead of just this one rarely used place.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:87: if (--info0.ref_count <= 0) {
Is it an error if this goes < 0?  If so add a DCHECK for that case?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:124: scoped_array<char> buffer(new
char[total_size]);
if you change this to unsigned char do the casts at 144 go away?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:14: #include
"gpu/command_buffer/common/gles2_cmd_format.h"
Is this needed?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:117: 
style: remove extra blank lines

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_lru_helper.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper.cc:30: static const
std::string emptyQueueResponse = "";
style: variables use under_score

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_lru_helper.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper.h:37: struct FastHash {
again with the FastHash, there's no need to go nuts here. If FastHash is a sure
win for std::string it should be added to base. No need to over optimize here.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_lru_helper_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper_unittest.cc:35:
ASSERT_EQ("1", key);
All these ASSERT_XX should be EXPECT_XX

ASSERT_XX is only used in tests when the test would crash otherwise.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper_unittest.cc:70:
ASSERT_EQ("4", lru_helper_->PeekKey());
same check twice?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:11: typedef
std::map<std::string, GLint> LocationMap;
sorry to harp on this but just to make it clear. It seems like this should go in
ProgramCache as in

class ProgramCache {
 public:
  typedef std::map<std::string, GLint> LocationMap;
..
}

It's part of it's interface so it needs to be defined in one place so that all
the overriding functions get the same signature everywhere.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:48: std::string(a_sha,
kHashLength),
style: indenting

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:67: void evict(const
std::string& program_hash,
style: functions are UpperCase

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:73: const uint32
hash_length() const {
Is there a reason have this vs just using ProgramCache::kHashLength everywhere?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:89: {
what's this extra code block for?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:94: shader.clear();
why are you clearing this string?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:104: {
same as above. What's the extra code block for and why clear strings?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:105: std::string shaderA =
shader1;
style: variables are under_score

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:126: char
a_sha[kHashLength];
dynamically sized c arrays are not standard c. This problem goes away if you
just use ProgramCache::kHashLength

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:133: b_sha,
style: indenting

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:137: std::string(a_sha,
kHashLength),
style: indenting

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:12: #include "base/bind.h"
is this include needed?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:17: #include "net/base/net_log.h"
is this include needed?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:300: if
(!feature_info->feature_flags().angle_translated_shader_source)
ugh: please add a new issue. We need to either cache the translated shader
source or call compile lazily 
so that calling glGetTranslatedShaderSourceANGLE works for programs loaded from
the cache. We can probably do that in another CL though.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:321:
glGetShaderiv(info->service_id(),  GL_COMPILE_STATUS, &status);
style: I'm sure this was just copied and passed but only 1 space before
GL_COMPILE_STATUS

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:365: *shader_a,
style: indenting

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:382:
manager_->DoCompileShader(attached_shaders_[0],
this code is repeated pretty much repeated twice. It seems like it should be a
function or a loop.

Also, a CHECK() for success would be good because we are assuming it here.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.h:15: #include
"gpu/command_buffer/service/feature_info.h"
forward declare instead?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.h:17: #include
"gpu/command_buffer/service/program_cache.h"
forward declare instead?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.h:327: GLint SwizzleLocation(GLint
unswizzled_location) const;
These changed. You probably need to rebase.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:1056: 
You need to add a bunch of unit tests here using a MockProgramCache using
googlemock.

Basically you make a MockProgramCache

class MockProgramCache : public ProgramCache {
 public:
  ...
  MOCK_CONST_MEHTOD4(LoadLinkedProgram, bool(
      const GLuint program,
      ShaderManager::ShaderInfo* shader_a,
      ShaderManager::ShaderInfo* shader_b,
      const std::map<std::string, GLint>*
         bind_attrib_location_map));
   ...
   etc...
)

And then write tests for DoCompileShader  and Link with expectations like

EXPECT_CALL(program_manager_, 
    LoadLinkedProgram(
        service_id, 
        untranslated_source_1,
        untranslated_source_2,
        etc..))
    .WillOnce(Return(true));

for all the various cases to see that ProgramManager calls ProgramCache as
expected on success and failure of both gl and when programcache returns true vs
false etc..

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/shader_manager.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:66: void SetPendingCompilation(bool
pending_cache_miss_compilation);
style: if this is just a simple setter it's named with lower_case_under_score as
in 'set_pending_compliation'

http://codereview.chromium.org/10534173/diff/25001/gpu/demos/framework/window.cc
File gpu/demos/framework/window.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/demos/framework/window...
gpu/demos/framework/window.cc:32: 
no need to add this line?

[dmurph, 2012-06-27 01:46:22]

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:81: FastHash> StoreMap;
On 2012/06/26 23:00:27, greggman wrote:
> I don't think you need FashHash. You're hashing std::string. If you FastHash
is
> so awesome for strings then add it to base in another CL so it speeds up all
of
> Chrome instead of just this one rarely used place. 

So SuperFastHash is defined in the disk cache, should I move the implementation
from the disk cache into base?  I'll just remove that from this cl

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

https://chromiumcodereview.appspot.com/10534173/diff/25001/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/25001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:51: value->format,
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]

https://chromiumcodereview.appspot.com/10534173/diff/25001/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/25001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:51: value->format,
On 2012/06/28 21:54:31, apatrick_chromium wrote:
> 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.

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]

https://chromiumcodereview.appspot.com/10534173/diff/25001/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/25001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:51: value->format,
On 2012/06/28 22:49:54, dmurph wrote:
> On 2012/06/28 21:54:31, apatrick_chromium wrote:
> > 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.
> 
> Well, the format was populated by a corresponding glGetProgramBinary.  Is
there
> a chance that it will write a format that it can't read?

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-06-28 23:57:56]

On 2012/06/28 23:22:33, apatrick_chromium wrote:
>
https://chromiumcodereview.appspot.com/10534173/diff/25001/gpu/command_buffer...
> File gpu/command_buffer/service/memory_program_cache.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10534173/diff/25001/gpu/command_buffer...
> gpu/command_buffer/service/memory_program_cache.cc:51: value->format,
> On 2012/06/28 22:49:54, dmurph wrote:
> > On 2012/06/28 21:54:31, apatrick_chromium wrote:
> > > 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.
> > 
> > Well, the format was populated by a corresponding glGetProgramBinary.  Is
> there
> > a chance that it will write a format that it can't read?
> 
> 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.

When things are saved/loaded from the disk, the
* glGetString(GL_VENDOR);
* glGetString(GL_RENDERER);
* driver version ID
* vendor ID
* possibly chrome version (or some content version that we have access to in
content/)
will all be included, so if they don't match they won't be loaded from the disk.

The in-memory cache assumes that those values will not change during the
lifetime of the gpu process, as it'll be destructed with the
gpu_channel_manager.

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

Didn't have time to upload the patch, done except for adding program manager
tests.

http://codereview.chromium.org/10534173/diff/25001/content/common/gpu/gpu_cha...
File content/common/gpu/gpu_channel_manager.h (right):

http://codereview.chromium.org/10534173/diff/25001/content/common/gpu/gpu_cha...
content/common/gpu/gpu_channel_manager.h:19: #include
"gpu/command_buffer/service/program_cache.h"
On 2012/06/26 23:00:27, greggman wrote:
> forward declare instead?

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:16: typedef
std::map<std::string, GLint> BindAttribMap;
On 2012/06/26 23:00:27, greggman wrote:
> 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 

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:49: const
scoped_refptr<ProgramCacheValue> value = found->second;
On 2012/06/26 23:00:27, greggman wrote:
> 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.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:51: value->format,
On 2012/06/28 23:22:33, apatrick_chromium wrote:
> On 2012/06/28 22:49:54, dmurph wrote:
> > On 2012/06/28 21:54:31, apatrick_chromium wrote:
> > > 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.
> > 
> > Well, the format was populated by a corresponding glGetProgramBinary.  Is
> there
> > a chance that it will write a format that it can't read?
> 
> 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.

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.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:52: static_cast<const
GLvoid*>(value->data.get()),
On 2012/06/26 23:00:27, greggman wrote:
> Did the compiler complain without the cast? I would have expected cashing to
> const GLvoid* requires no cast.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:107: if
(eviction_helper_.IsEmpty()) {
On 2012/06/26 23:00:27, greggman wrote:
> 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?

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:112: const
scoped_refptr<ProgramCacheValue>& evicting = found->second;
On 2012/06/26 23:00:27, greggman wrote:
> 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)

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:12: #include
"base/memory/scoped_ptr.h"
On 2012/06/26 23:00:27, greggman wrote:
> style: alphabetize includes

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:31: bool LoadLinkedProgram(
On 2012/06/26 23:00:27, greggman wrote:
> style: need 'virtual' in front and OVERRIDE in back.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:36: void SaveLinkedProgram(
On 2012/06/26 23:00:27, greggman wrote:
> style: need 'virtual' in front and OVERRIDE in back.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:43: void ClearBackend();
On 2012/06/26 23:00:27, greggman wrote:
> style: need 'virtual' in front and OVERRIDE in back.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:48: GLenum _format,
On 2012/06/26 23:00:27, greggman wrote:
> style: indenting

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:65: const GLsizei length;
On 2012/06/26 23:00:27, greggman wrote:
> style: public members in classes are not allowed. Either add accessors or make
> it a struct. It's fine to make this a struct AFAICT

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:87: if (--info0.ref_count <= 0) {
On 2012/06/26 23:00:27, greggman wrote:
> Is it an error if this goes < 0?  If so add a DCHECK for that case?

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:124: scoped_array<char> buffer(new
char[total_size]);
On 2012/06/26 23:00:27, greggman wrote:
> if you change this to unsigned char do the casts at 144 go away?

Yeah, done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:14: #include
"gpu/command_buffer/common/gles2_cmd_format.h"
On 2012/06/26 23:00:27, greggman wrote:
> Is this needed?

I need the definition of GLuint, should I use a different file?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.h:117: 
On 2012/06/26 23:00:27, greggman wrote:
> style: remove extra blank lines

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_lru_helper.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper.cc:30: static const
std::string emptyQueueResponse = "";
On 2012/06/26 23:00:27, greggman wrote:
> style: variables use under_score

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_lru_helper.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper.h:37: struct FastHash {
On 2012/06/26 23:00:27, greggman wrote:
> again with the FastHash, there's no need to go nuts here. If FastHash is a
sure
> win for std::string it should be added to base. No need to over optimize here.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_lru_helper_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper_unittest.cc:35:
ASSERT_EQ("1", key);
On 2012/06/26 23:00:27, greggman wrote:
> All these ASSERT_XX should be EXPECT_XX
> 
> ASSERT_XX is only used in tests when the test would crash otherwise.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper_unittest.cc:70:
ASSERT_EQ("4", lru_helper_->PeekKey());
On 2012/06/26 23:00:27, greggman wrote:
> same check twice?

Yeah, checking that it didn't pop

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:11: typedef
std::map<std::string, GLint> LocationMap;
On 2012/06/26 23:00:27, greggman wrote:
> sorry to harp on this but just to make it clear. It seems like this should go
in
> ProgramCache as in
> 
> class ProgramCache {
>  public:
>   typedef std::map<std::string, GLint> LocationMap;
> ..
> }
> 
> It's part of it's interface so it needs to be defined in one place so that all
> the overriding functions get the same signature everywhere.
> 

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:48: std::string(a_sha,
kHashLength),
On 2012/06/26 23:00:27, greggman wrote:
> style: indenting

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:73: const uint32
hash_length() const {
On 2012/06/26 23:00:27, greggman wrote:
> Is there a reason have this vs just using ProgramCache::kHashLength
everywhere? 

it was because I has kHashLength as a protected field, but I'll just make it
public

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:89: {
On 2012/06/26 23:00:27, greggman wrote:
> what's this extra code block for?

So the strings given will be destructed, to make sure the cache is doing a copy
or hash

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:94: shader.clear();
On 2012/06/26 23:00:27, greggman wrote:
> why are you clearing this string?

same.  I can get rid of these checks if they seem overdone

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:126: char
a_sha[kHashLength];
On 2012/06/26 23:00:27, greggman wrote:
> dynamically sized c arrays are not standard c. This problem goes away if you
> just use ProgramCache::kHashLength

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:133: b_sha,
On 2012/06/26 23:00:27, greggman wrote:
> style: indenting

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:137: std::string(a_sha,
kHashLength),
On 2012/06/26 23:00:27, greggman wrote:
> style: indenting

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:300: if
(!feature_info->feature_flags().angle_translated_shader_source)
On 2012/06/26 23:00:27, greggman wrote:
> ugh: please add a new issue. We need to either cache the translated shader
> source or call compile lazily 
> so that calling glGetTranslatedShaderSourceANGLE works for programs loaded
from
> the cache. We can probably do that in another CL though.

Hm, okay then I'll leave this alone?  Should the issue say "cache the translated
shader source or call compile lazily"?

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:321:
glGetShaderiv(info->service_id(),  GL_COMPILE_STATUS, &status);
On 2012/06/26 23:00:27, greggman wrote:
> style: I'm sure this was just copied and passed but only 1 space before
> GL_COMPILE_STATUS

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:365: *shader_a,
On 2012/06/26 23:00:27, greggman wrote:
> style: indenting

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:382:
manager_->DoCompileShader(attached_shaders_[0],
On 2012/06/26 23:00:27, greggman wrote:
> this code is repeated pretty much repeated twice. It seems like it should be a
> function or a loop.
> 
> Also, a CHECK() for success would be good because we are assuming it here.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.h:15: #include
"gpu/command_buffer/service/feature_info.h"
On 2012/06/26 23:00:27, greggman wrote:
> forward declare instead?

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.h:17: #include
"gpu/command_buffer/service/program_cache.h"
On 2012/06/26 23:00:27, greggman wrote:
> forward declare instead?

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.h:327: GLint SwizzleLocation(GLint
unswizzled_location) const;
On 2012/06/26 23:00:27, greggman wrote:
> These changed. You probably need to rebase.

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
File gpu/command_buffer/service/shader_manager.h (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:66: void SetPendingCompilation(bool
pending_cache_miss_compilation);
On 2012/06/26 23:00:27, greggman wrote:
> style: if this is just a simple setter it's named with lower_case_under_score
as
> in 'set_pending_compliation'

Done.

http://codereview.chromium.org/10534173/diff/25001/gpu/demos/framework/window.cc
File gpu/demos/framework/window.cc (right):

http://codereview.chromium.org/10534173/diff/25001/gpu/demos/framework/window...
gpu/demos/framework/window.cc:32: 
On 2012/06/26 23:00:27, greggman wrote:
> no need to add this line?

Done.

[dmurph, 2012-07-10 00:20:47]

Added many tests to program manager, fixed all lint issues in changed files.

[greggman, 2012-07-10 21:43:30]

Looks good. A few style issues and a couple of issues

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:32:
MemoryProgramCache::MemoryProgramCache() : max_size_bytes_(GetCacheSize()),
style: If they can't fit one one line the style is

ClassName::ClassName()
    : field1_(...),
      field2_(...) {
}

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:116: const std::string
sha_string(sha, kHashLength);
how about
const std::string sha_string(sha, sizeof(sha));
?

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.h (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.h:24: static const size_t
kDefaultMaxProgramCacheMemoryBytes = 6*1024*1024;
style: spaces around operators

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache_unittest.cc:22: // custom mock
interface due to limitations of gmock with void*
What is the limitation of gmock with void*?

You can use Invoke. As one solution see gles2_cmd_decoder_unittest.cc:2322

There are other ways to handle void* as well with gmock

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache_unittest.cc:26: GLsizei bufSize,
style: arguments are under_score

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache_unittest.cc:28: GLenum*
binaryFormat,
style: arguments are under_score

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache_unittest.cc:159:
gl_->ExpectAndPopulateProgramBinaryCalls(kProgramId,
Is seems better to use gmock for all this?

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache_unittest.cc:212: }
Should there be a checks that
*) The binary saved is the binary returned
*) A different source does not return the same program
*) A different attrib mapping does not return the same program
?

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:97: char* result) const {
style: indent

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:123: size_t calculateMapSize(const
std::map<std::string, GLint>* map) {
style: function names start with CapitalLetters

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:148:
memcpy(&buffer.get()[shader0_size], hashed_shader_1, shader1_size);
&buffer[shader0_size]?

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache.cc:160: buffer.get()[current_pos++] =
static_cast<unsigned char>(value >> 24);
buffer[current_pos++] ?

Also I think I asked before but are these casts needed? Are you getting a
warning or something that requires these casts? They'll be cast automatically.

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_lru_helper.cc (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_lru_helper.cc:30: static const
std::string empty_queue_response = "";
statically initialized classes are not allowed. Maybe remove 'static' and put
inside the if? Honestly though I think you can just use 

  return "";  // empty queue response.

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_cache_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:16: const GLuint program,
style: for unused parameters the format is

returntype FunctionName(
   type /* argument1_name */,
   type /* argument2_name */);

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:22: virtual void
SaveLinkedProgram(
see above

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:31: const std::string&
shader2,
style: indent

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:126: b_sha,
style: indent

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_cache_unittest.cc:130: std::string(a_sha,
ProgramCache::kHashLength),
style: indent

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager.cc (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:387: ==
ProgramCache::COMPILATION_SUCCEEDED) {
style: operators go at end of previous line

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:501: AttachShader(manager, info);
You shouldn't need go call AttachShader nor glAttachShader here. The user
already attached the shaders.

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:503:
info->set_pending_compilation(false);
Should ForceCompileShader set set_pending_compilation to false?

Seems like there's a chance for double compilation

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:1162:
.WillOnce(Return(result));
style: indent

[dmurph, 2012-07-11 23:32:51]

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:32:
MemoryProgramCache::MemoryProgramCache() : max_size_bytes_(GetCacheSize()),
On 2012/07/10 21:43:30, greggman wrote:
> style: If they can't fit one one line the style is
> 
> ClassName::ClassName()
>     : field1_(...),
>       field2_(...) {
> }

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:116: const std::string
sha_string(sha, kHashLength);
On 2012/07/10 21:43:30, greggman wrote:
> how about
> const std::string sha_string(sha, sizeof(sha));
> ?

Sure, done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.h:24: static const size_t
kDefaultMaxProgramCacheMemoryBytes = 6*1024*1024;
On 2012/07/10 21:43:30, greggman wrote:
> style: spaces around operators

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache_unittest.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache_unittest.cc:22: // custom mock
interface due to limitations of gmock with void*
On 2012/07/10 21:43:30, greggman wrote:
> What is the limitation of gmock with void*?
> 
> You can use Invoke. As one solution see gles2_cmd_decoder_unittest.cc:2322
> 
> There are other ways to handle void* as well with gmock

Awesome, I was digging around forever to figure out how to do this, thanks. 
Much cleaner.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache_unittest.cc:26: GLsizei bufSize,
On 2012/07/10 21:43:30, greggman wrote:
> style: arguments are under_score

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache_unittest.cc:28: GLenum*
binaryFormat,
On 2012/07/10 21:43:30, greggman wrote:
> style: arguments are under_score

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache_unittest.cc:159:
gl_->ExpectAndPopulateProgramBinaryCalls(kProgramId,
On 2012/07/10 21:43:30, greggman wrote:
> Is seems better to use gmock for all this?

Definitely.  Thanks for the tip above.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache_unittest.cc:212: }
On 2012/07/10 21:43:30, greggman wrote:
> Should there be a checks that
> *) The binary saved is the binary returned
> *) A different source does not return the same program
> *) A different attrib mapping does not return the same program
> ?

Implemented.  Also, I list all of my tests in the design doc.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
File gpu/command_buffer/service/program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.cc:97: char* result) const {
On 2012/07/10 21:43:30, greggman wrote:
> style: indent

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.cc:123: size_t calculateMapSize(const
std::map<std::string, GLint>* map) {
On 2012/07/10 21:43:30, greggman wrote:
> style: function names start with CapitalLetters

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.cc:148:
memcpy(&buffer.get()[shader0_size], hashed_shader_1, shader1_size);
On 2012/07/10 21:43:30, greggman wrote:
> &buffer[shader0_size]?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache.cc:160: buffer.get()[current_pos++] =
static_cast<unsigned char>(value >> 24);
On 2012/07/10 21:43:30, greggman wrote:
> buffer[current_pos++] ?
> 
> Also I think I asked before but are these casts needed? Are you getting a
> warning or something that requires these casts? They'll be cast automatically.

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
File gpu/command_buffer/service/program_cache_lru_helper.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache_lru_helper.cc:30: static const
std::string empty_queue_response = "";
On 2012/07/10 21:43:30, greggman wrote:
> statically initialized classes are not allowed. Maybe remove 'static' and put
> inside the if? Honestly though I think you can just use 
> 
>   return "";  // empty queue response.
> 

Since I'm returning a reference, I'm pretty sure that will be bad.  I'll change
this to returning a pointer, so I can return NULL

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
File gpu/command_buffer/service/program_cache_unittest.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache_unittest.cc:16: const GLuint program,
On 2012/07/10 21:43:30, greggman wrote:
> style: for unused parameters the format is
> 
> returntype FunctionName(
>    type /* argument1_name */,
>    type /* argument2_name */);

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache_unittest.cc:22: virtual void
SaveLinkedProgram(
On 2012/07/10 21:43:30, greggman wrote:
> see above

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache_unittest.cc:31: const std::string&
shader2,
On 2012/07/10 21:43:30, greggman wrote:
> style: indent

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache_unittest.cc:126: b_sha,
On 2012/07/10 21:43:30, greggman wrote:
> style: indent

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_cache_unittest.cc:130: std::string(a_sha,
ProgramCache::kHashLength),
On 2012/07/10 21:43:30, greggman wrote:
> style: indent

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
File gpu/command_buffer/service/program_manager.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_manager.cc:387: ==
ProgramCache::COMPILATION_SUCCEEDED) {
On 2012/07/10 21:43:30, greggman wrote:
> style: operators go at end of previous line

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_manager.cc:501: AttachShader(manager, info);
On 2012/07/10 21:43:30, greggman wrote:
> You shouldn't need go call AttachShader nor glAttachShader here. The user
> already attached the shaders.

Done.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_manager.cc:503:
info->set_pending_compilation(false);
On 2012/07/10 21:43:30, greggman wrote:
> Should ForceCompileShader set set_pending_compilation to false?
> 
> Seems like there's a chance for double compilation

Hm.... What if we just moved it right after the if statement, because that's
where we check for it?  I feel like it's better for ForceCompileShader to have
no knowledge of cache stuff.

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/39001/gpu/command_buffer...
gpu/command_buffer/service/program_manager_unittest.cc:1162:
.WillOnce(Return(result));
On 2012/07/10 21:43:30, greggman wrote:
> style: indent

Done.

[greggman, 2012-07-12 00:48:29]

Looking good.

Unfortunately there's a problem we hadn't thought of.

If you do this

vs = glCreateShader(..);
glShaderSource(vs, sourceOfCachedShader);
glCompileShader(vs);   // this is pending version since the source matches cache
glShaderSource(vs, newSource);
p = glCreateProgram();
glAttachShader(vs);
...
glLinkProgram(p);

Link will end up compiling with newSource when it should be compiling with
soruceOfCachedShader

That means in ProgramManager::DoCompileShader or some place. If you decide to
pend compilcation you need to save a copy of the source at that point in time in
case the user changes it.

Can you add code to deal with that and a test for it?

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager.cc (right):

http://codereview.chromium.org/10534173/diff/39001/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:503:
info->set_pending_compilation(false);
On 2012/07/11 23:32:52, dmurph wrote:
> On 2012/07/10 21:43:30, greggman wrote:
> > Should ForceCompileShader set set_pending_compilation to false?
> > 
> > Seems like there's a chance for double compilation
> 
> Hm.... What if we just moved it right after the if statement, because that's
> where we check for it?  I feel like it's better for ForceCompileShader to have
> no knowledge of cache stuff.

Maybe I'm mis-understanding. It seems like there's a way I can get
ForceCompileShader to be called from GLES2DecoderImpl::DoCompileShader and then
again in Link

Create program AB1 with shaders with src A and src B. 
Create new shader with src A. Shader is marked as pending since it's the same
source.
Change src of shader (glShaderSource) and call glCompileShader.
ForceComipleShader will be called by GLES2DecoderImpl::DoCompileShader but
shader is still marked as pending.
 
Am I right? If so maybe you could add a test for that case?

http://codereview.chromium.org/10534173/diff/49004/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/49004/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache_unittest.cc:48: for (int i = 0;
i < length; i++) {
EXPECT_EQ(0, memcmp(binary_, binary, length)); ?

[dmurph, 2012-07-12 01:14:08]

Yes, both of those issues definitely exist. I wasn't thinking about the case
where the shader source could be changed, which both issues touch upon. Thanks
for pointing this out!

[dmurph, 2012-07-13 22:09:14]

Patch uploaded with added tests for the two scenarios Gregg outlined, thoroughly
git try'd as well.

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

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager.cc (right):

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager.cc:435:
program_cache_->ShaderCompilationSucceeded(*source);
"source" can be null here in which case this will crash no?

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:138: :  manager_(NULL),
program_info_(NULL) {
style: one space between : and manager_

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:1119: kClientProgramId,
kServiceProgramId);
style: indenting

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
File gpu/command_buffer/service/shader_manager.h (right):

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:37: // prevent the compiled source
from getting deleted when source_ is reset
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:36]

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
File gpu/command_buffer/service/shader_manager.h (right):

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:37: // prevent the compiled source
from getting deleted when source_ is reset
On 2012/07/16 19:41:24, greggman wrote:
> 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?

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)

[greggman, 2012-07-16 21:57:25]

http://codereview.chromium.org/10534173/diff/49004/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/49004/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache_unittest.cc:48: for (int i = 0;
i < length; i++) {
On 2012/07/12 00:48:29, greggman wrote:
> EXPECT_EQ(0, memcmp(binary_, binary, length)); ?

ping?

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
File gpu/command_buffer/service/memory_program_cache.cc (right):

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/memory_program_cache.cc:104:
ComputeShaderHash(*shader_a->source(), a_sha);
shouldn't this be compilation_source()?

Can you add a unit test that fails with this current impl before fixing this?

Basically I understand this correctly

s1 = glCreateShader();
glShaderSource(s1, "orig");
glLinkProgram(p1_that_uses_s1);
s2 = glCreateShader();
glShaderSource(s1, "orig");
glCompileShader(s2, "diff");
glLinkProgram(p2_that_uses_s2);

s1 is already compiled
SaveLinkedProgram will get called but cache under "diff" instead of under "orig"

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:1366: const std::string*
originalSource = vertex_shader_->source();
style: variables are under_score
Also, doesn't UpdateSource potentially release the string pointed to by
originalSource above?

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
File gpu/command_buffer/service/shader_manager.h (right):

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:38: if(!compilation_source_.get() &&
source_.get()) {
Okay then maybe a bunch of comments need to change? See other comments.

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:74: // The source that was compiled
for this shader
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?

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:81: // Stores the current source as
the compiled source
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

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:82: void SetSourceAsCompiled(bool
pending_cache_miss_compilation) {
Also, there needs to be unit tests for these new functions in
shader_manager_unittest.cc

http://codereview.chromium.org/10534173/diff/67002/gpu/command_buffer/service...
gpu/command_buffer/service/shader_manager.h:172: // Holds on to the source that
was supposed to be compiled
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]

Do you think 'source_compiled()' is more clear in shader_manager?  I'm thinking
I should change it back to is_deferred_compilation, or something like that,
because we only use it to check that case.

https://chromiumcodereview.appspot.com/10534173/diff/49004/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache_unittest.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/49004/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache_unittest.cc:48: for (int i = 0;
i < length; i++) {
On 2012/07/16 21:57:25, greggman wrote:
> On 2012/07/12 00:48:29, greggman wrote:
> > EXPECT_EQ(0, memcmp(binary_, binary, length)); ?
> 
> ping?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
File gpu/command_buffer/service/memory_program_cache.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/memory_program_cache.cc:104:
ComputeShaderHash(*shader_a->source(), a_sha);
On 2012/07/16 21:57:26, greggman wrote:
> shouldn't this be compilation_source()?
> 
> Can you add a unit test that fails with this current impl before fixing this?
> 
> Basically I understand this correctly
> 
> s1 = glCreateShader();
> glShaderSource(s1, "orig");
> glLinkProgram(p1_that_uses_s1);
> s2 = glCreateShader();
> glShaderSource(s1, "orig");
> glCompileShader(s2, "diff");
> glLinkProgram(p2_that_uses_s2);
> 
> s1 is already compiled
> SaveLinkedProgram will get called but cache under "diff" instead of under
"orig"

Done, made tests in memory_program_cache_unittests.

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
File gpu/command_buffer/service/program_manager.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/program_manager.cc:435:
program_cache_->ShaderCompilationSucceeded(*source);
On 2012/07/16 19:41:24, greggman wrote:
> "source" can be null here in which case this will crash no?

Done.

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/program_manager_unittest.cc:138: :  manager_(NULL),
program_info_(NULL) {
On 2012/07/16 19:41:24, greggman wrote:
> style: one space between : and manager_

Done.

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/program_manager_unittest.cc:1119: kClientProgramId,
kServiceProgramId);
On 2012/07/16 19:41:24, greggman wrote:
> style: indenting

Done.

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/program_manager_unittest.cc:1366: const std::string*
originalSource = vertex_shader_->source();
On 2012/07/16 21:57:26, greggman wrote:
> style: variables are under_score
> Also, doesn't UpdateSource potentially release the string pointed to by
> originalSource above?

If vertex_shader_ is updated and compiled, then yeah that will be invalid, but
we don't deal with vertex_shader_ in this test.  But to be on the safe side for
future changes, I'll just do a copy.
(also, UpdateSource makes a copy, so new_vertex_shader wouldn't mess anything
up)

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
File gpu/command_buffer/service/shader_manager.h (right):

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/shader_manager.h:38: if(!compilation_source_.get() &&
source_.get()) {
On 2012/07/16 21:57:26, greggman wrote:
> Okay then maybe a bunch of comments need to change? See other comments.

Updated comment, simplified this statement

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/shader_manager.h:74: // The source that was compiled
for this shader
On 2012/07/16 21:57:26, greggman wrote:
> 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?

How about deferred_compilation_source?

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/shader_manager.h:81: // Stores the current source as
the compiled source
On 2012/07/16 21:57:26, greggman wrote:
> 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

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

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/shader_manager.h:82: void SetSourceAsCompiled(bool
pending_cache_miss_compilation) {
On 2012/07/16 21:57:26, greggman wrote:
> Also, there needs to be unit tests for these new functions in
> shader_manager_unittest.cc

Done.

https://chromiumcodereview.appspot.com/10534173/diff/67002/gpu/command_buffer...
gpu/command_buffer/service/shader_manager.h:172: // Holds on to the source that
was supposed to be compiled
On 2012/07/16 21:57:26, greggman wrote:
> 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.

Done.

[greggman, 2012-07-17 20:51:12]

Looks good.

A few minor things then LGTM (see comments)

1) There's another issue but let's fix it in another CL.

The issue is calls to GLES2DecoderImpl::HandleGetTranslatedShaderSourceANGLE
will fail once this is checked in.  The solution is probably that if compilation
is pending then compile.

Can you please file a bug

2) Before checking this in can you build chrome and run the webgl conformance
tests? You'll need to skip the context creation and destruction test. Some tests
may fail. click the run button next to them to see if they pass if run a second
time. Then compare canary or something.

3) Run the OpenGL ES 2.0 conformance tests. You'll need src_internal (if you
don't have it) Once you do, from the src folder

$ python build/gyp_chromium gpu/gles2_conform_test/gles2_conform_test.gyp
$ make BUILDTYPE=Debug -j3 gles2_conform_test_native

edit third_party/gles2_conform/GTF_ES/glsl/GTF/mustpass.run and comment out
lines

91: GL2FixedTests/buffer_clear/input.run
92: GL2FixedTests/buffer_color/input.run
95: GL2FixedTests/clip/input.run
96: GL2FixedTests/color_ramp/input.run
132: GL2Tests/framebuffer_objects/input.run
159: GLCoverage/input.run

edit third_party/gles2_conform/GTF_ES/glsl/GTF/GL/build/input.run and comment
out line

138: preprocess6_frag.test

Then run the test

$ out/Debug/gles2_conform_test_native -noimagefileio
-run=/usr/local/google/gman/src/chrome/src/third_party/gles2_conform/GTF_ES/glsl/GTF/mustpass.run


everything should pass

http://codereview.chromium.org/10534173/diff/66007/gpu/command_buffer/service...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

http://codereview.chromium.org/10534173/diff/66007/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:1385: EXPECT_EQ(false,
new_vertex_shader->source_compiled());
EXPECT_EQ -> EXPECT_FALSE

http://codereview.chromium.org/10534173/diff/66007/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:1386: EXPECT_EQ(false,
fragment_shader_->source_compiled());
EXPECT_EQ -> EXPECT_FALSE

http://codereview.chromium.org/10534173/diff/66007/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:1438: EXPECT_EQ(true,
new_vertex_shader->source_compiled());
EXPECT_EQ -> EXPECT_TRUE

http://codereview.chromium.org/10534173/diff/66007/gpu/command_buffer/service...
gpu/command_buffer/service/program_manager_unittest.cc:1439: EXPECT_EQ(false,
fragment_shader_->source_compiled());
EXPECT_EQ -> EXPECT_FALSE

[dmurph, 2012-07-18 00:45:50]

I received a 'FATAL:program_cache.cc(108)] Check failed: info0.ref_count > 0.'
Error during the second run of the conformance tests.  Just uploaded nit
changes, I'll look into that issue tomorrow, see if I can reproduce it.

https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
gpu/command_buffer/service/program_manager_unittest.cc:1385: EXPECT_EQ(false,
new_vertex_shader->source_compiled());
On 2012/07/17 20:51:12, greggman wrote:
> EXPECT_EQ -> EXPECT_FALSE

Done.

https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
gpu/command_buffer/service/program_manager_unittest.cc:1386: EXPECT_EQ(false,
fragment_shader_->source_compiled());
On 2012/07/17 20:51:12, greggman wrote:
> EXPECT_EQ -> EXPECT_FALSE

Done.

https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
gpu/command_buffer/service/program_manager_unittest.cc:1438: EXPECT_EQ(true,
new_vertex_shader->source_compiled());
On 2012/07/17 20:51:12, greggman wrote:
> EXPECT_EQ -> EXPECT_TRUE

Done.

https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
gpu/command_buffer/service/program_manager_unittest.cc:1439: EXPECT_EQ(false,
fragment_shader_->source_compiled());
On 2012/07/17 20:51:12, greggman wrote:
> EXPECT_EQ -> EXPECT_FALSE

Done.

[dmurph, 2012-07-18 00:46:56]

On 2012/07/18 00:45:50, dmurph wrote:
> I received a 'FATAL:program_cache.cc(108)] Check failed: info0.ref_count > 0.'
> Error during the second run of the conformance tests.  Just uploaded nit
> changes, I'll look into that issue tomorrow, see if I can reproduce it.
> 
>
https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
> File gpu/command_buffer/service/program_manager_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
> gpu/command_buffer/service/program_manager_unittest.cc:1385: EXPECT_EQ(false,
> new_vertex_shader->source_compiled());
> On 2012/07/17 20:51:12, greggman wrote:
> > EXPECT_EQ -> EXPECT_FALSE
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
> gpu/command_buffer/service/program_manager_unittest.cc:1386: EXPECT_EQ(false,
> fragment_shader_->source_compiled());
> On 2012/07/17 20:51:12, greggman wrote:
> > EXPECT_EQ -> EXPECT_FALSE
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
> gpu/command_buffer/service/program_manager_unittest.cc:1438: EXPECT_EQ(true,
> new_vertex_shader->source_compiled());
> On 2012/07/17 20:51:12, greggman wrote:
> > EXPECT_EQ -> EXPECT_TRUE
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10534173/diff/66007/gpu/command_buffer...
> gpu/command_buffer/service/program_manager_unittest.cc:1439: EXPECT_EQ(false,
> fragment_shader_->source_compiled());
> On 2012/07/17 20:51:12, greggman wrote:
> > EXPECT_EQ -> EXPECT_FALSE
> 
> Done.

Oh, and I'll remove that 'histogram' include, sorry.

[dmurph, 2012-07-18 21:30:14]

Fixed eviction error + fixed test to detect it.  Good to submit?

[commit-bot: I haz the power, 2012-07-18 21:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmurph@chromium.org/10534173/74002

[commit-bot: I haz the power, 2012-07-18 21:34:27]

Presubmit check for 10534173-74002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  gpu/command_buffer/service/program_cache_lru_helper.h

Presubmit checks took 3.4s to calculate.

[commit-bot: I haz the power, 2012-07-18 21:38:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmurph@chromium.org/10534173/70004

[commit-bot: I haz the power, 2012-07-18 22:53:32]

Change committed as 147328

[dmurph, 2012-07-20 22:17:05]

Updated patch with the correct extension check, caching hint on desktop, and
force compile/link on load binary failure.  General cleanup as well.