Support multiple different types of data in a buffer

Support multiple different types of data in a buffer

The current static buffer management assumes that there's only one basic set of vertex attributes in a buffer, which will all be used in one draw call. Putting multiple objects with multiple types of attributes into one buffer causes the buffer to be treated as streaming - in fact the buffer switches between streaming and static, due to the way we calculate whether it should be static.

To fix this case, instead make multiple static buffers for one vertex buffer. Each static buffer contains the data used in one draw call. When a bufferSubData occurs, only the relevant data needs to be evicted, so this can also support semi-dynamic buffers better. This change causes problems when the entire buffer has the same format but only (overlapping) portions are used in one draw call, but hopefully that case is rarer than the one addressed.

BUG=197
TEST=

[dgkoch, 2011-08-25 19:11:01]

I need to get Nicolas to look at this more, but if you could give some
before/after performance results from the apps you were testing as well as
against the benchmarks we used in our last set of optimizations (I sent you a
link to the last set of results) that would also be appreciated.

[John Bauman, 2011-08-25 19:29:25]

Ok, will do. In the app I was testing, I think it more than doubled
performance (mostly by reducing pause times when switching from streaming to
static and back), but I'll need to get real numbers.

On Thu, Aug 25, 2011 at 12:11 PM, <daniel@transgaming.com> wrote:

> I need to get Nicolas to look at this more, but if you could give some
> before/after performance results from the apps you were testing as well
> as against the benchmarks we used in our last set of optimizations (I
> sent you a link to the last set of results) that would also be
> appreciated.
>
>
>
http://codereview.appspot.com/**4923045/<http://codereview.appspot.com/4923045/>
>

[nicolas, 2011-08-29 12:21:18]

The patch looks great to me, but I'm also curious about the performance (and
robustness) in practice. I'll be able to do some testing myself next week...

[John Bauman, 2011-08-29 17:28:17]

I may want to change it some. I've found that the webgl ios rage demo causes
some pretty big performance problems, due to creating way too many buffers.

On Mon, Aug 29, 2011 at 5:21 AM, <nicolas@transgaming.com> wrote:

> The patch looks great to me, but I'm also curious about the performance
> (and robustness) in practice. I'll be able to do some testing myself
> next week...
>
>
>
http://codereview.appspot.com/**4923045/<http://codereview.appspot.com/4923045/>
>

[John Bauman, 2011-08-30 21:56:57]

Ok, just changing to use a std::map to look up the various attribute sets can
help a lot for big buffers. I probably want to do that for index buffers as
well, as that can help quite a bit for the webgl-ios-rage demo.

Performance is the same as before on most benchmarks, except my test-case which
puts lots of different types of data into the same buffer, where performance
went from 30->55 fps.

[dgkoch, 2011-09-01 20:32:06]

A few questions below.   
No major concerns at the moment, although Nicolas should look at it as well.

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/Buffer.cpp
File src/libGLESv2/Buffer.cpp (right):

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/Buffer.cpp#newc...
src/libGLESv2/Buffer.cpp:75: mStaticVertexBufferSet->invalidateRange(offset,
size);
Is there any point in doing this in the cases where we call
invalidateStaticData() below?  It seems that will just end up deleting
mStaticVertexBufferSet.

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/VertexDataManag...
File src/libGLESv2/VertexDataManager.cpp (right):

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/VertexDataManag...
src/libGLESv2/VertexDataManager.cpp:207: staticBuffer =
staticBufferSet->newBuffer();
is it possible for this to return NULL?

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/VertexDataManag...
src/libGLESv2/VertexDataManager.cpp:876: if ((mUsedSize > (1<<28)) ||
(mAllocatedSize >= (1<<28)))
should we log an error or assert if this happens?

[John Bauman, 2011-09-01 20:39:56]

Thanks for looking at this.

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/Buffer.cpp
File src/libGLESv2/Buffer.cpp (right):

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/Buffer.cpp#newc...
src/libGLESv2/Buffer.cpp:75: mStaticVertexBufferSet->invalidateRange(offset,
size);
On 2011/09/01 20:32:06, dgkoch wrote:
> Is there any point in doing this in the cases where we call
> invalidateStaticData() below?  It seems that will just end up deleting
> mStaticVertexBufferSet.
Good point. I'm not sure how common sharing a buffer for both index and vertex
data is, but I might as well have the invalidate happen first.

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/VertexDataManag...
File src/libGLESv2/VertexDataManager.cpp (right):

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/VertexDataManag...
src/libGLESv2/VertexDataManager.cpp:207: staticBuffer =
staticBufferSet->newBuffer();
On 2011/09/01 20:32:06, dgkoch wrote:
> is it possible for this to return NULL?
If this lookupBuffer returned NULL, then the lookupBuffer at the top returned
NULL, and we did an addAttribute which created this buffer. I should probably
add a couple of asserts to make sure that's always true.

http://codereview.appspot.com/4923045/diff/6001/src/libGLESv2/VertexDataManag...
src/libGLESv2/VertexDataManager.cpp:876: if ((mUsedSize > (1<<28)) ||
(mAllocatedSize >= (1<<28)))
On 2011/09/01 20:32:06, dgkoch wrote:
> should we log an error or assert if this happens?

No, this happens pretty often. These numbers are just a heuristic, so I don't
really care a huge amount if they reset. Resetting may even help change behavior
if the usage pattern for a buffer changes.