Fix global dumps to be dumped only when shared memory is actually used

Fix global dumps to be dumped only when shared memory is actually used

Before this CL, a global shared memory dump was created with calling
CreateSharedGlobalAllocatorDump, regardless of what its actual backend
was. For example, gpu::Buffer's backend might be shared memory or might
not. This CL fixes this to create global shared dumps only when the
backend is actually shared memory.

BUG=604726
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[hajimehoshi, 2017-05-08 12:15:34]

Description was changed from

==========
Fix global dumps to be dumped only when shared memory is actually used

Before this CL, a global shared memory dump was created with calling
CreateSharedGlobalAllocatorDump, regardless of what its actual backend
was. For example, gpu::Buffer's backend might be shared memory or might
not. This CL fixes this to create global shared dumps only when the
backend is actually shared memory.

BUG=604726
TEST=gpu_unittests
==========

to

==========
Fix global dumps to be dumped only when shared memory is actually used

Before this CL, a global shared memory dump was created with calling
CreateSharedGlobalAllocatorDump, regardless of what its actual backend
was. For example, gpu::Buffer's backend might be shared memory or might
not. This CL fixes this to create global shared dumps only when the
backend is actually shared memory.

BUG=604726
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hajimehoshi, 2017-05-08 12:15:36]

Description was changed from

==========
Fix global dumps to be dumped only when shared memory is actually used

Before this CL, a global shared memory dump was created with calling
CreateSharedGlobalAllocatorDump, regardless of what its actual backend
was. For example, gpu::Buffer's backend might be shared memory or might
not. This CL fixes this to create global shared dumps only when the
backend is actually shared memory.

BUG=604726
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix global dumps to be dumped only when shared memory is actually used

Before this CL, a global shared memory dump was created with calling
CreateSharedGlobalAllocatorDump, regardless of what its actual backend
was. For example, gpu::Buffer's backend might be shared memory or might
not. This CL fixes this to create global shared dumps only when the
backend is actually shared memory.

BUG=604726
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hajimehoshi, 2017-05-08 12:16:16]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 12:16:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hajimehoshi, 2017-05-08 12:16:58]

Description was changed from

==========
Fix global dumps to be dumped only when shared memory is actually used

Before this CL, a global shared memory dump was created with calling
CreateSharedGlobalAllocatorDump, regardless of what its actual backend
was. For example, gpu::Buffer's backend might be shared memory or might
not. This CL fixes this to create global shared dumps only when the
backend is actually shared memory.

BUG=604726
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix global dumps to be dumped only when shared memory is actually used

Before this CL, a global shared memory dump was created with calling
CreateSharedGlobalAllocatorDump, regardless of what its actual backend
was. For example, gpu::Buffer's backend might be shared memory or might
not. This CL fixes this to create global shared dumps only when the
backend is actually shared memory.

BUG=604726
TEST=gpu_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hajimehoshi, 2017-05-08 12:16:58]

hajimehoshi@chromium.org changed reviewers:
+ primiano@chromium.org, reveman@chromium.org, ssid@chromium.org

[hajimehoshi, 2017-05-08 12:18:49]

hajimehoshi@chromium.org changed reviewers:
+ wolenetz@chromium.org

[hajimehoshi, 2017-05-08 12:19:46]

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

[hajimehoshi, 2017-05-08 12:19:57]

PTAL

[commit-bot: I haz the power, 2017-05-08 12:47:40]

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

[commit-bot: I haz the power, 2017-05-08 12:47:42]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[hajimehoshi, 2017-05-09 07:37:47]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 07:38:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 10:35:37]

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

[commit-bot: I haz the power, 2017-05-09 10:35:37]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-05-09 18:59:50]

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

[ssid, 2017-05-09 18:59:51]

Is it the case that we create memory dumps for non-existent memory or is it the
case that we are creating edges for something other than base::SharedMemory?

I don't think we should do this. For the memory that is not backed by gpu memory
we should still create global dump edges. The "CreateGlobalSharedMemory" name is
not specific to just base::SharedMemory, it is common for all globally shared
memory, which could be gpu memory or anything else too.
Maybe what we want here is if not backed by base::SharedMemory then create the
global edges?

+Eric

[hajimehoshi, 2017-05-10 08:47:53]

On 2017/05/09 18:59:51, ssid wrote:
> Is it the case that we create memory dumps for non-existent memory or is it
the
> case that we are creating edges for something other than base::SharedMemory?
> 
> I don't think we should do this. For the memory that is not backed by gpu
memory
> we should still create global dump edges. The "CreateGlobalSharedMemory" name
is
> not specific to just base::SharedMemory, it is common for all globally shared
> memory, which could be gpu memory or anything else too.
> Maybe what we want here is if not backed by base::SharedMemory then create the
> global edges?
> 
> +Eric

Regarding GpuMemoryBuffer, you're right, this is shared even when the back end
is not shared memory. What this CL does is wrong.

Regarding gpu::Buffer that is used by TransferBuffer, I think this is not shared
when MemoryBufferBacking is used.

[hajimehoshi, 2017-05-10 09:13:19]

hajimehoshi@chromium.org changed reviewers:
- piman@chromium.org, wolenetz@chromium.org

[hajimehoshi, 2017-05-10 09:13:23]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 09:13:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hajimehoshi, 2017-05-10 09:14:08]

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

[hajimehoshi, 2017-05-10 09:28:28]

On 2017/05/10 08:47:53, hajimehoshi wrote:
> On 2017/05/09 18:59:51, ssid wrote:
> > Is it the case that we create memory dumps for non-existent memory or is it
> the
> > case that we are creating edges for something other than base::SharedMemory?
> > 
> > I don't think we should do this. For the memory that is not backed by gpu
> memory
> > we should still create global dump edges. The "CreateGlobalSharedMemory"
name
> is
> > not specific to just base::SharedMemory, it is common for all globally
shared
> > memory, which could be gpu memory or anything else too.
> > Maybe what we want here is if not backed by base::SharedMemory then create
the
> > global edges?
> > 
> > +Eric
> 
> Regarding GpuMemoryBuffer, you're right, this is shared even when the back end
> is not shared memory. What this CL does is wrong.
> 
> Regarding gpu::Buffer that is used by TransferBuffer, I think this is not
shared
> when MemoryBufferBacking is used.

BTW, I did a similar thing before https://codereview.chromium.org/2575803002,
and we might need to reconsidered.

[commit-bot: I haz the power, 2017-05-10 11:58:08]

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

[commit-bot: I haz the power, 2017-05-10 11:58:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hajimehoshi, 2017-05-12 05:57:53]

ping ericrk

[ericrk, 2017-05-15 18:38:54]

On 2017/05/12 05:57:53, hajimehoshi wrote:
> ping ericrk

I agree with ssid@ that our current logging is not dependent on shared memory,
and we are likely behaving ~correctly as-is.

I think a lot of the confusion is in the naming of functions on
TransferBuffer/etc... We may want to rename some of these.

In general, GetBufferGUIDForTracing doesn't care if the allocation is shared
memory or not. For instance, in the case of an in-process command buffer, we may
share these across components without shared memory. In these cases, we still
want to generate global dumps.

In fact, the IDs being dumped here are *never* the shared memory IDs (and this
may be what's causing confusion). TransferBuffer::ShmId is surprisingly *not* a
shared memory ID but a gpu::Buffer domain-specific ID. This function should
probably be renamed.

Even if we dump Shared memory later, I think we'd want to leave these global
dumps of BufferGUIDs, at least in the non-shared-memory case, to resolve
cross-component reference to the same non-shared-memory transfer buffers.

So long term, we could probably do something like:

- Dump shared memory iff this is a shared memory allocation
- Dump a generic Buffer GUID if this is a non-shared-memory allocation

We may be able to fold these Buffer GUIDs into the other non-shared-memory GUIDs
discussed in crrev.com/2775423003

[hajimehoshi, 2017-05-16 06:14:57]

Thank your for your reply!

On 2017/05/15 18:38:54, ericrk wrote:
> On 2017/05/12 05:57:53, hajimehoshi wrote:
> > ping ericrk
> 
> I agree with ssid@ that our current logging is not dependent on shared memory,
> and we are likely behaving ~correctly as-is.
> 
> I think a lot of the confusion is in the naming of functions on
> TransferBuffer/etc... We may want to rename some of these.
> 
> In general, GetBufferGUIDForTracing doesn't care if the allocation is shared
> memory or not. For instance, in the case of an in-process command buffer, we
may
> share these across components without shared memory. In these cases, we still
> want to generate global dumps.

I was wondering how MemoryBufferBacking is shared between processes. I thought
they are not shared actually as far as I read the code.

> 
> In fact, the IDs being dumped here are *never* the shared memory IDs (and this
> may be what's causing confusion). TransferBuffer::ShmId is surprisingly *not*
a
> shared memory ID but a gpu::Buffer domain-specific ID. This function should
> probably be renamed.
> 
> Even if we dump Shared memory later, I think we'd want to leave these global
> dumps of BufferGUIDs, at least in the non-shared-memory case, to resolve
> cross-component reference to the same non-shared-memory transfer buffers.
> 
> So long term, we could probably do something like:
> 
> - Dump shared memory iff this is a shared memory allocation
> - Dump a generic Buffer GUID if this is a non-shared-memory allocation
> 
> We may be able to fold these Buffer GUIDs into the other non-shared-memory
GUIDs
> discussed in crrev.com/2775423003

[ericrk, 2017-05-17 06:02:04]

On 2017/05/16 06:14:57, hajimehoshi wrote:
> Thank your for your reply!
> 
> On 2017/05/15 18:38:54, ericrk wrote:
> > On 2017/05/12 05:57:53, hajimehoshi wrote:
> > > ping ericrk
> > 
> > I agree with ssid@ that our current logging is not dependent on shared
memory,
> > and we are likely behaving ~correctly as-is.
> > 
> > I think a lot of the confusion is in the naming of functions on
> > TransferBuffer/etc... We may want to rename some of these.
> > 
> > In general, GetBufferGUIDForTracing doesn't care if the allocation is shared
> > memory or not. For instance, in the case of an in-process command buffer, we
> may
> > share these across components without shared memory. In these cases, we
still
> > want to generate global dumps.
> 
> I was wondering how MemoryBufferBacking is shared between processes. I thought
> they are not shared actually as far as I read the code.

When we use MemoryBufferBacking (as opposed to SharedMemoryBufferBacking),
the data itself isn't shared between processes. However, it might be shared
between multiple components within the same process. These components still need
a way to alias memory, so we use CreateSharedGlobalAllocatorDump and edges.

Maybe we could create an ownership edge for these without the shared global
allocator dump? Although using the global allocator dump seems to keep the code
uniform between the cross-process and cross-thread cases, as well as allowing
different components to dump / alias the same memory without knowledge of
each-other.

ssid@, is there another technique we should be using in these cases where
there's
no cross-process sharing?

> 
> > 
> > In fact, the IDs being dumped here are *never* the shared memory IDs (and
this
> > may be what's causing confusion). TransferBuffer::ShmId is surprisingly
*not*
> a
> > shared memory ID but a gpu::Buffer domain-specific ID. This function should
> > probably be renamed.
> > 
> > Even if we dump Shared memory later, I think we'd want to leave these global
> > dumps of BufferGUIDs, at least in the non-shared-memory case, to resolve
> > cross-component reference to the same non-shared-memory transfer buffers.
> > 
> > So long term, we could probably do something like:
> > 
> > - Dump shared memory iff this is a shared memory allocation
> > - Dump a generic Buffer GUID if this is a non-shared-memory allocation
> > 
> > We may be able to fold these Buffer GUIDs into the other non-shared-memory
> GUIDs
> > discussed in crrev.com/2775423003

[ssid, 2017-05-18 23:13:14]

On 2017/05/17 06:02:04, ericrk wrote:
> On 2017/05/16 06:14:57, hajimehoshi wrote:
> > Thank your for your reply!
> > 
> > On 2017/05/15 18:38:54, ericrk wrote:
> > > On 2017/05/12 05:57:53, hajimehoshi wrote:
> > > > ping ericrk
> > > 
> > > I agree with ssid@ that our current logging is not dependent on shared
> memory,
> > > and we are likely behaving ~correctly as-is.
> > > 
> > > I think a lot of the confusion is in the naming of functions on
> > > TransferBuffer/etc... We may want to rename some of these.
> > > 
> > > In general, GetBufferGUIDForTracing doesn't care if the allocation is
shared
> > > memory or not. For instance, in the case of an in-process command buffer,
we
> > may
> > > share these across components without shared memory. In these cases, we
> still
> > > want to generate global dumps.
> > 
> > I was wondering how MemoryBufferBacking is shared between processes. I
thought
> > they are not shared actually as far as I read the code.
> 
> When we use MemoryBufferBacking (as opposed to SharedMemoryBufferBacking),
> the data itself isn't shared between processes. However, it might be shared
> between multiple components within the same process. These components still
need
> a way to alias memory, so we use CreateSharedGlobalAllocatorDump and edges.
> 
> Maybe we could create an ownership edge for these without the shared global
> allocator dump? Although using the global allocator dump seems to keep the
code
> uniform between the cross-process and cross-thread cases, as well as allowing
> different components to dump / alias the same memory without knowledge of
> each-other.
> 
> ssid@, is there another technique we should be using in these cases where
> there's
> no cross-process sharing?

Let's say cc/transfer_memory1 is the main allocator, and gpu/memory_buffer1 is a
component sharing the memory:
TransferMemory does not know anything about who is using the memory. It just
allocates when some component requests it.
The MemoryBuffer dump provider should do:

MemoryBuffer::OnMemoryDump() does:

dump = CreateAllocatorDump("gpu/memory_buffer1");
dump->AddSize(123);
// Now this component knows about transfer buffers and knows the id "1"
// given by the TransferMemory provider since it allocates memory from that.
target = pmd->GetOrCreateMemoryDump("cc/transfer_memory1");
pmd->AddOwnershipEdge(dump->guid(), target->guid());

TransferMemory::OnMemoryDump() does:
pmd->CreateAllocatorDump("cc/transfer_memory1");

// To get unique id for the other components.
TransferMemory::GetUniqueId() {
 return 1;
}

This would cause the memory of transfer buffer to be attributed to the
gpu/memory_buffer and effective size of transfer buffer will become 0.

I see 2 problems if we want to use the in-process ownership edges instead of
global dump edges:
1. We should be able to tell if the memory is shared in-process or not.
2. It would be inconsistent with the global dump behavior. It was just
consistent to add global dump edges to all cases and let Trace viewer handle
where the edges are added to (in-process or out of process).

Unrelated question:
How does gpu memory get shared (memory from gpu driver)? I'd assume it is not
shared memory? We might just miss edges because we account for only
base::SharedMemory.