content: Add GpuMemoryBuffer MemoryDumpProvider implementation.

content: Add GpuMemoryBuffer MemoryDumpProvider implementation.

Includes a minor GpuMemoryBufferImpl refactor to provide the
manager with an estimated buffer size.

BUG=481203
Committed: https://crrev.com/84c4caf913fc2eb23f29784d1a9d72cdc06f9ab9
Cr-Commit-Position: refs/heads/master@{#327968}

[reveman, 2015-04-27 08:37:50]

reveman@chromium.org changed reviewers:
+ dcastagna@chromium.org, primiano@chromium.org

[reveman, 2015-04-27 08:37:50]

+primiano for memory tracing related changes to
content/browser/gpu/browser_gpu_memory_buffer_manager.*
+dcastagna for content/common/gpu/client/gpu_memory_buffer_impl.* changes

Note: This memory dump provider is not being registered as part of this patch
because the instance is created on the main thread but used (including DumpInto)
on the IO thread. Once RegisterDumpProvider takes a TaskRunner instance instead
of MemoryDumpProvider ctor we can register/unregister this allocator when
starting/stopping the IO thread.

[Daniele Castagna, 2015-04-27 19:07:47]

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.h:56: static bool
BufferSizeInBytes(const gfx::Size& size,
What about keeping GpuMemoryBufferImplSharedMemory::BufferSizeInBytes. This
method could be GpuMemoryBufferImpl::EstimateBufferSizeInBytes and would just
call GpuMemoryBufferImplSharedMemory::BufferSizeInBytes?
It'd make explicit what's going on.

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc (right):

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:129:
EXPECT_EQ(memcmp(static_cast<char*>(mapped_buffers[plane]) +
Not sure what you're testing here, just that copying data into the buffer
actually works?
It seems that memcpy is being tested here. :/

[reveman, 2015-04-27 19:30:30]

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.h:56: static bool
BufferSizeInBytes(const gfx::Size& size,
On 2015/04/27 at 19:07:47, Daniele Castagna wrote:
> What about keeping GpuMemoryBufferImplSharedMemory::BufferSizeInBytes. This
method could be GpuMemoryBufferImpl::EstimateBufferSizeInBytes and would just
call GpuMemoryBufferImplSharedMemory::BufferSizeInBytes?
> It'd make explicit what's going on.

I don't think we should have the base class depend on a derived class. I should
be able to remove GpuMemoryBufferImplSharedMemory without breaking the base
class..

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc (right):

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:129:
EXPECT_EQ(memcmp(static_cast<char*>(mapped_buffers[plane]) +
On 2015/04/27 at 19:07:47, Daniele Castagna wrote:
> Not sure what you're testing here, just that copying data into the buffer
actually works?
> It seems that memcpy is being tested here. :/

We're testing the storage and the memory mapping, just like before, but now
we're actually making sure that we can copy and read-back data for the full
buffer and not just the first row. This is a relatively useful test. Whether
we're using memcpy or some other method to write the data is not really
relevant.

It seemed like a good idea to improve this test while I was touching this code
but I can leave it for a follow up if you prefer?

[Daniele Castagna, 2015-04-27 19:43:56]

LGTM

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.h:56: static bool
BufferSizeInBytes(const gfx::Size& size,
On 2015/04/27 at 19:30:30, reveman wrote:
> On 2015/04/27 at 19:07:47, Daniele Castagna wrote:
> > What about keeping GpuMemoryBufferImplSharedMemory::BufferSizeInBytes. This
method could be GpuMemoryBufferImpl::EstimateBufferSizeInBytes and would just
call GpuMemoryBufferImplSharedMemory::BufferSizeInBytes?
> > It'd make explicit what's going on.
> 
> I don't think we should have the base class depend on a derived class. I
should be able to remove GpuMemoryBufferImplSharedMemory without breaking the
base class..

With this patch there might not be an explicit code dependency but logically the
base class is still depending on a particular implementation, hence why you
moved BufferSizeInBytes from GpuMemoryBufferImplSharedMemory to
GpuMemoryBufferImpl.

I'm completely fine with this code, I was just suggesting a way to make more
explicit what's going on.

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc (right):

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:129:
EXPECT_EQ(memcmp(static_cast<char*>(mapped_buffers[plane]) +
On 2015/04/27 at 19:30:30, reveman wrote:
> On 2015/04/27 at 19:07:47, Daniele Castagna wrote:
> > Not sure what you're testing here, just that copying data into the buffer
actually works?
> > It seems that memcpy is being tested here. :/
> 
> We're testing the storage and the memory mapping, just like before, but now
we're actually making sure that we can copy and read-back data for the full
buffer and not just the first row. This is a relatively useful test. Whether
we're using memcpy or some other method to write the data is not really
relevant.
> 
> It seemed like a good idea to improve this test while I was touching this code
but I can leave it for a follow up if you prefer?

Just looking at the code, and seeing
memcpy(target, pointer, size);
EXPECT_EQ(memcmp(target, pointer, size));

It's not really clear to me what could go wrong to make that test fail, and that
makes hard to understand what the test is about.

Maybe unmap and remap before the memcmp?

No need to change it here in this CL.

[reveman, 2015-04-27 20:28:09]

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.h:56: static bool
BufferSizeInBytes(const gfx::Size& size,
On 2015/04/27 at 19:43:56, Daniele Castagna wrote:
> On 2015/04/27 at 19:30:30, reveman wrote:
> > On 2015/04/27 at 19:07:47, Daniele Castagna wrote:
> > > What about keeping GpuMemoryBufferImplSharedMemory::BufferSizeInBytes.
This method could be GpuMemoryBufferImpl::EstimateBufferSizeInBytes and would
just call GpuMemoryBufferImplSharedMemory::BufferSizeInBytes?
> > > It'd make explicit what's going on.
> > 
> > I don't think we should have the base class depend on a derived class. I
should be able to remove GpuMemoryBufferImplSharedMemory without breaking the
base class..
> 
> With this patch there might not be an explicit code dependency but logically
the base class is still depending on a particular implementation, hence why you
moved BufferSizeInBytes from GpuMemoryBufferImplSharedMemory to
GpuMemoryBufferImpl.

That's not the case. BufferSizeInBytes returns a minimal size for the buffer. An
implementation can chose to use this or not. The SharedMemory implementation
does but it doesn't have to. Other implementations might use more memory (hence
the comment in the memorydumpprovider implementation about adding a factory
interface if necessary).

> 
> I'm completely fine with this code, I was just suggesting a way to make more
explicit what's going on.

Seems like a naming issue. Maybe MinimalBufferSizeInBytes/MinimalRowSizeInBytes
is better?

Note that "RowSize" is different from "stride" as "stride" can be negative but
"RowSize" cannot.

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc (right):

https://codereview.chromium.org/1055403010/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:129:
EXPECT_EQ(memcmp(static_cast<char*>(mapped_buffers[plane]) +
On 2015/04/27 at 19:43:56, Daniele Castagna wrote:
> On 2015/04/27 at 19:30:30, reveman wrote:
> > On 2015/04/27 at 19:07:47, Daniele Castagna wrote:
> > > Not sure what you're testing here, just that copying data into the buffer
actually works?
> > > It seems that memcpy is being tested here. :/
> > 
> > We're testing the storage and the memory mapping, just like before, but now
we're actually making sure that we can copy and read-back data for the full
buffer and not just the first row. This is a relatively useful test. Whether
we're using memcpy or some other method to write the data is not really
relevant.
> > 
> > It seemed like a good idea to improve this test while I was touching this
code but I can leave it for a follow up if you prefer?
> 
> Just looking at the code, and seeing
> memcpy(target, pointer, size);
> EXPECT_EQ(memcmp(target, pointer, size));
> 
> It's not really clear to me what could go wrong to make that test fail, and
that makes hard to understand what the test is about.

It could crash and it could fail if there's something wrong with how this memory
was mapped. e.g. the implementation didn't allocate enough memory for the
buffer.

I'll add comment to make this more clear.

> 
> Maybe unmap and remap before the memcmp?

Maybe. I'd prefer a separate test for that as I'm not sure we can require all
implementations work this way. A buffer queue implementation like SurfaceTexture
might return a new buffer on the second map.
 
> 
> No need to change it here in this CL.

Ok, cool but please don't hold back any suggestions if you feel like the current
code is less than ideal.

[reveman, 2015-04-28 19:14:30]

Latest patch will register/unregister the provider. PTAL.

[Primiano Tucci (use gerrit), 2015-04-28 20:13:08]

MemoryDump* stuff LGTM with just a couple of comments/notes.

https://codereview.chromium.org/1055403010/diff/80001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1055403010/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:188: for (auto&
client_it : clients_) {
Maybe these two could be "const" auto& (or is there some non-const accessor
there)?

https://codereview.chromium.org/1055403010/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:207:
dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameInnerSize,
Is it this really intending to be InnerSize (As opposite to OuterSize).
In my mind an allocator should offer either the OuterSize or both. In essence
it's mostly a deal of being aware or not of internal fragmentation / bookeeping
metadata etc.

If you did just because kNameInnerSize is what shows up in the UI... we just
fixed that and will be reversed in the next UI roll (i.e. kNameOuterSize will be
the thing that the UI will show by default in the summary).

[reveman, 2015-04-28 20:52:24]

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

[reveman, 2015-04-28 20:52:25]

+piman for content/

https://codereview.chromium.org/1055403010/diff/80001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/1055403010/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:188: for (auto&
client_it : clients_) {
On 2015/04/28 at 20:13:08, Primiano Tucci wrote:
> Maybe these two could be "const" auto& (or is there some non-const accessor
there)?

Done.

https://codereview.chromium.org/1055403010/diff/80001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:207:
dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameInnerSize,
On 2015/04/28 at 20:13:08, Primiano Tucci wrote:
> Is it this really intending to be InnerSize (As opposite to OuterSize).
> In my mind an allocator should offer either the OuterSize or both. In essence
it's mostly a deal of being aware or not of internal fragmentation / bookeeping
metadata etc.
> 
> If you did just because kNameInnerSize is what shows up in the UI... we just
fixed that and will be reversed in the next UI roll (i.e. kNameOuterSize will be
the thing that the UI will show by default in the summary).

I used InnerSize as we don't know what the internal fragmentation and possible
book keeping metadata is at this level. The comment above is related to this.

I changed it in latest patch as I think it's better to think of it as OuterSize
at this level and add another dump provider at the lower level with InnerSize
that matches the OuterSize here if necessary.

[reveman, 2015-04-30 19:02:28]

ping for content/browser/ owner review

[piman, 2015-05-01 18:30:39]

lgtm

[reveman, 2015-05-01 18:38:18]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-05-01 18:38:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1055403010/#ps150001
(title: "move register out of ifdef")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1055403010/150001

[commit-bot: I haz the power, 2015-05-01 20:10:30]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2015-05-01 20:11:30]

Patchset 9 (id:??) landed as
https://crrev.com/84c4caf913fc2eb23f29784d1a9d72cdc06f9ab9
Cr-Commit-Position: refs/heads/master@{#327968}