Log both GL and cpu-side memory in GLImage*Memory

Log both GL and cpu-side memory in GLImage*Memory

Previously we would only log cpu-side memory when tracking
GLImage*Memory. Now we log both the cpu-side and GL side memory. To
disambiguate this in the traces, we log these as:

gpu/gl/textures/client_#/texture_#/face_#/level_#/texture_memory
gpu/gl/textures/client_#/texture_#/face_#/level_#/shared_memory

or

gpu/gl/textures/client_#/texture_#/face_#/level_#/texture_memory
gpu/gl/textures/client_#/texture_#/face_#/level_#/private_memory

BUG=514914
Committed: https://crrev.com/7ab865ae23faf16ff07d0f6498e6bfb7d21c7ab4
Cr-Commit-Position: refs/heads/master@{#346676}

[ericrk, 2015-08-28 17:51:29]

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

[reveman, 2015-08-28 18:18:44]

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:448: size_t size_in_bytes = egl_texture_id_ ?
SizeInBytes(size_, format_) : 0;
This is not enough. The gl(Compressed)TexImage2D call above will define the
texture this is bound too. For now I think it's enough to make this:

bool is_bound_to_texture = target_ && !need_do_bind_tex_image_;
size_t size_in_bytes = (egl_texture_id_ || is_bound_to_texture) ?
SizeInBytes(size_, format_) : 0;

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:451: pmd->CreateAllocatorDump(dump_name +
"/gl_texture");
nit: s/gl_texture/texture_memory/ as I think that's a bit more consistent

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.h
File ui/gl/gl_image_memory.h (right):

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.h...
ui/gl/gl_image_memory.h:57: void
DumpGLTexture(base::trace_event::ProcessMemoryDump* pmd,
nit: OnTextureMemoryDump maybe?

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_ref_coun...
File ui/gl/gl_image_ref_counted_memory.cc (right):

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_ref_coun...
ui/gl/gl_image_ref_counted_memory.cc:47: pmd->CreateAllocatorDump(dump_name +
"/ref_counted_memory");
nit: /private_memory ? although it doesn't matter much as this class is only
used for tests right now

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_ref_coun...
ui/gl/gl_image_ref_counted_memory.cc:50: static_cast<uint64_t>(size_in_bytes));
We should also add an add ownership edge to the system allocator as this is
malloc-ed memory.

[ericrk, 2015-08-28 23:27:54]

Patchset #3 (id:40001) has been deleted

[ericrk, 2015-08-31 17:19:32]

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:448: size_t size_in_bytes = egl_texture_id_ ?
SizeInBytes(size_, format_) : 0;
On 2015/08/28 at 18:18:43, reveman wrote:
> This is not enough. The gl(Compressed)TexImage2D call above will define the
texture this is bound too. For now I think it's enough to make this:
> 
> bool is_bound_to_texture = target_ && !need_do_bind_tex_image_;
> size_t size_in_bytes = (egl_texture_id_ || is_bound_to_texture) ?
SizeInBytes(size_, format_) : 0;

Re worked this to track the bound texture_id_ and ensure we don't switch. With
this logic, a simple check still works. PTAL.

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:451: pmd->CreateAllocatorDump(dump_name +
"/gl_texture");
On 2015/08/28 at 18:18:43, reveman wrote:
> nit: s/gl_texture/texture_memory/ as I think that's a bit more consistent

sgtm

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.h
File ui/gl/gl_image_memory.h (right):

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_memory.h...
ui/gl/gl_image_memory.h:57: void
DumpGLTexture(base::trace_event::ProcessMemoryDump* pmd,
On 2015/08/28 at 18:18:43, reveman wrote:
> nit: OnTextureMemoryDump maybe?

done.

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_ref_coun...
File ui/gl/gl_image_ref_counted_memory.cc (right):

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_ref_coun...
ui/gl/gl_image_ref_counted_memory.cc:47: pmd->CreateAllocatorDump(dump_name +
"/ref_counted_memory");
On 2015/08/28 at 18:18:44, reveman wrote:
> nit: /private_memory ? although it doesn't matter much as this class is only
used for tests right now

sgtm

https://codereview.chromium.org/1323593003/diff/20001/ui/gl/gl_image_ref_coun...
ui/gl/gl_image_ref_counted_memory.cc:50: static_cast<uint64_t>(size_in_bytes));
On 2015/08/28 at 18:18:43, reveman wrote:
> We should also add an add ownership edge to the system allocator as this is
malloc-ed memory.

done.

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.h
File ui/gl/gl_image_memory.h (right):

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.h...
ui/gl/gl_image_memory.h:52: void
OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
Mojo uses GLImageMemory directly (now I remember why I added it in in the first
place). That said, mojo provides memory from a GPU memory buffer, but no
additional data, preventing us from cross-correlating. We just log the GLTexture
component, as the rest will be logged with the GPUMemoryBuffer. We may want to
fix this later?

[reveman, 2015-08-31 17:38:48]

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:449: // Make sure the same image is only ever bound to
one texture.
I don't think any of this is needed as we will always release the image before
re-binding it. All we need to check afaict is that the image is not already
bound when trying to bind it.

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.h
File ui/gl/gl_image_memory.h (right):

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.h...
ui/gl/gl_image_memory.h:72: bool need_to_free_texture_id_;
I don't think you need these. See my comment below.

[ericrk, 2015-08-31 18:27:38]

Patchset #5 (id:100001) has been deleted

[ericrk, 2015-08-31 18:28:37]

Patchset #5 (id:120001) has been deleted

[ericrk, 2015-08-31 18:31:14]

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:449: // Make sure the same image is only ever bound to
one texture.
On 2015/08/31 at 17:38:48, reveman wrote:
> I don't think any of this is needed as we will always release the image before
re-binding it. All we need to check afaict is that the image is not already
bound when trying to bind it.

sgtm - added a comment and used the original logic you suggested.

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.h
File ui/gl/gl_image_memory.h (right):

https://codereview.chromium.org/1323593003/diff/80001/ui/gl/gl_image_memory.h...
ui/gl/gl_image_memory.h:72: bool need_to_free_texture_id_;
On 2015/08/31 at 17:38:48, reveman wrote:
> I don't think you need these. See my comment below.

removed / simplified.

[ericrk, 2015-08-31 21:38:37]

Patchset #6 (id:160001) has been deleted

[ericrk, 2015-08-31 22:12:56]

Patchset #6 (id:180001) has been deleted

[reveman, 2015-08-31 22:42:24]

lgtm

[ericrk, 2015-08-31 23:29:18]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-31 23:29:22]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-08-31 23:35:21]

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

[ericrk, 2015-08-31 23:35:22]

Hey Victor, can you take a look at the comment-only change to
gpu/command_buffer/service/texture_manager.cc

[vmiura, 2015-09-01 05:37:03]

On 2015/08/31 23:35:22, ericrk wrote:
> Hey Victor, can you take a look at the comment-only change to
> gpu/command_buffer/service/texture_manager.cc

Lgtm.

[ericrk, 2015-09-01 15:54:31]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-09-01 15:54:46]

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

[commit-bot: I haz the power, 2015-09-01 17:08:05]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2015-09-01 17:09:18]

Patchset 6 (id:??) landed as
https://crrev.com/7ab865ae23faf16ff07d0f6498e6bfb7d21c7ab4
Cr-Commit-Position: refs/heads/master@{#346676}