Add tracing for GL texture objects

Adds tracing support for GL textures in both CC (via ResourceProvider) and GPU (via TextureManager). Does not currently log all renderer textures, as we still need a follow up patch to account for Skia allocations.

BUG=512534
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f08855b468d24b9cc4752107b50e5763daecbf56
Cr-Commit-Position: refs/heads/master@{#342020}

[ericrk, 2015-07-23 19:06:51]

Patchset #1 (id:1) has been deleted

[ericrk, 2015-07-23 19:06:56]

Patchset #1 (id:20001) has been deleted

[ericrk, 2015-07-23 19:07:01]

Patchset #1 (id:30001) has been deleted

[ericrk, 2015-07-23 20:28:17]

ericrk@chromium.org changed reviewers:
+ reveman@chromium.org

[ericrk, 2015-07-23 20:29:49]

ericrk@chromium.org changed reviewers:
+ primiano@chromium.org

[ericrk, 2015-07-24 00:26:16]

Patchset #2 (id:70001) has been deleted

[ericrk, 2015-07-24 20:31:18]

Patchset #1 (id:50001) has been deleted

[ericrk, 2015-07-24 20:31:23]

Patchset #1 (id:90001) has been deleted

[ericrk, 2015-07-24 20:31:29]

Patchset #1 (id:110001) has been deleted

[ericrk, 2015-07-24 20:32:06]

ericrk@chromium.org changed reviewers:
+ ssid@chromium.org
- primiano@chromium.org

[ericrk, 2015-07-24 20:34:22]

Sending out a first pass of this to the tracing folks before broadening the CL
to GPU owners.

[reveman, 2015-07-24 21:16:36]

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:1157: this);
You'll need to provide a ThreadTaskRunnerHandle here to be sure that this is
called on the correct thread and there's no race at shutdown.

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:1967: // We currently only support memory
dumping GL resources.
FYI, we should dump memory usage of shared bitmaps too and it's not very hard to
add as a browser side memory dump provider for that already exists.

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:1968: if (default_resource_type_ ==
RESOURCE_TYPE_GL_TEXTURE) {
is the resource.gl_id check below not enough? what if we have some resources
that were not created using the default type?

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:1981: base::StringPrintf("gl/CC/textures/%d",
resource.gl_id);
I think the naming convention I used in my one-copy patch is better. That is to
use a dump name that describes the usage rather than the type of memory used.
Something like:

StringPrintf("cc/resource_memory/resource%d", resource_entry.first)

this way we can use the same name for gpu-memory-buffer and shared-bitmap backed
resources.

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:2001: tracing_process_id, resource.gl_id));
This is not going to be correct if the resource is backed by a GpuMemoryBuffer.
You need a gpumemorybuffer-x-process guid in that case.

https://codereview.chromium.org/1256613002/diff/130001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/130001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1384: this);
You'll need to provide a ThreadTaskRunnerHandle here.

https://codereview.chromium.org/1256613002/diff/130001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2066: "gl/process_%" PRIx64
"/textures/%d", tracing_id, ref->client_id());
s/%" PRIx64 "/%d/

nit: "gl/textures/client_%d/texture%d" as that would be more consistent with how
we report memory for gpu-memory-buffers

https://codereview.chromium.org/1256613002/diff/130001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2075: "gl-x-process/%" PRIx64
"/texture_%d", tracing_id, ref->client_id()));
Can you move this logic to where it can be shared between service and client?
Similar to what I did by adding GetGpuMemoryBufferGUIDForTracing to
ui/gfx/gpu_memory_buffer.h.

[ericrk, 2015-07-28 20:43:06]

Patchset #3 (id:170001) has been deleted

[ericrk, 2015-07-28 20:58:47]

Patchset #3 (id:190001) has been deleted

[ericrk, 2015-07-28 21:02:33]

Thanks for the feedback! let me know how this looks.

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:1157: this);
On 2015/07/24 21:16:35, reveman wrote:
> You'll need to provide a ThreadTaskRunnerHandle here to be sure that this is
> called on the correct thread and there's no race at shutdown.

Done. Also moved this above (we were skipping registration in the GL case.

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:1967: // We currently only support memory
dumping GL resources.
On 2015/07/24 21:16:36, reveman wrote:
> FYI, we should dump memory usage of shared bitmaps too and it's not very hard
to
> add as a browser side memory dump provider for that already exists.

Added this for Shared Bitmaps and for GPU Memory Buffers.

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:1968: if (default_resource_type_ ==
RESOURCE_TYPE_GL_TEXTURE) {
On 2015/07/24 21:16:36, reveman wrote:
> is the resource.gl_id check below not enough? what if we have some resources
> that were not created using the default type?

True - now that we log all types this no longer applies.

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:1981: base::StringPrintf("gl/CC/textures/%d",
resource.gl_id);
On 2015/07/24 21:16:36, reveman wrote:
> I think the naming convention I used in my one-copy patch is better. That is
to
> use a dump name that describes the usage rather than the type of memory used.
> Something like:
> 
> StringPrintf("cc/resource_memory/resource%d", resource_entry.first)
> 
> this way we can use the same name for gpu-memory-buffer and shared-bitmap
backed
> resources.

Makes sense.

https://codereview.chromium.org/1256613002/diff/130001/cc/resources/resource_...
cc/resources/resource_provider.cc:2001: tracing_process_id, resource.gl_id));
On 2015/07/24 21:16:36, reveman wrote:
> This is not going to be correct if the resource is backed by a
GpuMemoryBuffer.
> You need a gpumemorybuffer-x-process guid in that case.

Done.

https://codereview.chromium.org/1256613002/diff/130001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/130001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:1384: this);
On 2015/07/24 21:16:36, reveman wrote:
> You'll need to provide a ThreadTaskRunnerHandle here.

Done.

https://codereview.chromium.org/1256613002/diff/130001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2066: "gl/process_%" PRIx64
"/textures/%d", tracing_id, ref->client_id());
On 2015/07/24 21:16:36, reveman wrote:
> s/%" PRIx64 "/%d/
> 
> nit: "gl/textures/client_%d/texture%d" as that would be more consistent with
how
> we report memory for gpu-memory-buffers

Re-worked this to be consistent with gpu-memory-buffers.

https://codereview.chromium.org/1256613002/diff/130001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2075: "gl-x-process/%" PRIx64
"/texture_%d", tracing_id, ref->client_id()));
On 2015/07/24 21:16:36, reveman wrote:
> Can you move this logic to where it can be shared between service and client?
> Similar to what I did by adding GetGpuMemoryBufferGUIDForTracing to
> ui/gfx/gpu_memory_buffer.h.

Done.

https://codereview.chromium.org/1256613002/diff/210001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/BUILD.gn#newcode528
cc/BUILD.gn:528: "//ui/gl",
put the GL object GUID generation stuff in ui/gl/trace_util.h, not sure if
that's the best
place for it? seems like it could go in gpu? or maybe even cc?

It just operates on GLuints, so it doesn't really have a specific object's .h to
live in.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
File cc/resources/shared_bitmap.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
cc/resources/shared_bitmap.cc:75: "gpumemorybuffer-x-process/%s",
bitmap_id_hex.c_str()));
These IDs are globally unique already right? SharedBitmapManager doesn't track
clients/child process IDs directly, so we'd need some more plumbing if that is
required.

https://codereview.chromium.org/1256613002/diff/210001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:300: if (client_id ==
gpu_client_id_) {
A bit of a hack... we're hitting a similar issue to what we hit with GPU
process... we could track a tracing ID per client which could be provided by the
clients, but as we don't register all clients with this class up front it's a
bit roundabout...

[reveman, 2015-07-28 21:38:50]

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:1976:
base::StringPrintf("CC/resource_memory/resource_%d", id);
nit: I'm using lower case "cc/..." in one-copy dump provider and having this be
consistent is useful when trying to determine the total amount of memory used by
cc/

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2014: // process.
Not just the GPU process but also the browser process. Maybe say "...to share
this allocation with other processes."

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2015: auto guid =
gfx::GetSharedBitmapGUIDForTracing(
SharedBitmap? shouldn't this be gfx::GetGpuMemoryBufferGUIDForTracing?

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2024: // process.
Not the GPU process. Only the browser process in case of shared bitmaps.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2026:
GetSharedMemoryBufferGUIDForTracing(resource.shared_bitmap->id());
what's GetSharedMemoryBufferGUIDForTracing? is this not where we should be using
gfx::GetSharedBitmapGUIDForTracing?

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.h:581: const ResourceId& id,
nit: no need to pass the id by reference

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
File cc/resources/shared_bitmap.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
cc/resources/shared_bitmap.cc:75: "gpumemorybuffer-x-process/%s",
bitmap_id_hex.c_str()));
s/gpumemorybuffer-x-process/sharedbitmap-x-process/

https://codereview.chromium.org/1256613002/diff/210001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:300: if (client_id ==
gpu_client_id_) {
On 2015/07/28 at 21:02:33, ericrk wrote:
> A bit of a hack... we're hitting a similar issue to what we hit with GPU
> process... we could track a tracing ID per client which could be provided by
the
> clients, but as we don't register all clients with this class up front it's a
> bit roundabout...

Can you add a GpuClientIdToTracingProcessId function that takes care of this
special case?

https://codereview.chromium.org/1256613002/diff/210001/content/common/host_sh...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/content/common/host_sh...
content/common/host_shared_bitmap_manager.cc:160: "%s/%s", kMemoryAllocatorName,
while here, can you remove the kMemoryAllocatorName constant and just put the
name in here? more consistent with other memory dump providers.

https://codereview.chromium.org/1256613002/diff/210001/content/common/host_sh...
content/common/host_shared_bitmap_manager.cc:171: // process.
not shared with GPU process. only shared with renderer processes.

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2076: auto guid =
gfx::GetGLTextureGUIDForTracing(
what if the texture is backed by a GLImage? I think we need some GLImage API to
report that correctly as the texture might be gpumemorybuffer backed or both
texture and gpumemorybuffer backed depending on the GLImage implementation.

https://codereview.chromium.org/1256613002/diff/210001/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/1256613002/diff/210001/ui/gl/BUILD.gn#newcode106
ui/gl/BUILD.gn:106: "trace_util.h",
did you forget to add these files?

[ericrk, 2015-07-28 21:38:58]

Patchset #4 (id:230001) has been deleted

[ericrk, 2015-07-28 21:40:42]

Patchset #4 (id:250001) has been deleted

[ssid, 2015-07-29 11:17:44]

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2015: auto guid =
gfx::GetSharedBitmapGUIDForTracing(
On 2015/07/28 21:38:49, reveman wrote:
> SharedBitmap? shouldn't this be gfx::GetGpuMemoryBufferGUIDForTracing?

+1

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2021: DumpForResource(pmd,
resource_entry.first, resource_entry.second);
This can be moved out of the if/else. It seems common. I also think the function
DumpForResource can be removed and the 2 lines moved in this method.
This might be easier to read the code, instead of moving between functions.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2026:
GetSharedMemoryBufferGUIDForTracing(resource.shared_bitmap->id());
I can't find this function?

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2036:
gfx::GetGLTextureGUIDForTracing(tracing_process_id, resource.gl_id);
Am i missing a dependent cl?

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2038: pmd->AddOwnershipEdge(dump->guid(),
guid, kImportance);
I think this memory is shared between processes. In that case just adding
ownership edge does not help, the dump with the guid must be actually created in
both the processes.
You need to add pmd->CreateSharedGlobalAllocatorDump(guid); for the ownership
edge to be valid.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
File cc/resources/shared_bitmap.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
cc/resources/shared_bitmap.cc:73: auto bitmap_id_hex =
base::HexEncode(bitmap_id.name, sizeof(bitmap_id.name));
Please add a line  pmd->CreateSharedGlobalAllocatorDump(created_guid); here, as
I said in the other comment.

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2042: // Only dump memory info for
textures actually owned by this TextureManager.
textures 'is'?

[petrcermak, 2015-07-29 16:13:33]

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

[petrcermak, 2015-07-29 16:13:35]

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2017: const int kImportance = 2;
This constant is defined three times within this function. Could it be a global
constant within this file?

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2018: pmd->AddOwnershipEdge(dump->guid(),
guid, kImportance);
This call is also repeated three times withing this function. I'd factor it out
of the if-statement.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
File cc/resources/shared_bitmap.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
cc/resources/shared_bitmap.cc:75: "gpumemorybuffer-x-process/%s",
bitmap_id_hex.c_str()));
On 2015/07/28 21:02:32, ericrk wrote:
> These IDs are globally unique already right? SharedBitmapManager doesn't track
> clients/child process IDs directly, so we'd need some more plumbing if that is
> required.

Yest, the IDs are globally unique.

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:87: uint64_t ClientTracingId()
override { return client_tracing_id_; }
const?

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:88: int ClientId() override {
return client_id_; }
ditto

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:93: uint64_t client_tracing_id_;
const?

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:94: int client_id_;
ditto

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
File gpu/command_buffer/service/memory_tracking.h (right):

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_tracking.h:35: virtual uint64_t
ClientTracingId() = 0;
I think this should be const.

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_tracking.h:38: virtual int ClientId() = 0;
ditto

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2042: // Only dump memory info for
textures actually owned by this TextureManager.
On 2015/07/29 11:17:44, ssid wrote:
> textures 'is'?

I think it's correct as is.

[ericrk, 2015-07-29 18:46:47]

Thanks for all the detailed feedback - cleaned things up quite a bit - let me
know how this looks.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:1976:
base::StringPrintf("CC/resource_memory/resource_%d", id);
On 2015/07/28 21:38:49, reveman wrote:
> nit: I'm using lower case "cc/..." in one-copy dump provider and having this
be
> consistent is useful when trying to determine the total amount of memory used
by
> cc/

Done.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2014: // process.
On 2015/07/28 21:38:49, reveman wrote:
> Not just the GPU process but also the browser process. Maybe say "...to share
> this allocation with other processes."

Done.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2015: auto guid =
gfx::GetSharedBitmapGUIDForTracing(
On 2015/07/28 21:38:49, reveman wrote:
> SharedBitmap? shouldn't this be gfx::GetGpuMemoryBufferGUIDForTracing?

my bad, did a re-name right at the end and replaced the wrong thing.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2017: const int kImportance = 2;
On 2015/07/29 16:13:34, petrcermak wrote:
> This constant is defined three times within this function. Could it be a
global
> constant within this file?

refactored this section - should be a lot nicer now.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2018: pmd->AddOwnershipEdge(dump->guid(),
guid, kImportance);
On 2015/07/29 16:13:34, petrcermak wrote:
> This call is also repeated three times withing this function. I'd factor it
out
> of the if-statement.

refactored.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2021: DumpForResource(pmd,
resource_entry.first, resource_entry.second);
On 2015/07/29 11:17:43, ssid wrote:
> This can be moved out of the if/else. It seems common. I also think the
function
> DumpForResource can be removed and the 2 lines moved in this method.
> This might be easier to read the code, instead of moving between functions.

Done.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2024: // process.
On 2015/07/28 21:38:49, reveman wrote:
> Not the GPU process. Only the browser process in case of shared bitmaps.

refactored.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2026:
GetSharedMemoryBufferGUIDForTracing(resource.shared_bitmap->id());
On 2015/07/29 11:17:43, ssid wrote:
> I can't find this function?

meant to replace this via rename, replaced the wrong thing.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2036:
gfx::GetGLTextureGUIDForTracing(tracing_process_id, resource.gl_id);
On 2015/07/29 11:17:43, ssid wrote:
> Am i missing a dependent cl?

added missing files.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.cc:2038: pmd->AddOwnershipEdge(dump->guid(),
guid, kImportance);
On 2015/07/29 11:17:43, ssid wrote:
> I think this memory is shared between processes. In that case just adding
> ownership edge does not help, the dump with the guid must be actually created
in
> both the processes.
> You need to add pmd->CreateSharedGlobalAllocatorDump(guid); for the ownership
> edge to be valid.

Interestingly, this was working - probably because we add the
SharedGlobalAllocatorDump for the same GUID in texture_manager.cc?

Added this.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/resource_...
cc/resources/resource_provider.h:581: const ResourceId& id,
On 2015/07/28 21:38:49, reveman wrote:
> nit: no need to pass the id by reference

removed this fn.

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
File cc/resources/shared_bitmap.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
cc/resources/shared_bitmap.cc:73: auto bitmap_id_hex =
base::HexEncode(bitmap_id.name, sizeof(bitmap_id.name));
On 2015/07/29 11:17:43, ssid wrote:
> Please add a line  pmd->CreateSharedGlobalAllocatorDump(created_guid); here,
as
> I said in the other comment.

We've currently been doing this at the call-site (added it to
resource_provider.cc... we could re-factor all of these to do it at GUID
generation... might be cleaner - what do you think?

https://codereview.chromium.org/1256613002/diff/210001/cc/resources/shared_bi...
cc/resources/shared_bitmap.cc:75: "gpumemorybuffer-x-process/%s",
bitmap_id_hex.c_str()));
On 2015/07/28 21:38:49, reveman wrote:
> s/gpumemorybuffer-x-process/sharedbitmap-x-process/

Done.

https://codereview.chromium.org/1256613002/diff/210001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:300: if (client_id ==
gpu_client_id_) {
On 2015/07/28 21:38:49, reveman wrote:
> On 2015/07/28 at 21:02:33, ericrk wrote:
> > A bit of a hack... we're hitting a similar issue to what we hit with GPU
> > process... we could track a tracing ID per client which could be provided by
> the
> > clients, but as we don't register all clients with this class up front it's
a
> > bit roundabout...
> 
> Can you add a GpuClientIdToTracingProcessId function that takes care of this
> special case?

Added a function ClientIdToTracingProcessId which I think encapsulates this
cleanly. Let me know.

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:87: uint64_t ClientTracingId()
override { return client_tracing_id_; }
On 2015/07/29 16:13:34, petrcermak wrote:
> const?

Done.

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:88: int ClientId() override {
return client_id_; }
On 2015/07/29 16:13:34, petrcermak wrote:
> ditto

Done.

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:93: uint64_t client_tracing_id_;
On 2015/07/29 16:13:34, petrcermak wrote:
> const?

Done.

https://codereview.chromium.org/1256613002/diff/210001/content/common/gpu/gpu...
content/common/gpu/gpu_command_buffer_stub.cc:94: int client_id_;
On 2015/07/29 16:13:34, petrcermak wrote:
> ditto

Done.

https://codereview.chromium.org/1256613002/diff/210001/content/common/host_sh...
File content/common/host_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/content/common/host_sh...
content/common/host_shared_bitmap_manager.cc:160: "%s/%s", kMemoryAllocatorName,
On 2015/07/28 21:38:49, reveman wrote:
> while here, can you remove the kMemoryAllocatorName constant and just put the
> name in here? more consistent with other memory dump providers.

Done.

https://codereview.chromium.org/1256613002/diff/210001/content/common/host_sh...
content/common/host_shared_bitmap_manager.cc:171: // process.
On 2015/07/28 21:38:49, reveman wrote:
> not shared with GPU process. only shared with renderer processes.

yup, got a little to copy-pasty.

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
File gpu/command_buffer/service/memory_tracking.h (right):

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_tracking.h:35: virtual uint64_t
ClientTracingId() = 0;
On 2015/07/29 16:13:35, petrcermak wrote:
> I think this should be const.

Done.

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
gpu/command_buffer/service/memory_tracking.h:38: virtual int ClientId() = 0;
On 2015/07/29 16:13:35, petrcermak wrote:
> ditto

Done.

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/210001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2076: auto guid =
gfx::GetGLTextureGUIDForTracing(
On 2015/07/28 21:38:49, reveman wrote:
> what if the texture is backed by a GLImage? I think we need some GLImage API
to
> report that correctly as the texture might be gpumemorybuffer backed or both
> texture and gpumemorybuffer backed depending on the GLImage implementation.

Going to save this for a follow-up CL. Added a TODO. crbug.com/514914

https://codereview.chromium.org/1256613002/diff/210001/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/1256613002/diff/210001/ui/gl/BUILD.gn#newcode106
ui/gl/BUILD.gn:106: "trace_util.h",
On 2015/07/28 21:38:49, reveman wrote:
> did you forget to add these files?

yup.

[ericrk, 2015-07-29 18:46:48]

Thanks for all the detailed feedback - cleaned things up quite a bit - let me
know how this looks.

[ericrk, 2015-07-29 19:01:48]

Patchset #5 (id:290001) has been deleted

[reveman, 2015-07-29 19:41:20]

lgtm with nits

https://codereview.chromium.org/1256613002/diff/270001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/270001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:600:
->tracing_process_id();
nit: Please add a "const int gpu_client_tracing_id_" member that is set in the
ctor instead of calling this here. The comment can then be moved to the
BrowserGpuChannelHostFactory ctor where we have this logic already.

https://codereview.chromium.org/1256613002/diff/270001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:601: } else {
nit: maybe skip this else statement as you return above in the if scope

https://codereview.chromium.org/1256613002/diff/270001/ui/gl/trace_util.cc
File ui/gl/trace_util.cc (right):

https://codereview.chromium.org/1256613002/diff/270001/ui/gl/trace_util.cc#ne...
ui/gl/trace_util.cc:14: uint32_t client_id) {
texture_id

https://codereview.chromium.org/1256613002/diff/270001/ui/gl/trace_util.h
File ui/gl/trace_util.h (right):

https://codereview.chromium.org/1256613002/diff/270001/ui/gl/trace_util.h#new...
ui/gl/trace_util.h:15: uint32_t client_id);
nit: s/client_id/texture_id/

[ericrk, 2015-07-29 21:34:47]

Patchset #6 (id:330001) has been deleted

[ericrk, 2015-07-29 21:37:45]

ericrk@chromium.org changed reviewers:
+ piman@chromium.org

[ericrk, 2015-07-29 21:37:46]

Thanks for the feedback. Made the suggested changes.

+piman for GPU portions

https://codereview.chromium.org/1256613002/diff/270001/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/270001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:600:
->tracing_process_id();
On 2015/07/29 19:41:19, reveman wrote:
> nit: Please add a "const int gpu_client_tracing_id_" member that is set in the
> ctor instead of calling this here. The comment can then be moved to the
> BrowserGpuChannelHostFactory ctor where we have this logic already.

good point. done.

https://codereview.chromium.org/1256613002/diff/270001/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:601: } else {
On 2015/07/29 19:41:19, reveman wrote:
> nit: maybe skip this else statement as you return above in the if scope

Done.

https://codereview.chromium.org/1256613002/diff/270001/ui/gl/trace_util.cc
File ui/gl/trace_util.cc (right):

https://codereview.chromium.org/1256613002/diff/270001/ui/gl/trace_util.cc#ne...
ui/gl/trace_util.cc:14: uint32_t client_id) {
On 2015/07/29 19:41:19, reveman wrote:
> texture_id

Done.

https://codereview.chromium.org/1256613002/diff/270001/ui/gl/trace_util.h
File ui/gl/trace_util.h (right):

https://codereview.chromium.org/1256613002/diff/270001/ui/gl/trace_util.h#new...
ui/gl/trace_util.h:15: uint32_t client_id);
On 2015/07/29 19:41:19, reveman wrote:
> nit: s/client_id/texture_id/

Done.

[reveman, 2015-07-29 21:59:04]

https://codereview.chromium.org/1256613002/diff/320022/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/1256613002/diff/320022/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.h:25: explicit
BrowserGpuMemoryBufferManager(int gpu_client_id,
nit: 'explicit' is no longer needed

[ericrk, 2015-07-29 22:50:15]

https://codereview.chromium.org/1256613002/diff/320022/content/browser/gpu/br...
File content/browser/gpu/browser_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/1256613002/diff/320022/content/browser/gpu/br...
content/browser/gpu/browser_gpu_memory_buffer_manager.h:25: explicit
BrowserGpuMemoryBufferManager(int gpu_client_id,
On 2015/07/29 21:59:04, reveman wrote:
> nit: 'explicit' is no longer needed

Done.

[petrcermak, 2015-07-30 08:03:11]

A few more drive-by nits.

Thanks,
Petr

https://codereview.chromium.org/1256613002/diff/360001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager_unittest.cc (right):

https://codereview.chromium.org/1256613002/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager_unittest.cc:24: uint64_t ClientTracingId()
override { return 0; }
nit: This should be const

https://codereview.chromium.org/1256613002/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager_unittest.cc:25: int ClientId() override {
return 0; }
ditto

https://codereview.chromium.org/1256613002/diff/360001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/360001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2090: int kImportance = 0;  //
default importance
nit: strictly speaking, this is not a constant (because you assign to it twice),
so I don't think it should start with "k". Just "importance"?

https://codereview.chromium.org/1256613002/diff/360001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2090: int kImportance = 0;  //
default importance
nit: The comment should start with a capital letter and end with a fullstop.

[ericrk, 2015-07-30 16:58:36]

https://codereview.chromium.org/1256613002/diff/360001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_manager_unittest.cc (right):

https://codereview.chromium.org/1256613002/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager_unittest.cc:24: uint64_t ClientTracingId()
override { return 0; }
On 2015/07/30 08:03:11, petrcermak wrote:
> nit: This should be const

Done.

https://codereview.chromium.org/1256613002/diff/360001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_manager_unittest.cc:25: int ClientId() override {
return 0; }
On 2015/07/30 08:03:11, petrcermak wrote:
> ditto

Done.

https://codereview.chromium.org/1256613002/diff/360001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/360001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2090: int kImportance = 0;  //
default importance
On 2015/07/30 08:03:11, petrcermak wrote:
> nit: The comment should start with a capital letter and end with a fullstop.

Done.

[ericrk, 2015-07-30 17:43:46]

Patchset #9 (id:400001) has been deleted

[ericrk, 2015-07-31 20:30:04]

On 2015/07/30 16:58:37, ericrk wrote:

ping for review of gpu/ ui/gl components - piman, can you take a look?

[piman, 2015-08-03 21:36:21]

lgtm

https://codereview.chromium.org/1256613002/diff/420001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/420001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2087:
gfx::GetGLTextureGUIDForTracing(0, ref->texture()->service_id());
This is fine as a first pass. For Android WebView, we actually have separate
service-side share groups, so to get accurate data, you'll need to ensure
uniqueness using, maybe, the GLShareGroup. Also, on top of that we have a
sharing mechanism between these share groups which involves EGLImages (see the
code in MailboxManagerSync). Maybe add a TODO for that part?

[ericrk, 2015-08-03 21:53:05]

ericrk@chromium.org changed reviewers:
+ erg@chromium.org

[ericrk, 2015-08-03 22:11:05]

+erg for command_buffer_driver.cc. Just adding stub methods to the
MemoryTrackerStub. Let me know if this looks OK.

Thanks!

https://codereview.chromium.org/1256613002/diff/420001/gpu/command_buffer/ser...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1256613002/diff/420001/gpu/command_buffer/ser...
gpu/command_buffer/service/texture_manager.cc:2087:
gfx::GetGLTextureGUIDForTracing(0, ref->texture()->service_id());
On 2015/08/03 21:36:21, piman (Very slow to review) wrote:
> This is fine as a first pass. For Android WebView, we actually have separate
> service-side share groups, so to get accurate data, you'll need to ensure
> uniqueness using, maybe, the GLShareGroup. Also, on top of that we have a
> sharing mechanism between these share groups which involves EGLImages (see the
> code in MailboxManagerSync). Maybe add a TODO for that part?

Done.

[Elliot Glaysher, 2015-08-03 22:14:32]

erg@chromium.org changed reviewers:
+ jam@chromium.org

[Elliot Glaysher, 2015-08-03 22:14:33]

redirecting to jam@, who has actually worked in this file.

[jam, 2015-08-04 15:18:06]

lgtm

https://codereview.chromium.org/1256613002/diff/460001/components/view_manage...
File components/view_manager/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1256613002/diff/460001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.cc:47: 
nit: no blank line since these are all overridden methods from the same
interface

[ericrk, 2015-08-04 19:06:00]

https://codereview.chromium.org/1256613002/diff/460001/components/view_manage...
File components/view_manager/gles2/command_buffer_driver.cc (right):

https://codereview.chromium.org/1256613002/diff/460001/components/view_manage...
components/view_manager/gles2/command_buffer_driver.cc:47: 
On 2015/08/04 15:18:06, jam wrote:
> nit: no blank line since these are all overridden methods from the same
> interface

Done.

[ericrk, 2015-08-04 19:06:04]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-04 19:06:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1256613002/#ps480001
(title: "nit: remove blank line")

[commit-bot: I haz the power, 2015-08-04 19:06:56]

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

[commit-bot: I haz the power, 2015-08-04 20:07:26]

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

[commit-bot: I haz the power, 2015-08-04 20:07:27]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ericrk, 2015-08-04 20:52:41]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-08-04 20:53:32]

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

[ericrk, 2015-08-05 01:20:20]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-08-05 03:14:46]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-08-05 03:15:09]

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

[commit-bot: I haz the power, 2015-08-05 07:05:47]

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

[commit-bot: I haz the power, 2015-08-05 07:05:48]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[ericrk, 2015-08-05 18:53:02]

Patchset #13 (id:500001) has been deleted

[ericrk, 2015-08-05 18:55:08]

Patchset #13 (id:520001) has been deleted

[ericrk, 2015-08-05 18:58:25]

ericrk@chromium.org changed reviewers:
+ boliu@chromium.org

[ericrk, 2015-08-05 18:58:26]

Workaround for Android WebView.

[ericrk, 2015-08-05 19:08:54]

re-did TextureManager memory dump registration to key off of memory_tracker_,
per offline conversation with Bo.

[boliu, 2015-08-05 19:11:33]

On 2015/08/05 19:08:54, ericrk wrote:
> re-did TextureManager memory dump registration to key off of memory_tracker_,
> per offline conversation with Bo.

lg2m

Context: android webview runs cc::surface + command buffer service on a shared
render thread with rest of android views. That thread is created and owned by
android, so there is no message loop.

[ericrk, 2015-08-05 19:27:45]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-05 19:27:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, jam@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1256613002/#ps580001
(title: "Key texture manager memory dump provider registration off of
memory_tracker_")

[commit-bot: I haz the power, 2015-08-05 19:28:44]

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

[ericrk, 2015-08-05 20:05:49]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-08-05 20:30:50]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-08-05 20:31:20]

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

[ericrk, 2015-08-05 20:42:13]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-08-05 20:55:39]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-08-05 20:57:24]

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

[commit-bot: I haz the power, 2015-08-05 23:27:54]

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

[commit-bot: I haz the power, 2015-08-05 23:27:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ericrk, 2015-08-05 23:37:20]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-08-05 23:38:16]

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

[commit-bot: I haz the power, 2015-08-06 00:20:30]

Committed patchset #15 (id:580001)

[commit-bot: I haz the power, 2015-08-06 00:21:12]

Patchset 15 (id:??) landed as
https://crrev.com/f08855b468d24b9cc4752107b50e5763daecbf56
Cr-Commit-Position: refs/heads/master@{#342020}