When we're not using the NULL buffer data hint update with glBufferData rather than glBufferSubData

[bsalomon, 2011-10-10 13:34:33]

This is the fix for the Mac performance issues in C2D. Using the NULL hint is
still faster on at least some systems when talking to a real GL driver rather
than the cmd buffer (e.g. my Z600 with AMD HD5770). 

I got rid of GrGeometryBuffer::updateSubData because no one was using it and
this change would break it if we did:

buffer->updateData(data, dataSize); // where dataSize < buffer->size();
buffer->updateSubData(subData, subSize, subOffset); // where subSize + subOffset
> dataSize

[TomH, 2011-10-10 14:12:21]

Yay for eliminating unused code. Was it ever used? If so, what changed to
obsolete it? Or was it code we wrote on spec that turned out unnecessary?

http://codereview.appspot.com/5253047/diff/1001/gpu/include/GrGLConfig.h
File gpu/include/GrGLConfig.h (right):

http://codereview.appspot.com/5253047/diff/1001/gpu/include/GrGLConfig.h#newc...
gpu/include/GrGLConfig.h:68: * However, this can be an unoptomization on some
platforms, esp. Chrome.
sp? - unoptimization
(which is an interesting coinage...)

http://codereview.appspot.com/5253047/diff/1001/gpu/src/GrGLIndexBuffer.cpp
File gpu/src/GrGLIndexBuffer.cpp (right):

http://codereview.appspot.com/5253047/diff/1001/gpu/src/GrGLIndexBuffer.cpp#n...
gpu/src/GrGLIndexBuffer.cpp:118: // allocate a new buffer by making the old
contents in accessible
Fix comment - is this "marking the old comments inaccessible?" Or something else
I'm not understanding?

http://codereview.appspot.com/5253047/diff/1001/gpu/src/GrGLVertexBuffer.cpp
File gpu/src/GrGLVertexBuffer.cpp (right):

http://codereview.appspot.com/5253047/diff/1001/gpu/src/GrGLVertexBuffer.cpp#...
gpu/src/GrGLVertexBuffer.cpp:121: }
Maybe stupid question - but this looks identical to
GrGLIndexBuffer::updateData(). Am I missing some difference? Or are we violating
DRY? If so, could we avoid doing so?

[bsalomon, 2011-10-10 14:36:58]

>Yay for eliminating unused code. Was it ever used? If so, >what changed to
>obsolete it? Or was it code we wrote on spec that turned out >unnecessary?

It was originally going to be used in GrBufferAllocPool but then I realized that
unless we use glDrawRangeElements rather than glDrawElements the driver may not
know if previous draws referenced memory in the sub-range being updated or not.
So I don't think it was ever actually used.

http://codereview.appspot.com/5253047/diff/1001/gpu/include/GrGLConfig.h
File gpu/include/GrGLConfig.h (right):

http://codereview.appspot.com/5253047/diff/1001/gpu/include/GrGLConfig.h#newc...
gpu/include/GrGLConfig.h:68: * However, this can be an unoptomization on some
platforms, esp. Chrome.
On 2011/10/10 14:12:21, TomH wrote:
> sp? - unoptimization
> (which is an interesting coinage...)
Heh, fixed the spelling, I don't know of a good simple antonym for optimization
so I made one up. :)

http://codereview.appspot.com/5253047/diff/1001/gpu/src/GrGLIndexBuffer.cpp
File gpu/src/GrGLIndexBuffer.cpp (right):

http://codereview.appspot.com/5253047/diff/1001/gpu/src/GrGLIndexBuffer.cpp#n...
gpu/src/GrGLIndexBuffer.cpp:118: // allocate a new buffer by making the old
contents in accessible
On 2011/10/10 14:12:21, TomH wrote:
> Fix comment - is this "marking the old comments inaccessible?" Or something
else
> I'm not understanding?
New comment should clarify.

http://codereview.appspot.com/5253047/diff/1001/gpu/src/GrGLVertexBuffer.cpp
File gpu/src/GrGLVertexBuffer.cpp (right):

http://codereview.appspot.com/5253047/diff/1001/gpu/src/GrGLVertexBuffer.cpp#...
gpu/src/GrGLVertexBuffer.cpp:121: }
On 2011/10/10 14:12:21, TomH wrote:
> Maybe stupid question - but this looks identical to
> GrGLIndexBuffer::updateData(). Am I missing some difference? Or are we
violating
> DRY? If so, could we avoid doing so?

Yes, the implementations are almost identical. Consolidating is complicated by
the inheritance hierarchy:

GrGeometryBuffer--->GrIndexBuffer---->GrGLIndexBuffer
                |
                --->GrVertexBuffer--->GrGLVertexBuffer

We could probably have a implementation class that does most of the work. It'd
be templated on a GrGLEnum that is either GR_GL_ARRAY_BUFFER or
GR_GL_ELEMENT_ARRAY_BUFFER. I filed skia issue 373 to do this.

[TomH, 2011-10-10 14:42:44]

On 2011/10/10 14:36:58, bsalomon wrote:
> Yes, the implementations are almost identical. Consolidating is complicated by
> the inheritance hierarchy:
> 
> GrGeometryBuffer--->GrIndexBuffer---->GrGLIndexBuffer
>                 |
>                 --->GrVertexBuffer--->GrGLVertexBuffer
> 
> We could probably have a implementation class that does most of the work. It'd
> be templated on a GrGLEnum that is either GR_GL_ARRAY_BUFFER or
> GR_GL_ELEMENT_ARRAY_BUFFER. I filed skia issue 373 to do this.

Hmm, that sounds like it's introducing its own complexities. Ah well.

LGTM.

[bsalomon, 2011-10-12 18:14:32]

Closed with r2443