Track GL buffer state based on unique resource ID

Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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

Committed: https://skia.googlesource.com/skia/+/deacc97bc63513b5eacaf21f858727f6e8b98ce5

Committed: https://skia.googlesource.com/skia/+/e2e71c2df4e72e897bbe745752be0444aee5c29f

[Chris Dalton, 2016-04-05 14:51:58]

Description was changed from

==========
Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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

to

==========
Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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

[Chris Dalton, 2016-04-05 14:52:54]

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

[bsalomon, 2016-04-05 15:17:28]

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

https://codereview.chromium.org/1854283004/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:254: fCopyProgramArrayBuffer.reset();
Should we go ahead and declare GrGLGpu final? I doubt it would ever get
subclassed, but if so this could be a problem.

https://codereview.chromium.org/1854283004/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:291: void GrGLGpu::initGpuResources() {
Could we eliminate the need for this deferred initializer by just doing these
things lazily the first time they are needed?

https://codereview.chromium.org/1854283004/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:424: if (fCopyProgramArrayBuffer) {
We shouldn't ever call abandon outside of the cache. It is only public because
of some terrible issue in Chromium. The cache will abandon all the GrGpuResource
instances.

[Chris Dalton, 2016-04-05 17:59:15]

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

https://codereview.chromium.org/1854283004/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:254: fCopyProgramArrayBuffer.reset();
On 2016/04/05 15:17:28, bsalomon wrote:
> Should we go ahead and declare GrGLGpu final? I doubt it would ever get
> subclassed, but if so this could be a problem.

Done.

https://codereview.chromium.org/1854283004/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:291: void GrGLGpu::initGpuResources() {
On 2016/04/05 15:17:28, bsalomon wrote:
> Could we eliminate the need for this deferred initializer by just doing these
> things lazily the first time they are needed?

Done.

https://codereview.chromium.org/1854283004/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:424: if (fCopyProgramArrayBuffer) {
On 2016/04/05 15:17:28, bsalomon wrote:
> We shouldn't ever call abandon outside of the cache. It is only public because
> of some terrible issue in Chromium. The cache will abandon all the
GrGpuResource
> instances.

Done.

[bsalomon, 2016-04-06 13:40:33]

lgtm

[Chris Dalton, 2016-04-06 13:41:48]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-04-06 13:42:00]

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

[Chris Dalton, 2016-04-06 13:42:29]

The CQ bit was unchecked by cdalton@nvidia.com

[Chris Dalton, 2016-04-06 13:43:04]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-04-06 13:43:09]

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

[commit-bot: I haz the power, 2016-04-06 13:44:41]

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

[commit-bot: I haz the power, 2016-04-06 13:44:42]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[Chris Dalton, 2016-04-06 14:31:47]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-04-06 14:31:47]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1854283004/#ps60001
(title: "uglify for msvc")

[commit-bot: I haz the power, 2016-04-06 14:31:56]

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

[commit-bot: I haz the power, 2016-04-06 16:17:56]

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

[commit-bot: I haz the power, 2016-04-06 16:17:57]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[Chris Dalton, 2016-04-06 17:10:41]

The CQ bit was checked by cdalton@nvidia.com

[commit-bot: I haz the power, 2016-04-06 17:10:49]

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

[commit-bot: I haz the power, 2016-04-06 19:01:22]

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

[commit-bot: I haz the power, 2016-04-06 19:01:23]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[Chris Dalton, 2016-04-06 20:05:54]

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

[commit-bot: I haz the power, 2016-04-06 20:05:58]

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

[Chris Dalton, 2016-04-06 20:06:22]

https://codereview.chromium.org/1854283004/diff/80001/tests/GpuLayerCacheTest...
File tests/GpuLayerCacheTest.cpp (right):

https://codereview.chromium.org/1854283004/diff/80001/tests/GpuLayerCacheTest...
tests/GpuLayerCacheTest.cpp:110: DEF_GPUTEST_FOR_NULL_CONTEXT(GpuLayerCache,
reporter, ctxInfo) {
This technically isn't a problem right now, but since the GrGLGpu can now create
resources of its own it isn't safe to assert on absolute stats anymore. Null
context should be fine. Is there a reason I'm not seeing that this test needs
real rendering contexts?

[commit-bot: I haz the power, 2016-04-06 20:33:45]

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

[commit-bot: I haz the power, 2016-04-06 20:33:46]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-04-06 20:58:06]

https://codereview.chromium.org/1854283004/diff/80001/tests/GpuLayerCacheTest...
File tests/GpuLayerCacheTest.cpp (right):

https://codereview.chromium.org/1854283004/diff/80001/tests/GpuLayerCacheTest...
tests/GpuLayerCacheTest.cpp:110: DEF_GPUTEST_FOR_NULL_CONTEXT(GpuLayerCache,
reporter, ctxInfo) {
On 2016/04/06 20:06:21, Chris Dalton wrote:
> This technically isn't a problem right now, but since the GrGLGpu can now
create
> resources of its own it isn't safe to assert on absolute stats anymore. Null
> context should be fine. Is there a reason I'm not seeing that this test needs
> real rendering contexts?

It still passes because we don't trigger those creations in this test? I don't
think making this run on the null GL context will really help at all because it
still has a GrGLGpu which can create (null) resources of its own.

Perhaps this test should track deltas rather then absolute counts?

One thing I've I wanted to do for a long time is beef up the MockContext to
actually be functional (have Mock resource classes). Since it uses its own
MockGpu subclass of GrGpu it would circumvent this problem, but I don't see
anyone doing it anytime soon.

Also, I imagine when Rob finishes the work to have separate DrawTargets for
different DrawContexts this test can be deleted.

[Chris Dalton, 2016-04-06 21:06:41]

On 2016/04/06 20:58:06, bsalomon wrote:
>
https://codereview.chromium.org/1854283004/diff/80001/tests/GpuLayerCacheTest...
> File tests/GpuLayerCacheTest.cpp (right):
> 
>
https://codereview.chromium.org/1854283004/diff/80001/tests/GpuLayerCacheTest...
> tests/GpuLayerCacheTest.cpp:110: DEF_GPUTEST_FOR_NULL_CONTEXT(GpuLayerCache,
> reporter, ctxInfo) {
> On 2016/04/06 20:06:21, Chris Dalton wrote:
> > This technically isn't a problem right now, but since the GrGLGpu can now
> create
> > resources of its own it isn't safe to assert on absolute stats anymore. Null
> > context should be fine. Is there a reason I'm not seeing that this test
needs
> > real rendering contexts?
> 
> It still passes because we don't trigger those creations in this test? I don't
> think making this run on the null GL context will really help at all because
it
> still has a GrGLGpu which can create (null) resources of its own.
> 
> Perhaps this test should track deltas rather then absolute counts?
> 
> One thing I've I wanted to do for a long time is beef up the MockContext to
> actually be functional (have Mock resource classes). Since it uses its own
> MockGpu subclass of GrGpu it would circumvent this problem, but I don't see
> anyone doing it anytime soon.
> 
> Also, I imagine when Rob finishes the work to have separate DrawTargets for
> different DrawContexts this test can be deleted.

Oh, right. Duh. Undid the test change. Like I said, it's just a hypothetical
issue right now and since this test can be deleted soonish it doesn't seem like
a problem.

[Chris Dalton, 2016-04-06 21:08:00]

Re-committing now that the null context is fixed.

[Chris Dalton, 2016-04-06 21:10:48]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-04-06 21:10:49]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1854283004/#ps100001
(title: "undo layer test change")

[commit-bot: I haz the power, 2016-04-06 21:10:59]

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

[commit-bot: I haz the power, 2016-04-06 21:26:36]

Description was changed from

==========
Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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

to

==========
Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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/+/deacc97bc63513b5eacaf21f858727f6e8b98ce5
==========

[commit-bot: I haz the power, 2016-04-06 21:26:37]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/deacc97bc63513b5eacaf21f858727f6e8b98ce5

[mtklein, 2016-04-07 01:24:18]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1870553002/ by mtklein@google.com.

The reason for reverting is: Chrome roll's broken, seems to be missing fTarget:

https://codereview.chromium.org/1861473005.

[Chris Dalton, 2016-04-07 02:06:50]

Description was changed from

==========
Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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/+/deacc97bc63513b5eacaf21f858727f6e8b98ce5
==========

to

==========
Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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/+/deacc97bc63513b5eacaf21f858727f6e8b98ce5
==========

[Chris Dalton, 2016-04-07 02:07:37]

This should fix the chrome issue

[Chris Dalton, 2016-04-07 16:38:40]

On 2016/04/07 02:07:37, Chris Dalton wrote:
> This should fix the chrome issue

Can I go ahead and resubmit now that the chrome issue is resolved?

[Chris Dalton, 2016-04-07 19:29:41]

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

[commit-bot: I haz the power, 2016-04-07 19:29:53]

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

[commit-bot: I haz the power, 2016-04-07 19:39:56]

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

[commit-bot: I haz the power, 2016-04-07 19:39:57]

Dry run: This issue passed the CQ dry run.

[Chris Dalton, 2016-04-08 01:11:54]

Re-landing before it ends up needing a rebase.

[Chris Dalton, 2016-04-08 01:12:06]

The CQ bit was checked by cdalton@nvidia.com

[Chris Dalton, 2016-04-08 01:12:06]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1854283004/#ps120001
(title: "fix for GR_GL_USE_BUFFER_DATA_NULL_HINT=0")

[commit-bot: I haz the power, 2016-04-08 01:12:13]

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

[commit-bot: I haz the power, 2016-04-08 01:13:42]

Description was changed from

==========
Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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/+/deacc97bc63513b5eacaf21f858727f6e8b98ce5
==========

to

==========
Track GL buffer state based on unique resource ID

Reworks GrGLGpu to track GL buffer state based on the unique
GrGpuResource ID. This eliminates the need to notify the gpu object
whenever a buffer is deleted.

This change also allows us to remove the type specifier from GrBuffer.
At this point a buffer is just a chunk of memory, and the type
given at creation time is just a suggestion to the GL backend about
which target to bind to for updates.

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/+/deacc97bc63513b5eacaf21f858727f6e8b98ce5

Committed:
https://skia.googlesource.com/skia/+/e2e71c2df4e72e897bbe745752be0444aee5c29f
==========

[commit-bot: I haz the power, 2016-04-08 01:13:43]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/e2e71c2df4e72e897bbe745752be0444aee5c29f