Add build flag to prevent use of GL constant vertex attr

[bsalomon, 2011-04-19 16:51:37]

The motivation for this change is that constant vertex attributes (specified via
glVertexAttrib rather than glVertexAttribPointer) are really slow through ANGLE.
It creates a temporary VB per draw.

Longer term I'd like to do 2 things: Move this and other GL behavior flags to
GrGLInterface and make the GrGLInterface be specified per-GrContext. This would
allow us to have different GrContext objects in the same app with different
configurations.

[reed1, 2011-04-19 17:43:58]

LGTM

[junov, 2011-04-19 18:04:15]

Awesome that you found this fast path!

http://codereview.appspot.com/4434057/diff/1/gpu/include/GrGLConfig_chrome.h
File gpu/include/GrGLConfig_chrome.h (right):

http://codereview.appspot.com/4434057/diff/1/gpu/include/GrGLConfig_chrome.h#...
gpu/include/GrGLConfig_chrome.h:11: #define GR_GL_NO_CONSTANT_ATTRIBUTES   
GR_WIN32_BUILD
Are we assuming that win32 builds use angle?  Is that a correct assumption?

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGLProgram.cpp#newcode434
gpu/src/GrGLProgram.cpp:434: GrAssert(-1 !=
programData->fUniLocations.fColorUni);
It would be nice to have a constant with a name that expresses what -1 means.
Something like kInvalidUniform or kUnusedUniform.

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGLProgram.h
File gpu/src/GrGLProgram.h (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGLProgram.h#newcode94
gpu/src/GrGLProgram.h:94: // using shorts instead int, enum type, or bool
Are you sure that using shorts is that good of an idea? In my opinion, this is
an over-optimization that may end-up doing more harm than good. Writing to
packed shorts causes a CPU pipeline stall (storage forwarding blocked).  On
current Intel CPUs, it is often a lot less of a performance hit to process a few
extra bytes of data.  If you just used an enum type, you would be letting the
compiler decide what it thinks is best on each platform.

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:508: desc.fColorType =
GrGLProgram::ProgramDesc::kUniform_ColorType;
Not a big deal, but shouldn't this go before the GR_AGGRESSIVE_SHADER_OPTS part?

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders2.cpp
File gpu/src/GrGpuGLShaders2.cpp (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders2.cpp#newc...
gpu/src/GrGpuGLShaders2.cpp:1: /*
General comment for this file: when do we stop maintaining it?

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders2.cpp#newc...
gpu/src/GrGpuGLShaders2.cpp:392: float colorType = random.nextF();
Please explain.  Why does the color type need to be random?  Was this a test?

[bsalomon, 2011-04-19 19:31:59]

Thanks for the review! Updated with a new patch.

http://codereview.appspot.com/4434057/diff/1/gpu/include/GrGLConfig_chrome.h
File gpu/include/GrGLConfig_chrome.h (right):

http://codereview.appspot.com/4434057/diff/1/gpu/include/GrGLConfig_chrome.h#...
gpu/include/GrGLConfig_chrome.h:11: #define GR_GL_NO_CONSTANT_ATTRIBUTES   
GR_WIN32_BUILD
On 2011/04/19 18:04:15, junov wrote:
> Are we assuming that win32 builds use angle?  Is that a correct assumption?

Note that this is a chrome-specific file. So it's correct unless the
--use-gl=desktop command line parameter is passed. I haven't seen a performance
decrease when using this in that case, though. It's also a dev/debugging flag.
Once the GL optimization flags are specifiable at runtime we could detect this
when setting up a GrGLInterface for the GrContext in Chrome.

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGLProgram.cpp
File gpu/src/GrGLProgram.cpp (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGLProgram.cpp#newcode434
gpu/src/GrGLProgram.cpp:434: GrAssert(-1 !=
programData->fUniLocations.fColorUni);
On 2011/04/19 18:04:15, junov wrote:
> It would be nice to have a constant with a name that expresses what -1 means.
> Something like kInvalidUniform or kUnusedUniform.

Agreed. I'll file a bug for that since we're using -1 many other places to
indicate unused uniforms (gl's convention).

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGLProgram.h
File gpu/src/GrGLProgram.h (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGLProgram.h#newcode94
gpu/src/GrGLProgram.h:94: // using shorts instead int, enum type, or bool
On 2011/04/19 18:04:15, junov wrote:
> Are you sure that using shorts is that good of an idea? In my opinion, this is
> an over-optimization that may end-up doing more harm than good. Writing to
> packed shorts causes a CPU pipeline stall (storage forwarding blocked).  On
> current Intel CPUs, it is often a lot less of a performance hit to process a
few
> extra bytes of data.  If you just used an enum type, you would be letting the
> compiler decide what it thinks is best on each platform. 

I agree completely. The reason I was trying to pack these in with fVertexLayout
was that GrVertexLayout is a short and I didn't want padding since this thing is
used as a key. It was boneheaded in the first place to make GrVertexLayout a
short. So I went ahead and changed that. I also now memset the desc to 0 so any
padding is cleared.

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:508: desc.fColorType =
GrGLProgram::ProgramDesc::kUniform_ColorType;
On 2011/04/19 18:04:15, junov wrote:
> Not a big deal, but shouldn't this go before the GR_AGGRESSIVE_SHADER_OPTS
part?

We want to prefer kNone to kUniform if its an option.

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders2.cpp
File gpu/src/GrGpuGLShaders2.cpp (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders2.cpp#newc...
gpu/src/GrGpuGLShaders2.cpp:1: /*
On 2011/04/19 18:04:15, junov wrote:
> General comment for this file: when do we stop maintaining it?

I think the test should be verify no DRT regressions with the new code and that
the IE test drive canvas tests get the same performance.

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders2.cpp#newc...
gpu/src/GrGpuGLShaders2.cpp:392: float colorType = random.nextF();
On 2011/04/19 18:04:15, junov wrote:
> Please explain.  Why does the color type need to be random?  Was this a test?

Yup, this is  the UnitTest function that generates shaders from random
ProgramDescs and verifies that they compile and link. (I temporarily enabled it
while developing my CL).

[reed1, 2011-04-19 19:41:04]

keep it up, junov!

[junov, 2011-04-19 20:05:47]

LGTM

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4434057/diff/1/gpu/src/GrGpuGLShaders.cpp#newco...
gpu/src/GrGpuGLShaders.cpp:508: desc.fColorType =
GrGLProgram::ProgramDesc::kUniform_ColorType;
Ah, I missed the dangling 'else' in my first read. So this works, but loses
points for legibility. :-)
On 2011/04/19 19:31:59, bsalomon wrote:
> On 2011/04/19 18:04:15, junov wrote:
> > Not a big deal, but shouldn't this go before the GR_AGGRESSIVE_SHADER_OPTS
> part?
> 
> We want to prefer kNone to kUniform if its an option.

http://codereview.appspot.com/4434057/diff/6001/gpu/src/GrGpuGLShaders2.cpp
File gpu/src/GrGpuGLShaders2.cpp (right):

http://codereview.appspot.com/4434057/diff/6001/gpu/src/GrGpuGLShaders2.cpp#n...
gpu/src/GrGpuGLShaders2.cpp:253: return (x & 0x00ff00) | (x << 24) | (x >> 24);
Don't you like that we will longer have worry about this stuff in the new
GrGpuGLShaders ;-))