Generate path names on the client side

Generate path names on the client side

Pre-allocates a range of path names and manages allocations within
that range on the client side. This allows us to generate new path
objects in a feed-forward manner that doesn't require round trips to
the GL server.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/511923443facc611b3735b4688342c12071feaa0

[Chris Dalton, 2014-05-29 23:29:42]

No tests yet. Overall the change is complex but helpful.

[bsalomon, 2014-05-30 18:09:11]

Seems like a pretty nice solution to the problem to me. It'd be nice to have
unit tests for the sparse name range.

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
File src/gpu/gl/GrGLNameAllocator.cpp (right):

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.cpp:6: * found in the LICENSE file.
This file could really use some documentation.

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.cpp:26: SparseNameRange* ref() {
Are these really frequently ref'ed and unref'ed?

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator.h
File src/gpu/gl/GrGLNameAllocator.h (right):

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.h:13: #include "gl/GrGLFunctions.h"
Is this for the GrGL data types?

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.h:22: class SparseNameRange;
Can you move these down to the explicit private section?

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.h:30: GrGLuint names() const { return fNames; }
some docs here would useful

[Chris Dalton, 2014-05-30 21:39:41]

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
File src/gpu/gl/GrGLNameAllocator.cpp (right):

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.cpp:26: SparseNameRange* ref() {
On 2014/05/30 18:09:11, bsalomon wrote:
> Are these really frequently ref'ed and unref'ed?

In theory it could. In practice it might not. I can try using standard ref
counting at least for the initial commit. If it starts showing up in the
profiles we can always come back to it.

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator.h
File src/gpu/gl/GrGLNameAllocator.h (right):

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.h:13: #include "gl/GrGLFunctions.h"
On 2014/05/30 18:09:11, bsalomon wrote:
> Is this for the GrGL data types?

Yeah - GrGLuint. Would you prefer to use a more generic type and not need this
header?

[bsalomon, 2014-06-02 15:14:42]

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
File src/gpu/gl/GrGLNameAllocator.cpp (right):

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.cpp:26: SparseNameRange* ref() {
On 2014/05/30 21:39:41, Chris Dalton wrote:
> On 2014/05/30 18:09:11, bsalomon wrote:
> > Are these really frequently ref'ed and unref'ed?
> 
> In theory it could. In practice it might not. I can try using standard ref
> counting at least for the initial commit. If it starts showing up in the
> profiles we can always come back to it.

I'd prefer that. If we're going to tackle non-atomic refs for Gr code, I'd
rather do it separately and more comprehensively (i.e. for more than just one
type).

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator.h
File src/gpu/gl/GrGLNameAllocator.h (right):

https://codereview.chromium.org/304403003/diff/1/src/gpu/gl/GrGLNameAllocator...
src/gpu/gl/GrGLNameAllocator.h:13: #include "gl/GrGLFunctions.h"
On 2014/05/30 21:39:41, Chris Dalton wrote:
> On 2014/05/30 18:09:11, bsalomon wrote:
> > Is this for the GrGL data types?
> 
> Yeah - GrGLuint. Would you prefer to use a more generic type and not need this
> header?

I was mostly just curious. But maybe we could separate out the simple types from
GrGLFunctions.h

[Chris Dalton, 2014-06-06 01:54:55]

And here is the polished set, complete with a new test

[bsalomon, 2014-06-09 13:52:22]

It's looking good. Two small comments.

https://codereview.chromium.org/304403003/diff/20001/src/gpu/gl/GrGLNameAlloc...
File src/gpu/gl/GrGLNameAllocator.h (right):

https://codereview.chromium.org/304403003/diff/20001/src/gpu/gl/GrGLNameAlloc...
src/gpu/gl/GrGLNameAllocator.h:15: /**
Nice dox in this class, thanks!

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest.cpp
File tests/NameAllocatorTest.cpp (right):

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest....
tests/NameAllocatorTest.cpp:15: class NameLeakTest {
style nit: Lots of missing this-> for method calls in this file.

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest....
tests/NameAllocatorTest.cpp:100: const GrGLuint seed = (count + fRandomName) /
2;
Is there a reason for adding a new random number generator rather than
SkRandom::nextULessThan or SkRandom::nextURange?

[Chris Dalton, 2014-06-09 16:59:39]

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest.cpp
File tests/NameAllocatorTest.cpp (right):

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest....
tests/NameAllocatorTest.cpp:100: const GrGLuint seed = (count + fRandomName) /
2;
On 2014/06/09 13:52:21, bsalomon wrote:
> Is there a reason for adding a new random number generator rather than
> SkRandom::nextULessThan or SkRandom::nextURange?

.. Because I wasn't aware of SkRandom?

But also, it looks like SkRandom just does a modulus, whereas the test depends
on the random generator to hit every unique index exactly once before repeating.

[bsalomon, 2014-06-09 17:09:42]

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest.cpp
File tests/NameAllocatorTest.cpp (right):

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest....
tests/NameAllocatorTest.cpp:100: const GrGLuint seed = (count + fRandomName) /
2;
On 2014/06/09 16:59:39, Chris Dalton wrote:
> On 2014/06/09 13:52:21, bsalomon wrote:
> > Is there a reason for adding a new random number generator rather than
> > SkRandom::nextULessThan or SkRandom::nextURange?
> 
> .. Because I wasn't aware of SkRandom?
> 
> But also, it looks like SkRandom just does a modulus, whereas the test depends
> on the random generator to hit every unique index exactly once before
repeating.

Ah, ok, that makes sense.

[Chris Dalton, 2014-06-09 17:27:28]

This addresses the new comments

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest.cpp
File tests/NameAllocatorTest.cpp (right):

https://codereview.chromium.org/304403003/diff/20001/tests/NameAllocatorTest....
tests/NameAllocatorTest.cpp:15: class NameLeakTest {
On 2014/06/09 13:52:21, bsalomon wrote:
> style nit: Lots of missing this-> for method calls in this file.

Done.

[bsalomon, 2014-06-09 17:44:08]

lgtm

[Chris Dalton, 2014-06-09 17:59:35]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2014-06-09 18:00:20]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/cdalton@nvidia.com/304403003/40001

[commit-bot: I haz the power, 2014-06-09 18:17:02]

Change committed as 511923443facc611b3735b4688342c12071feaa0