Add vertex declaration cache

Profiling shows that creating and destroying vertex declarations is extremely expensive, so we can keep a 16-element cache around to speed that up.

BUG=
TEST=JSGameBench

Committed: http://code.google.com/p/angleproject/source/detail?r=609

[nicolas, 2011-04-05 11:50:27]

Just my two cents.

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager.cpp
File src/libGLESv2/VertexDataManager.cpp (right):

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager....
src/libGLESv2/VertexDataManager.cpp:522: int maxLru;
This should be a VertexDataManager member variable, initialized to 0.

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager....
src/libGLESv2/VertexDataManager.cpp:526: D3DVERTEXELEMENT9
elements[MAX_VERTEX_ATTRIBS + 1];
Nicely spotted!

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager....
src/libGLESv2/VertexDataManager.cpp:551: if (memcmp(entry->cachedElements,
elements, element + 1 - elements) == 0 && entry->vertexDeclaration)
The last argument to memcpy is in bytes, while this formula will return the
number of elements. Also, there's no need to compare the last element (which is
always D3DDECL_END()).

So I believe (element - elements) * sizeof(D3DVERTEXELEMENT9) should be used
instead.

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager....
src/libGLESv2/VertexDataManager.cpp:575: memcpy(lastCache->cachedElements,
elements, element + 1 - elements);
Same comment as for memcmp above.

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager.h
File src/libGLESv2/VertexDataManager.h (right):

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager....
src/libGLESv2/VertexDataManager.h:23: const int g_vertexDeclCacheSize = 16;
It is preferable to use an enum inside VertexDataManger instead of a global
(cfr. NUM_GL_VERTEX_ATTRIB_TYPES). That makes it a true compile-time constant.

[apatrick1, 2011-04-05 18:05:22]

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager.cpp
File src/libGLESv2/VertexDataManager.cpp (right):

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager....
src/libGLESv2/VertexDataManager.cpp:546: *element = end;
This comment logically follows the one below. Please read my second comment
first :)

If this line was:

*(element++) = end;

then the easy to forget +1's below would not be needed.

http://codereview.appspot.com/4358051/diff/1/src/libGLESv2/VertexDataManager....
src/libGLESv2/VertexDataManager.cpp:551: if (memcmp(entry->cachedElements,
elements, element + 1 - elements) == 0 && entry->vertexDeclaration)
On 2011/04/05 11:50:27, nicolas wrote:
> The last argument to memcpy is in bytes, while this formula will return the
> number of elements. Also, there's no need to compare the last element (which
is
> always D3DDECL_END()).
> 
> So I believe (element - elements) * sizeof(D3DVERTEXELEMENT9) should be used
> instead.

I think the "+ 1" in (element + 1 - elements) * sizeof(D3DVERTEXELEMENT9) is not
actually needed as nicholas suggests.

Comparing the terminating D3DDECL_END ensures that the "elements" array and
"entry->cachedElements" array are the same length. However the D3DDECL_END
comparison is redundant in the case where both arrays have the maximum length.

That chews up a heck of a lot of mental bandwidth to figure out though for a
simple array comparison. Maybe it would be clearer if the VertexDeclCacheEntry
struct had an explicit length member and we compared that before doing the
memcmp? Then it's clear.

[apatrick1, 2011-04-06 07:32:32]

LGTM

[nicolas, 2011-04-06 07:40:46]

The code looks good to me. I assume it has been/will be extensively tested
against the OpenGL ES 2.0 and WebGL conformance test suites?

By the way, TransGaming is also about to perform a round of optimizations. This
vertex declaration cache is likely to get moved around a bit...

[dgkoch, 2011-04-06 14:15:00]

Other than making the LRU counter unsigned it looks good.

A couple questions though:
 1) do you have measurable performance gains due to this?
 2) have you done experimentation with different sized caches?  I'm just
wondering if 16 is really large enough to have much benefit in say a more
complex application.  That said, it's easy enough to tune that later.

Daniel

http://codereview.appspot.com/4358051/diff/9001/src/libGLESv2/VertexDataManag...
File src/libGLESv2/VertexDataManager.h (right):

http://codereview.appspot.com/4358051/diff/9001/src/libGLESv2/VertexDataManag...
src/libGLESv2/VertexDataManager.h:167: int mMaxLru;
make this unsigned.

http://codereview.appspot.com/4358051/diff/9001/src/libGLESv2/VertexDataManag...
src/libGLESv2/VertexDataManager.h:174: int lruCount;
this should also be unsigned.

[John Bauman, 2011-04-06 16:36:02]

I don't have actual framerate numbers, but before JSGamebench was spending
somewhere around 20% of a core on creating and destroying vertex declarations,
while now it spends 0%.

Unfortunately, I don't really have enough applications to test with to determine
the optimal size. I know from experience that 32 entries is enough for pretty
much any Direct3D app, so unless OpenGL applications are really perverse some
small number will work. I decided to maybe err on the small side, because it is
easy to increase.

[dgkoch, 2011-04-06 18:06:41]

On 2011/04/06 16:36:02, John Bauman wrote:
> I don't have actual framerate numbers, but before JSGamebench was spending
> somewhere around 20% of a core on creating and destroying vertex declarations,
> while now it spends 0%.
> 
> Unfortunately, I don't really have enough applications to test with to
determine
> the optimal size. I know from experience that 32 entries is enough for pretty
> much any Direct3D app, so unless OpenGL applications are really perverse some
> small number will work. I decided to maybe err on the small side, because it
is
> easy to increase.

Preliminary testing with a prototype application that Vangelis sent us lead to a
30% increase and was even higher with a bigger cache.    That said, lets get
this in and we can take over tweaking the size as necessary.

LGTM once you make the LRU variables unsigned.

[apatrick1, 2011-04-06 18:35:09]

Awesome work John. When you check this in, can you roll DEPS to pull this into
Chromium? Let's get this in Canary for testing. Ping me if you have trouble.

Tip: a common DEPS roll error is to not check whether the shader translator has
warnings as errors on linux and mac. It's worth sending the patch to the trybots
on all platforms.