Add onMemoryDump to GrContext

Add onMemoryDump to GrContext

Adds an entry point to GrContext to allow enumeration and tracing of GPU resources
via the newly added SkTraceMemoryDump.

Plan is for Chrome to call this on each of its GrContexts.

Dumps both the total size of GPU resources, as well as the total purgeable size.

BUG=526261
Committed: https://skia.googlesource.com/skia/+/0a5fa484fd58d27088f8696bdc11c8cc8f2b4866

[ericrk, 2015-08-24 21:41:51]

ericrk@chromium.org changed reviewers:
+ bsalomon@chromium.org, reed@google.com

[ericrk, 2015-08-24 21:43:22]

Here's a first cut of an entry point to GrContext to allow Chrome to trace GPU
resources in use by Skia. Let me know how this looks.

[ericrk, 2015-08-26 16:19:12]

On 2015/08/24 at 21:43:22, ericrk wrote:
> Here's a first cut of an entry point to GrContext to allow Chrome to trace GPU
resources in use by Skia. Let me know how this looks.

friendly ping for review. Thanks!

[bsalomon, 2015-08-26 16:35:59]

bsalomon@google.com changed reviewers:
+ bsalomon@google.com

[bsalomon, 2015-08-26 16:36:00]

Hey Eric, sorry for the slow response. I was OOO yesterday and came back to some
fresh bugs on my plate! :) Overall this seems fine to me.

Curious about the name setMemoryDump... are we using that phrasing on the SW
side. I'd prefer something like "recordResources()" or something that indicates
that it will add GL (or VK) resource handles to the dumper.

Also, one thing to note is that some GrGpuResource instances actually wrap IDs
that the client provided us (e.g. tile textures). Does that matter for how
they're recorded?

https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h...
include/gpu/GrContext.h:366: void onMemoryDump(SkTraceMemoryDump*
traceMemoryDump) const;
Can we call this something like dumpMemory? We usually use "on" for private
virtual methods that implement non virtual public methods.

https://codereview.chromium.org/1313743002/diff/60001/src/gpu/GrGpuResource.cpp
File src/gpu/GrGpuResource.cpp (right):

https://codereview.chromium.org/1313743002/diff/60001/src/gpu/GrGpuResource.c...
src/gpu/GrGpuResource.cpp:70: setMemoryBacking(traceMemoryDump, dumpName);
this->

[ericrk, 2015-08-26 17:01:04]

On 2015/08/26 at 16:36:00, bsalomon wrote:
> Hey Eric, sorry for the slow response. I was OOO yesterday and came back to
some fresh bugs on my plate! :) Overall this seems fine to me.
> 
> Curious about the name setMemoryDump... are we using that phrasing on the SW
side. I'd prefer something like "recordResources()" or something that indicates
that it will add GL (or VK) resource handles to the dumper.
> 
> Also, one thing to note is that some GrGpuResource instances actually wrap IDs
that the client provided us (e.g. tile textures). Does that matter for how
they're recorded?
> 
> https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h
> File include/gpu/GrContext.h (right):
> 
>
https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h...
> include/gpu/GrContext.h:366: void onMemoryDump(SkTraceMemoryDump*
traceMemoryDump) const;
> Can we call this something like dumpMemory? We usually use "on" for private
virtual methods that implement non virtual public methods.
> 
>
https://codereview.chromium.org/1313743002/diff/60001/src/gpu/GrGpuResource.cpp
> File src/gpu/GrGpuResource.cpp (right):
> 
>
https://codereview.chromium.org/1313743002/diff/60001/src/gpu/GrGpuResource.c...
> src/gpu/GrGpuResource.cpp:70: setMemoryBacking(traceMemoryDump, dumpName);
> this->

No worries at all! thanks for the review!

When you wrote "setMemoryDump" above did you mean "setMemoryBacking" or
"onMemoryDump"? For "onMemoryDump", see my response to your other comment (I've
updated the name). For "setMemoryBacking", that's the name on the Skia interface
we added (SkTraceMemoryDump) - it sets backings for all sorts of things -
sometimes these are GL objects, but other times it calls it with something like
"malloc" to let our dumper know that the object was allocated with malloc.

Re. GrGpuResources that wrap IDs, that should be fine. When we setMemoryBacking
with the ID, it creates a GUID for the resource that will allow it to be
de-duped with regards to other uses of the same ID.

[ericrk, 2015-08-26 17:01:09]

https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h...
include/gpu/GrContext.h:366: void onMemoryDump(SkTraceMemoryDump*
traceMemoryDump) const;
On 2015/08/26 at 16:36:00, bsalomon wrote:
> Can we call this something like dumpMemory? We usually use "on" for private
virtual methods that implement non virtual public methods.

OnMemoryDump matches the name in chrome, but I'm happy with whatever you think
makes most sense for Skia. In the SW dumping code, we currently have
DumpMemoryStatistics, but that isn't checked in yet (CL here:
https://codereview.chromium.org/1319473003)

I'll switch to DumpMemoryStatistics to match our planned SW name - does that
work for you?

https://codereview.chromium.org/1313743002/diff/60001/src/gpu/GrGpuResource.cpp
File src/gpu/GrGpuResource.cpp (right):

https://codereview.chromium.org/1313743002/diff/60001/src/gpu/GrGpuResource.c...
src/gpu/GrGpuResource.cpp:70: setMemoryBacking(traceMemoryDump, dumpName);
On 2015/08/26 at 16:36:00, bsalomon wrote:
> this->

yup.

[bsalomon, 2015-08-26 17:05:51]

On 2015/08/26 17:01:04, ericrk wrote:
> On 2015/08/26 at 16:36:00, bsalomon wrote:
> > Hey Eric, sorry for the slow response. I was OOO yesterday and came back to
> some fresh bugs on my plate! :) Overall this seems fine to me.
> > 
> > Curious about the name setMemoryDump... are we using that phrasing on the SW
> side. I'd prefer something like "recordResources()" or something that
indicates
> that it will add GL (or VK) resource handles to the dumper.
> > 
> > Also, one thing to note is that some GrGpuResource instances actually wrap
IDs
> that the client provided us (e.g. tile textures). Does that matter for how
> they're recorded?
> > 
> >
https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h
> > File include/gpu/GrContext.h (right):
> > 
> >
>
https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h...
> > include/gpu/GrContext.h:366: void onMemoryDump(SkTraceMemoryDump*
> traceMemoryDump) const;
> > Can we call this something like dumpMemory? We usually use "on" for private
> virtual methods that implement non virtual public methods.
> > 
> >
>
https://codereview.chromium.org/1313743002/diff/60001/src/gpu/GrGpuResource.cpp
> > File src/gpu/GrGpuResource.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1313743002/diff/60001/src/gpu/GrGpuResource.c...
> > src/gpu/GrGpuResource.cpp:70: setMemoryBacking(traceMemoryDump, dumpName);
> > this->
> 
> No worries at all! thanks for the review!
> 
> When you wrote "setMemoryDump" above did you mean "setMemoryBacking" or
> "onMemoryDump"? 

Oops, I meant setMemoryBacking.

For "setMemoryBacking", that's the name on the Skia interface
> we added (SkTraceMemoryDump) - it sets backings for all sorts of things -
> sometimes these are GL objects, but other times it calls it with something
like
> "malloc" to let our dumper know that the object was allocated with malloc.

I see, then let's keep it as is. 

> 
> Re. GrGpuResources that wrap IDs, that should be fine. When we
setMemoryBacking
> with the ID, it creates a GUID for the resource that will allow it to be
> de-duped with regards to other uses of the same ID.

[bsalomon, 2015-08-26 17:08:56]

lgtm, I think you missed a couple subclasses though (GrGLRenderTarget and
GrGLTextureRenderTarget).

https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1313743002/diff/60001/include/gpu/GrContext.h...
include/gpu/GrContext.h:366: void onMemoryDump(SkTraceMemoryDump*
traceMemoryDump) const;
On 2015/08/26 17:01:09, ericrk wrote:
> On 2015/08/26 at 16:36:00, bsalomon wrote:
> > Can we call this something like dumpMemory? We usually use "on" for private
> virtual methods that implement non virtual public methods.
> 
> OnMemoryDump matches the name in chrome, but I'm happy with whatever you think
> makes most sense for Skia. In the SW dumping code, we currently have
> DumpMemoryStatistics, but that isn't checked in yet (CL here:
> https://codereview.chromium.org/1319473003)
> 
> I'll switch to DumpMemoryStatistics to match our planned SW name - does that
> work for you?

sgtm

[ssid, 2015-08-26 17:10:02]

ssid@chromium.org changed reviewers:
+ ssid@chromium.org

[ssid, 2015-08-26 17:10:03]

Just one concern. Looks good. Thanks!

https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:764:
//////////////////////////////////////////////////////////////////////////////
New line after this?

https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrGpuResource.cpp
File src/gpu/GrGpuResource.cpp (right):

https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrGpuResource.c...
src/gpu/GrGpuResource.cpp:58: SkString dumpName("skia/gpu_resources/resource_");
Is the plan to dump all the gpu resources in separate category as:
gpu/skia/...? I am not sure

[reed1, 2015-08-26 18:51:11]

reed@google.com changed required reviewers:
+ reed@google.com

[reed1, 2015-08-26 19:09:56]

reed@google.com changed required reviewers:
- reed@google.com

[ericrk, 2015-08-26 19:40:22]

Took a stab at adding this to GrGLTextureRenderTarget and GrGLRenderTarget. It
gets a bit ugly due to the need to track the texture/renderbuffer in the
GrGLRenderTarget separately. Let me know if this approach seems sane.

Won't commit this until I've done a bit more testing by filling out the chrome
side.

https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:764:
//////////////////////////////////////////////////////////////////////////////
On 2015/08/26 at 17:10:03, ssid wrote:
> New line after this?

done.

https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrGpuResource.cpp
File src/gpu/GrGpuResource.cpp (right):

https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrGpuResource.c...
src/gpu/GrGpuResource.cpp:58: SkString dumpName("skia/gpu_resources/resource_");
On 2015/08/26 at 17:10:03, ssid wrote:
> Is the plan to dump all the gpu resources in separate category as:
> gpu/skia/...? I am not sure

Pretty sure we want to dump this under skia/, similar to how we dump CC gl
resources under cc/. These resources are already counted in the GPU process as
just standard gpu/gl/..., so we already account for them in the GPU column. The
idea here is just to get more context into the Skia column.

[bsalomon, 2015-08-26 19:53:30]

On 2015/08/26 19:40:22, ericrk wrote:
> Took a stab at adding this to GrGLTextureRenderTarget and GrGLRenderTarget. It
> gets a bit ugly due to the need to track the texture/renderbuffer in the
> GrGLRenderTarget separately. Let me know if this approach seems sane.
> 

Long term we plan to stop bundling renderbuffers FBOs and textures in one object
so hopefully it gets better eventually. I think you forgot to add the cpp for
texturerendertarget.

> Won't commit this until I've done a bit more testing by filling out the chrome
> side.
> 
> https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrContext.cpp#n...
> src/gpu/GrContext.cpp:764:
> //////////////////////////////////////////////////////////////////////////////
> On 2015/08/26 at 17:10:03, ssid wrote:
> > New line after this?
> 
> done.
> 
>
https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrGpuResource.cpp
> File src/gpu/GrGpuResource.cpp (right):
> 
>
https://codereview.chromium.org/1313743002/diff/80001/src/gpu/GrGpuResource.c...
> src/gpu/GrGpuResource.cpp:58: SkString
dumpName("skia/gpu_resources/resource_");
> On 2015/08/26 at 17:10:03, ssid wrote:
> > Is the plan to dump all the gpu resources in separate category as:
> > gpu/skia/...? I am not sure
> 
> Pretty sure we want to dump this under skia/, similar to how we dump CC gl
> resources under cc/. These resources are already counted in the GPU process as
> just standard gpu/gl/..., so we already account for them in the GPU column.
The
> idea here is just to get more context into the Skia column.

[ericrk, 2015-09-01 22:39:50]

Added missing files from last upload.

[ssid, 2015-09-02 10:45:58]

https://codereview.chromium.org/1313743002/diff/180001/gyp/gpu.gypi
File gyp/gpu.gypi (right):

https://codereview.chromium.org/1313743002/diff/180001/gyp/gpu.gypi#newcode350
gyp/gpu.gypi:350: '<(skia_src_path)/gpu/gl/GrGLTextureRenderTarget.cpp',
Please add this file.

[ericrk, 2015-09-02 17:25:16]

Patchset #11 (id:200001) has been deleted

[ericrk, 2015-09-02 17:28:37]

https://codereview.chromium.org/1313743002/diff/180001/gyp/gpu.gypi
File gyp/gpu.gypi (right):

https://codereview.chromium.org/1313743002/diff/180001/gyp/gpu.gypi#newcode350
gyp/gpu.gypi:350: '<(skia_src_path)/gpu/gl/GrGLTextureRenderTarget.cpp',
On 2015/09/02 10:45:58, ssid wrote:
> Please add this file.

My bad - added now. Thanks!

[bsalomon, 2015-09-08 15:00:49]

Sorry for the slow review! Been OOO.

https://codereview.chromium.org/1313743002/diff/220001/src/gpu/gl/GrGLRenderT...
File src/gpu/gl/GrGLRenderTarget.cpp (right):

https://codereview.chromium.org/1313743002/diff/220001/src/gpu/gl/GrGLRenderT...
src/gpu/gl/GrGLRenderTarget.cpp:94: if (fMSColorRenderbufferID && fRTFBOID != 0)
{
A bug in similar logic was fixed recently in GrGLRenderTaret::init(). Can we add
a private method on GrGLRT to get the number of color color values per pixel to
share implementation? Or maybe a method that computes the memory size of the RT
if that can be shared?

[ericrk, 2015-09-14 21:55:54]

https://codereview.chromium.org/1313743002/diff/220001/src/gpu/gl/GrGLRenderT...
File src/gpu/gl/GrGLRenderTarget.cpp (right):

https://codereview.chromium.org/1313743002/diff/220001/src/gpu/gl/GrGLRenderT...
src/gpu/gl/GrGLRenderTarget.cpp:94: if (fMSColorRenderbufferID && fRTFBOID != 0)
{
On 2015/09/08 15:00:49, bsalomon wrote:
> A bug in similar logic was fixed recently in GrGLRenderTaret::init(). Can we
add
> a private method on GrGLRT to get the number of color color values per pixel
to
> share implementation? Or maybe a method that computes the memory size of the
RT
> if that can be shared?

This isn't quite what we want to do here... we only want to log the MSAA buffer
if it exists, not log the size of the entire object (MSAA buffer + texture
resolve target), so this logic doesn't match the logic in init. It doesn't even
match the number of samples calculated in init (intentionally).

I've factored this into a couple helper functions, which I think makes things
clearer... let me know what you think.

https://codereview.chromium.org/1313743002/diff/260001/gyp/gpu.gypi
File gyp/gpu.gypi (right):

https://codereview.chromium.org/1313743002/diff/260001/gyp/gpu.gypi#newcode139
gyp/gpu.gypi:139: '<(skia_src_path)/gpu/GrProcessorUnitTest.cpp',
unintentional rebase damage.

[bsalomon, 2015-09-15 12:56:48]

lgtm

https://codereview.chromium.org/1313743002/diff/260001/src/gpu/gl/GrGLRenderT...
File src/gpu/gl/GrGLRenderTarget.cpp (right):

https://codereview.chromium.org/1313743002/diff/260001/src/gpu/gl/GrGLRenderT...
src/gpu/gl/GrGLRenderTarget.cpp:47: fGpuMemorySize = totalSamples() *
totalBytesPerSample();
tint nit: this-> for the member funcs

https://codereview.chromium.org/1313743002/diff/260001/src/gpu/gl/GrGLRenderT...
src/gpu/gl/GrGLRenderTarget.cpp:160: size_t size = msaaSamples() *
totalBytesPerSample();
this->

[ericrk, 2015-09-15 21:03:56]

Patchset #14 (id:280001) has been deleted

[ericrk, 2015-09-15 21:09:55]

https://codereview.chromium.org/1313743002/diff/260001/src/gpu/gl/GrGLRenderT...
File src/gpu/gl/GrGLRenderTarget.cpp (right):

https://codereview.chromium.org/1313743002/diff/260001/src/gpu/gl/GrGLRenderT...
src/gpu/gl/GrGLRenderTarget.cpp:47: fGpuMemorySize = totalSamples() *
totalBytesPerSample();
On 2015/09/15 12:56:48, bsalomon wrote:
> tint nit: this-> for the member funcs

Done.

https://codereview.chromium.org/1313743002/diff/260001/src/gpu/gl/GrGLRenderT...
src/gpu/gl/GrGLRenderTarget.cpp:160: size_t size = msaaSamples() *
totalBytesPerSample();
On 2015/09/15 12:56:48, bsalomon wrote:
> this->

Done.

[ericrk, 2015-09-15 21:09:59]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-09-15 21:10:00]

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/1313743002/#ps320001
(title: "Small build fix")

[commit-bot: I haz the power, 2015-09-15 21:10:13]

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

[commit-bot: I haz the power, 2015-09-15 21:16:13]

Committed patchset #15 (id:320001) as
https://skia.googlesource.com/skia/+/0a5fa484fd58d27088f8696bdc11c8cc8f2b4866