Consolidate GPU buffer implementations

Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1825393002

Committed: https://skia.googlesource.com/skia/+/8b1bff29675afd25843439eade634a57f68fe16f

Committed: https://skia.googlesource.com/skia/+/397536cabe12a9936659870dd220c869789424ba

[Chris Dalton, 2016-03-23 16:48:12]

Description was changed from

==========
Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
==========

to

==========
Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Chris Dalton, 2016-03-23 16:53:48]

cdalton@nvidia.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com, robertphillips@google.com

[Chris Dalton, 2016-03-23 16:53:49]

This is mostly a cut/paste/search/replace marathon. The actual behavior should
be identical to before (working now to generate before/after GL traces that
verify this statement). Files of main interest:

GrTypesPriv.h
GrResourceProvider.*
GrGpu.*
GrGLGpu.*
GrBuffer.h
GrGLBuffer.*

I haven't looked into the Vulkan implementation yet. I'll do that once I get
some positive feedback about the direction this is headed. Any Vulkan pointers
are also greatly appreciated.

https://codereview.chromium.org/1825393002/diff/1/include/gpu/GrContextOptions.h
File include/gpu/GrContextOptions.h (left):

https://codereview.chromium.org/1825393002/diff/1/include/gpu/GrContextOption...
include/gpu/GrContextOptions.h:20: , fGeometryBufferMapThreshold(-1)
Could this affect Chrome? If so it may be easier just to leave the name...

[egdaniel, 2016-03-23 16:56:57]

egdaniel@google.com changed reviewers:
+ jvanverth@google.com

[egdaniel, 2016-03-23 16:56:58]

adding jim for vulkan buffer knowledge

[Chris Dalton, 2016-03-24 03:44:25]

Ok, I've verified that the GL command stream is identical before and after this
change, for all of gm and tests.

[Chris Dalton, 2016-03-24 05:07:59]

This updates the vulkan bits. The vulkan build has several compile errors from
me out of the box, but this is the best I could do.

[jvanverth1, 2016-03-24 13:36:14]

On 2016/03/24 05:07:59, Chris Dalton wrote:
> This updates the vulkan bits. The vulkan build has several compile errors from
> me out of the box, but this is the best I could do.

I tried to apply the patch to test compilation with Vulkan, and gyp reports that
GrGeometryBuffer.h is missing.

[Chris Dalton, 2016-03-24 14:06:32]

On 2016/03/24 13:36:14, jvanverth1 wrote:
> On 2016/03/24 05:07:59, Chris Dalton wrote:
> > This updates the vulkan bits. The vulkan build has several compile errors
from
> > me out of the box, but this is the best I could do.
> 
> I tried to apply the patch to test compilation with Vulkan, and gyp reports
that
> GrGeometryBuffer.h is missing.

Weird, I wonder why It didn't complain for me. Can you try again? Patchset 5
takes GrGeometryBuffer out of gpu.gpyi.

[bsalomon, 2016-03-24 14:30:44]

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h
File src/gpu/GrBuffer.h (right):

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
src/gpu/GrBuffer.h:19: * and "stream" access patterns are disqualified by nature
from being cached and reused.)
why not stream?

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
src/gpu/GrBuffer.h:66: this->onMap();
if (!fMapPtr) {
   this->onMap();
}
return fMapPtr

?

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBufferAllocPo...
File src/gpu/GrBufferAllocPool.h (left):

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBufferAllocPo...
src/gpu/GrBufferAllocPool.h:131: class GrVertexBufferAllocPool : public
GrBufferAllocPool {
Should we get rid of these classes and just add makeVertexSpace and
makeIndexSpace helper functions on GrBufferAllocPool? It could have a generic
makeSpace(size_t size, GrBuffer** buffer, size_t* bufferOffset) exposed as well
that these wrap.

[Chris Dalton, 2016-03-24 15:04:40]

<Publish+Mail link not working. Testing with nonempty commit message>

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h
File src/gpu/GrBuffer.h (right):

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
src/gpu/GrBuffer.h:19: * and "stream" access patterns are disqualified by nature
from being cached and reused.)
On 2016/03/24 14:30:44, bsalomon wrote:
> why not stream?

Stream is defined as "specified once, and used at most a few times". If we
cached and reused it, it would be respecified many times.

We can always change it in the future though. (In fact, skia actually sends
GL_STREAM_DRAW when you ask for dynamic), I just intended for this change to be
a pure refactor with zero change in functionality.

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
src/gpu/GrBuffer.h:66: this->onMap();
On 2016/03/24 14:30:44, bsalomon wrote:
> if (!fMapPtr) {
>    this->onMap();
> }
> return fMapPtr
> 
> ?

Good idea.

Now the comment above says: "It is an error to call map on an already mapped
buffer."

Would you prefer an assert here? Or just changing the comment? I don't care
either way.

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBufferAllocPo...
File src/gpu/GrBufferAllocPool.h (left):

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBufferAllocPo...
src/gpu/GrBufferAllocPool.h:131: class GrVertexBufferAllocPool : public
GrBufferAllocPool {
On 2016/03/24 14:30:44, bsalomon wrote:
> Should we get rid of these classes and just add makeVertexSpace and
> makeIndexSpace helper functions on GrBufferAllocPool? It could have a generic
> makeSpace(size_t size, GrBuffer** buffer, size_t* bufferOffset) exposed as
well
> that these wrap.

There are several things that can be cleaned up and improved once this patch
lands: DSA, tracking buffers by ID, cleaning up these files... I left the class
structure pretty much the same in vk, but with this rework there's actually a
lot of potential to improve things over there as well.

I was kind of hoping to keep this patch as minimal as possible while making sure
it's still solid. Then once it lands, it will be easier to build off it in
future commits by adding new features and cleanups.

[bsalomon, 2016-03-24 15:55:47]

lgtm assuming it doesn't break the vulkan build.

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h
File src/gpu/GrBuffer.h (right):

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
src/gpu/GrBuffer.h:19: * and "stream" access patterns are disqualified by nature
from being cached and reused.)
On 2016/03/24 15:04:40, Chris Dalton wrote:
> On 2016/03/24 14:30:44, bsalomon wrote:
> > why not stream?
> 
> Stream is defined as "specified once, and used at most a few times". If we
> cached and reused it, it would be respecified many times.
> 
> We can always change it in the future though. (In fact, skia actually sends
> GL_STREAM_DRAW when you ask for dynamic), I just intended for this change to
be
> a pure refactor with zero change in functionality.

Gotcha. I see you changed the alloc pool to request dynamic, so should have the
same behavior as before.

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
src/gpu/GrBuffer.h:66: this->onMap();
On 2016/03/24 15:04:40, Chris Dalton wrote:
> On 2016/03/24 14:30:44, bsalomon wrote:
> > if (!fMapPtr) {
> >    this->onMap();
> > }
> > return fMapPtr
> > 
> > ?
> 
> Good idea.
> 
> Now the comment above says: "It is an error to call map on an already mapped
> buffer."
> 
> Would you prefer an assert here? Or just changing the comment? I don't care
> either way.

I think we can just delete the comment.

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBufferAllocPo...
File src/gpu/GrBufferAllocPool.h (left):

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBufferAllocPo...
src/gpu/GrBufferAllocPool.h:131: class GrVertexBufferAllocPool : public
GrBufferAllocPool {
On 2016/03/24 15:04:40, Chris Dalton wrote:
> On 2016/03/24 14:30:44, bsalomon wrote:
> > Should we get rid of these classes and just add makeVertexSpace and
> > makeIndexSpace helper functions on GrBufferAllocPool? It could have a
generic
> > makeSpace(size_t size, GrBuffer** buffer, size_t* bufferOffset) exposed as
> well
> > that these wrap.
> 
> There are several things that can be cleaned up and improved once this patch
> lands: DSA, tracking buffers by ID, cleaning up these files... I left the
class
> structure pretty much the same in vk, but with this rework there's actually a
> lot of potential to improve things over there as well.
> 
> I was kind of hoping to keep this patch as minimal as possible while making
sure
> it's still solid. Then once it lands, it will be easier to build off it in
> future commits by adding new features and cleanups.

sgtm

[Chris Dalton, 2016-03-24 16:07:07]

Jim and Greg, the vk changes are a bare minimum to keep it compiling, but I'm
hoping you agree the changes to Gr allow vk buffers to clean up in the direction
you kind of wanted to go in the first place.

I'd appreciate a quick signoff on the vk side of things. I am unable to build
the vk backend completely right now due to a few warning-errors and other
things...

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h
File src/gpu/GrBuffer.h (right):

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
src/gpu/GrBuffer.h:19: * and "stream" access patterns are disqualified by nature
from being cached and reused.)
On 2016/03/24 15:55:47, bsalomon wrote:
> On 2016/03/24 15:04:40, Chris Dalton wrote:
> > On 2016/03/24 14:30:44, bsalomon wrote:
> > > why not stream?
> > 
> > Stream is defined as "specified once, and used at most a few times". If we
> > cached and reused it, it would be respecified many times.
> > 
> > We can always change it in the future though. (In fact, skia actually sends
> > GL_STREAM_DRAW when you ask for dynamic), I just intended for this change to
> be
> > a pure refactor with zero change in functionality.
> 
> Gotcha. I see you changed the alloc pool to request dynamic, so should have
the
> same behavior as before.

Yep. I traced every GL call for all of gm and tests, with multiple configs. The
200 MB of trace is still identical after applying this patch.

[jvanverth1, 2016-03-24 16:31:32]

On 2016/03/24 16:07:07, Chris Dalton wrote:
> Jim and Greg, the vk changes are a bare minimum to keep it compiling, but I'm
> hoping you agree the changes to Gr allow vk buffers to clean up in the
direction
> you kind of wanted to go in the first place.
> 
> I'd appreciate a quick signoff on the vk side of things. I am unable to build
> the vk backend completely right now due to a few warning-errors and other
> things...
> 
> https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h
> File src/gpu/GrBuffer.h (right):
> 
>
https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
> src/gpu/GrBuffer.h:19: * and "stream" access patterns are disqualified by
nature
> from being cached and reused.)
> On 2016/03/24 15:55:47, bsalomon wrote:
> > On 2016/03/24 15:04:40, Chris Dalton wrote:
> > > On 2016/03/24 14:30:44, bsalomon wrote:
> > > > why not stream?
> > > 
> > > Stream is defined as "specified once, and used at most a few times". If we
> > > cached and reused it, it would be respecified many times.
> > > 
> > > We can always change it in the future though. (In fact, skia actually
sends
> > > GL_STREAM_DRAW when you ask for dynamic), I just intended for this change
to
> > be
> > > a pure refactor with zero change in functionality.
> > 
> > Gotcha. I see you changed the alloc pool to request dynamic, so should have
> the
> > same behavior as before.
> 
> Yep. I traced every GL call for all of gm and tests, with multiple configs.
The
> 200 MB of trace is still identical after applying this patch.

I ran into some "size_t not convertable to int" warnings when compiling, but
after fixing those it compiled. You'll need to rebase to ToT, as Greg renamed
GrVkProgram.{cpp,h} (I rolled back to an earlier commit).

I'll take a quick look at the actual code now, but I expect it'll be okay.

[jvanverth1, 2016-03-24 16:47:16]

On 2016/03/24 16:31:32, jvanverth1 wrote:
> On 2016/03/24 16:07:07, Chris Dalton wrote:
> > Jim and Greg, the vk changes are a bare minimum to keep it compiling, but
I'm
> > hoping you agree the changes to Gr allow vk buffers to clean up in the
> direction
> > you kind of wanted to go in the first place.
> > 
> > I'd appreciate a quick signoff on the vk side of things. I am unable to
build
> > the vk backend completely right now due to a few warning-errors and other
> > things...
> > 
> > https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h
> > File src/gpu/GrBuffer.h (right):
> > 
> >
>
https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
> > src/gpu/GrBuffer.h:19: * and "stream" access patterns are disqualified by
> nature
> > from being cached and reused.)
> > On 2016/03/24 15:55:47, bsalomon wrote:
> > > On 2016/03/24 15:04:40, Chris Dalton wrote:
> > > > On 2016/03/24 14:30:44, bsalomon wrote:
> > > > > why not stream?
> > > > 
> > > > Stream is defined as "specified once, and used at most a few times". If
we
> > > > cached and reused it, it would be respecified many times.
> > > > 
> > > > We can always change it in the future though. (In fact, skia actually
> sends
> > > > GL_STREAM_DRAW when you ask for dynamic), I just intended for this
change
> to
> > > be
> > > > a pure refactor with zero change in functionality.
> > > 
> > > Gotcha. I see you changed the alloc pool to request dynamic, so should
have
> > the
> > > same behavior as before.
> > 
> > Yep. I traced every GL call for all of gm and tests, with multiple configs.
> The
> > 200 MB of trace is still identical after applying this patch.
> 
> I ran into some "size_t not convertable to int" warnings when compiling, but
> after fixing those it compiled. You'll need to rebase to ToT, as Greg renamed
> GrVkProgram.{cpp,h} (I rolled back to an earlier commit).
> 
> I'll take a quick look at the actual code now, but I expect it'll be okay.

Vulkan side looks fine to me. I assume there's still some work to do in the
future, as I can't see why we'd need the separate subclasses for each buffer
type.

[jvanverth1, 2016-03-24 16:49:32]

On 2016/03/24 16:47:16, jvanverth1 wrote:
> On 2016/03/24 16:31:32, jvanverth1 wrote:
> > On 2016/03/24 16:07:07, Chris Dalton wrote:
> > > Jim and Greg, the vk changes are a bare minimum to keep it compiling, but
> I'm
> > > hoping you agree the changes to Gr allow vk buffers to clean up in the
> > direction
> > > you kind of wanted to go in the first place.
> > > 
> > > I'd appreciate a quick signoff on the vk side of things. I am unable to
> build
> > > the vk backend completely right now due to a few warning-errors and other
> > > things...
> > > 
> > > https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h
> > > File src/gpu/GrBuffer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
> > > src/gpu/GrBuffer.h:19: * and "stream" access patterns are disqualified by
> > nature
> > > from being cached and reused.)
> > > On 2016/03/24 15:55:47, bsalomon wrote:
> > > > On 2016/03/24 15:04:40, Chris Dalton wrote:
> > > > > On 2016/03/24 14:30:44, bsalomon wrote:
> > > > > > why not stream?
> > > > > 
> > > > > Stream is defined as "specified once, and used at most a few times".
If
> we
> > > > > cached and reused it, it would be respecified many times.
> > > > > 
> > > > > We can always change it in the future though. (In fact, skia actually
> > sends
> > > > > GL_STREAM_DRAW when you ask for dynamic), I just intended for this
> change
> > to
> > > > be
> > > > > a pure refactor with zero change in functionality.
> > > > 
> > > > Gotcha. I see you changed the alloc pool to request dynamic, so should
> have
> > > the
> > > > same behavior as before.
> > > 
> > > Yep. I traced every GL call for all of gm and tests, with multiple
configs.
> > The
> > > 200 MB of trace is still identical after applying this patch.
> > 
> > I ran into some "size_t not convertable to int" warnings when compiling, but
> > after fixing those it compiled. You'll need to rebase to ToT, as Greg
renamed
> > GrVkProgram.{cpp,h} (I rolled back to an earlier commit).
> > 
> > I'll take a quick look at the actual code now, but I expect it'll be okay.
> 
> Vulkan side looks fine to me. I assume there's still some work to do in the
> future, as I can't see why we'd need the separate subclasses for each buffer
> type.

Actually, hang on -- I just tried to run my build and it asserted in
GrVkBuffer::onUpdateData() (it thinks the buffer is mapped).

[jvanverth1, 2016-03-24 16:55:19]

https://codereview.chromium.org/1825393002/diff/100001/src/gpu/GrResourceProv...
File src/gpu/GrResourceProvider.cpp (right):

https://codereview.chromium.org/1825393002/diff/100001/src/gpu/GrResourceProv...
src/gpu/GrResourceProvider.cpp:40: uint16_t* data = (uint16_t*) buffer->map();
I'm not sure how this ever worked -- if we useTempData, we map() the buffer here
and then try to update it, and never unmap it.

[jvanverth1, 2016-03-24 17:05:43]

A couple of other things. First, on the Vulkan side we are not storing
UniformBuffers in the GrResourceCache. But now they're derived from
GrGpuResource (via GrBuffer) so when we unref() them,
GrResourceCache::notifyCntReachedZero() asserts because they're not in the
cache.

And also:

https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer.h
File src/gpu/vk/GrVkBuffer.h (right):

https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer....
src/gpu/vk/GrVkBuffer.h:80: void*                   fMapPtr;
This needs to be deleted -- it duplicates fMapPtr in GrBuffer.

[jvanverth1, 2016-03-24 17:06:38]

On 2016/03/24 17:05:43, jvanverth1 wrote:
> A couple of other things. First, on the Vulkan side we are not storing
> UniformBuffers in the GrResourceCache. But now they're derived from
> GrGpuResource (via GrBuffer) so when we unref() them,
> GrResourceCache::notifyCntReachedZero() asserts because they're not in the
> cache.
> 
> And also:
> 
> https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer.h
> File src/gpu/vk/GrVkBuffer.h (right):
> 
>
https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer....
> src/gpu/vk/GrVkBuffer.h:80: void*                   fMapPtr;
> This needs to be deleted -- it duplicates fMapPtr in GrBuffer.

To sum up: this does not look good to me.

[Chris Dalton, 2016-03-24 17:09:13]

On 2016/03/24 17:06:38, jvanverth1 wrote:
> On 2016/03/24 17:05:43, jvanverth1 wrote:
> > A couple of other things. First, on the Vulkan side we are not storing
> > UniformBuffers in the GrResourceCache. But now they're derived from
> > GrGpuResource (via GrBuffer) so when we unref() them,
> > GrResourceCache::notifyCntReachedZero() asserts because they're not in the
> > cache.
> > 
> > And also:
> > 
> >
https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer.h
> > File src/gpu/vk/GrVkBuffer.h (right):
> > 
> >
>
https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer....
> > src/gpu/vk/GrVkBuffer.h:80: void*                   fMapPtr;
> > This needs to be deleted -- it duplicates fMapPtr in GrBuffer.
> 
> To sum up: this does not look good to me.

Ok, putting uniform buffers in the resource cache is a big change. I'll fix
that.

Also, it looks like some of the warning-errors are mine, so maybe I can get it
building after all.

[jvanverth1, 2016-03-24 17:23:17]

On 2016/03/24 17:09:13, Chris Dalton wrote:
> On 2016/03/24 17:06:38, jvanverth1 wrote:
> > On 2016/03/24 17:05:43, jvanverth1 wrote:
> > > A couple of other things. First, on the Vulkan side we are not storing
> > > UniformBuffers in the GrResourceCache. But now they're derived from
> > > GrGpuResource (via GrBuffer) so when we unref() them,
> > > GrResourceCache::notifyCntReachedZero() asserts because they're not in the
> > > cache.
> > > 
> > > And also:
> > > 
> > >
> https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer.h
> > > File src/gpu/vk/GrVkBuffer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer....
> > > src/gpu/vk/GrVkBuffer.h:80: void*                   fMapPtr;
> > > This needs to be deleted -- it duplicates fMapPtr in GrBuffer.
> > 
> > To sum up: this does not look good to me.
> 
> Ok, putting uniform buffers in the resource cache is a big change. I'll fix
> that.
> 
> Also, it looks like some of the warning-errors are mine, so maybe I can get it
> building after all.

I'm not sure what your plan is, but I'd be more in favor of a change where
uniform and transfer buffers aren't derived from GrGpuResource (because IMHO
they don't need to be in the cache). There might be something I'm missing,
though.

[bsalomon, 2016-03-24 17:28:45]

On 2016/03/24 17:23:17, jvanverth1 wrote:
> On 2016/03/24 17:09:13, Chris Dalton wrote:
> > On 2016/03/24 17:06:38, jvanverth1 wrote:
> > > On 2016/03/24 17:05:43, jvanverth1 wrote:
> > > > A couple of other things. First, on the Vulkan side we are not storing
> > > > UniformBuffers in the GrResourceCache. But now they're derived from
> > > > GrGpuResource (via GrBuffer) so when we unref() them,
> > > > GrResourceCache::notifyCntReachedZero() asserts because they're not in
the
> > > > cache.
> > > > 
> > > > And also:
> > > > 
> > > >
> >
https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer.h
> > > > File src/gpu/vk/GrVkBuffer.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1825393002/diff/100001/src/gpu/vk/GrVkBuffer....
> > > > src/gpu/vk/GrVkBuffer.h:80: void*                   fMapPtr;
> > > > This needs to be deleted -- it duplicates fMapPtr in GrBuffer.
> > > 
> > > To sum up: this does not look good to me.
> > 
> > Ok, putting uniform buffers in the resource cache is a big change. I'll fix
> > that.
> > 
> > Also, it looks like some of the warning-errors are mine, so maybe I can get
it
> > building after all.
> 
> I'm not sure what your plan is, but I'd be more in favor of a change where
> uniform and transfer buffers aren't derived from GrGpuResource (because IMHO
> they don't need to be in the cache). There might be something I'm missing,
> though.

Don't we need to account for the memory that they use?

[Chris Dalton, 2016-03-24 18:14:11]

Ok, this is a much simpler stab at the vulkan part.

In the future we can consolidate vk buffers better.

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h
File src/gpu/GrBuffer.h (right):

https://codereview.chromium.org/1825393002/diff/80001/src/gpu/GrBuffer.h#newc...
src/gpu/GrBuffer.h:19: * and "stream" access patterns are disqualified by nature
from being cached and reused.)
On 2016/03/24 16:07:06, Chris Dalton wrote:
> On 2016/03/24 15:55:47, bsalomon wrote:
> > On 2016/03/24 15:04:40, Chris Dalton wrote:
> > > On 2016/03/24 14:30:44, bsalomon wrote:
> > > > why not stream?
> > > 
> > > Stream is defined as "specified once, and used at most a few times". If we
> > > cached and reused it, it would be respecified many times.
> > > 
> > > We can always change it in the future though. (In fact, skia actually
sends
> > > GL_STREAM_DRAW when you ask for dynamic), I just intended for this change
to
> > be
> > > a pure refactor with zero change in functionality.
> > 
> > Gotcha. I see you changed the alloc pool to request dynamic, so should have
> the
> > same behavior as before.
> 
> Yep. I traced every GL call for all of gm and tests, with multiple configs.
The
> 200 MB of trace is still identical after applying this patch.

Done.

[jvanverth1, 2016-03-24 19:21:01]

lgtm

A few casts you might need to add (I hit warnings on my build):

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/GrBuffer.h
File src/gpu/GrBuffer.h (right):

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/GrBuffer.h#new...
src/gpu/GrBuffer.h:27: builder[1] = size;
needs a cast to uint32_t

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/GrBuffer.h#new...
src/gpu/GrBuffer.h:35: builder[1] = size;
needs a cast to uint32_t

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/batches/GrAADi...
File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/batches/GrAADi...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:495: int maxInstancesPerDraw =
flushInfo->fIndexBuffer->gpuMemorySize() / sizeof(uint16_t) / 6;
needs a cast to int

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/batches/GrAtla...
File src/gpu/batches/GrAtlasTextBatch.cpp (right):

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/batches/GrAtla...
src/gpu/batches/GrAtlasTextBatch.cpp:184: int maxGlyphsPerDraw =
flushInfo->fIndexBuffer->gpuMemorySize() / sizeof(uint16_t) / 6;
needs a cast to int

[Chris Dalton, 2016-03-24 21:12:06]

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/GrBuffer.h
File src/gpu/GrBuffer.h (right):

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/GrBuffer.h#new...
src/gpu/GrBuffer.h:27: builder[1] = size;
On 2016/03/24 19:21:01, jvanverth1 wrote:
> needs a cast to uint32_t

Done.

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/GrBuffer.h#new...
src/gpu/GrBuffer.h:35: builder[1] = size;
On 2016/03/24 19:21:01, jvanverth1 wrote:
> needs a cast to uint32_t

Done.

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/batches/GrAADi...
File src/gpu/batches/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/batches/GrAADi...
src/gpu/batches/GrAADistanceFieldPathRenderer.cpp:495: int maxInstancesPerDraw =
flushInfo->fIndexBuffer->gpuMemorySize() / sizeof(uint16_t) / 6;
On 2016/03/24 19:21:01, jvanverth1 wrote:
> needs a cast to int

Done.

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/batches/GrAtla...
File src/gpu/batches/GrAtlasTextBatch.cpp (right):

https://codereview.chromium.org/1825393002/diff/120001/src/gpu/batches/GrAtla...
src/gpu/batches/GrAtlasTextBatch.cpp:184: int maxGlyphsPerDraw =
flushInfo->fIndexBuffer->gpuMemorySize() / sizeof(uint16_t) / 6;
On 2016/03/24 19:21:01, jvanverth1 wrote:
> needs a cast to int

Done.

[Chris Dalton, 2016-03-24 21:52:43]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-03-24 21:52:43]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
jvanverth@google.com
Link to the patchset: https://codereview.chromium.org/1825393002/#ps140001
(title: "casts")

[commit-bot: I haz the power, 2016-03-24 21:52:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825393002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825393002/140001

[commit-bot: I haz the power, 2016-03-24 21:54:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-24 21:54:24]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[Chris Dalton, 2016-03-25 08:31:00]

The CQ bit was checked by cdalton@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-25 08:31:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825393002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825393002/160001

[commit-bot: I haz the power, 2016-03-25 08:33:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-25 08:33:01]

Dry run: Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[Chris Dalton, 2016-03-25 08:37:44]

The CQ bit was checked by cdalton@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-25 08:37:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825393002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825393002/180001

[commit-bot: I haz the power, 2016-03-25 08:49:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-25 08:49:01]

Dry run: This issue passed the CQ dry run.

[Chris Dalton, 2016-03-25 08:53:22]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-03-25 08:53:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
jvanverth@google.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1825393002/#ps180001
(title: "more warnings")

[commit-bot: I haz the power, 2016-03-25 08:53:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825393002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825393002/180001

[commit-bot: I haz the power, 2016-03-25 08:54:59]

Description was changed from

==========
Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/8b1bff29675afd25843439eade634a57f68fe16f
==========

[commit-bot: I haz the power, 2016-03-25 08:55:01]

Committed patchset #10 (id:180001) as
https://skia.googlesource.com/skia/+/8b1bff29675afd25843439eade634a57f68fe16f

[robertphillips, 2016-03-25 11:55:26]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1831133004/ by robertphillips@google.com.

The reason for reverting is: Lots of Android redness.

[robertphillips, 2016-03-25 13:24:17]

In case the bits rot, the Android One, Galaxy S3, Nexus 10, Nexus 5, Nexus 6 and
Nexus Player Debug bots were failing with:

../../../src/gpu/gl/GrGLBuffer.cpp:328: fatal error: ""0x8892 == fTarget ||
0x8893 == fTarget || 0x88EB == fTarget || 0x88EC == fTarget || 0x78ED == fTarget
|| 0x78EC == fTarget""

[Chris Dalton, 2016-03-25 16:10:10]

Description was changed from

==========
Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/8b1bff29675afd25843439eade634a57f68fe16f
==========

to

==========
Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/8b1bff29675afd25843439eade634a57f68fe16f
==========

[Chris Dalton, 2016-03-25 16:14:38]

The CQ bit was checked by cdalton@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-25 16:14:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825393002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825393002/200001

[commit-bot: I haz the power, 2016-03-25 16:29:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-25 16:29:34]

Dry run: This issue passed the CQ dry run.

[Chris Dalton, 2016-03-25 18:42:41]

Qualcomm, ARM, and Imagination are green now. This was just a hyperactive
assert.

I see GrGLBuffer eventually getting to the point that it doesn't have a target
field anymore, and we just bind based on the GrBufferType instead.

Does patchset 11 look ok?

[jvanverth1, 2016-03-25 18:54:10]

On 2016/03/25 18:42:41, Chris Dalton wrote:
> Qualcomm, ARM, and Imagination are green now. This was just a hyperactive
> assert.
> 
> I see GrGLBuffer eventually getting to the point that it doesn't have a target
> field anymore, and we just bind based on the GrBufferType instead.
> 
> Does patchset 11 look ok?

Looks fine to me.

[Chris Dalton, 2016-03-25 19:13:36]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-03-25 19:13:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
jvanverth@google.com, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1825393002/#ps200001
(title: "asserts")

[commit-bot: I haz the power, 2016-03-25 19:13:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825393002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825393002/200001

[commit-bot: I haz the power, 2016-03-25 19:15:06]

Description was changed from

==========
Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/8b1bff29675afd25843439eade634a57f68fe16f
==========

to

==========
Consolidate GPU buffer implementations

Consolidates all the different buffer implementations into a single
GrBuffer class. This will allow us to add new buffer types, use DSA in
OpenGL, track buffer bindings by unique ID, cache buffers without
respect to the type of data they have been used for previously, etc.
This change is strictly a refactor; it introduces no change in
functionality.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/8b1bff29675afd25843439eade634a57f68fe16f

Committed:
https://skia.googlesource.com/skia/+/397536cabe12a9936659870dd220c869789424ba
==========

[commit-bot: I haz the power, 2016-03-25 19:15:08]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/397536cabe12a9936659870dd220c869789424ba