memory-infra: add ProcessType and expose data in RequestGlobalDump()

memory-infra: add ProcessType and expose data in RequestGlobalDump()

This CL addresses two distinct problems:
1) Adds a ProcessType in the per-process struct returned by
ProcessMemoryDump. This will be required in the next CLs for
UMA / UKM aggregation.

2) Adds a GlobalMemoryDump return object to the RequestGlobalMemoryDump
service API. This object will contain the data that UMA/UKM need to
upload.

As a bonus this CL removes the mojo type
memory_instrumentation.mojom.MemoryDumpCallbackResult.
That callback was never meant to be exposed across a service boundary.
It is only used between base::MemoryDumpManager and the client library,
which are by design both hosted in the same process.

BUG=703184
Review-Url: https://codereview.chromium.org/2871223002
Cr-Commit-Position: refs/heads/master@{#471415}
Committed: https://chromium.googlesource.com/chromium/src/+/5a7bd66da934d6decc3d60a8c0640a990fc0d0a8

[Primiano Tucci (use gerrit), 2017-05-10 14:36:26]

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

[commit-bot: I haz the power, 2017-05-10 14:36:58]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-10 14:48:50]

Description was changed from

==========
memory-infra: add ProcessType plumbing to the service

BUG=
==========

to

==========
memory-infra: add ProcessType and expose data in RequestGlobalDump

This CL addresses two distinct problems:
1) Adds a ProcessType in the per-process struct returned by ProcessMemoryDump.
This will be required in the next CLs for UMA / UKM aggregation.

2) Adds a GlobalMemoryDump return object to the RequestGlobalMemoryDump service
API.
This object will contain the data that UMA/UKM need to upload.

As a bonus this CL removes the
memory_instrumentation.mojom.MemoryDumpCallbackResult mojo
type. That callback was never meant to be exposed on a service boundary. That
callback is only
used between base::MemoryDumpManager and the client library, which are by design
both hosted
in the same process.

BUG=703184
==========

[Primiano Tucci (use gerrit), 2017-05-10 14:48:50]

primiano@chromium.org changed reviewers:
+ fmeawad@chromium.org, hjd@chromium.org

[Primiano Tucci (use gerrit), 2017-05-10 14:49:55]

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

[commit-bot: I haz the power, 2017-05-10 14:50:08]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-10 14:52:04]

Description was changed from

==========
memory-infra: add ProcessType and expose data in RequestGlobalDump

This CL addresses two distinct problems:
1) Adds a ProcessType in the per-process struct returned by ProcessMemoryDump.
This will be required in the next CLs for UMA / UKM aggregation.

2) Adds a GlobalMemoryDump return object to the RequestGlobalMemoryDump service
API.
This object will contain the data that UMA/UKM need to upload.

As a bonus this CL removes the
memory_instrumentation.mojom.MemoryDumpCallbackResult mojo
type. That callback was never meant to be exposed on a service boundary. That
callback is only
used between base::MemoryDumpManager and the client library, which are by design
both hosted
in the same process.

BUG=703184
==========

to

==========
memory-infra: add ProcessType and expose data in RequestGlobalDump()

This CL addresses two distinct problems:
1) Adds a ProcessType in the per-process struct returned by ProcessMemoryDump.
This will be required in the next CLs for UMA / UKM aggregation.

2) Adds a GlobalMemoryDump return object to the RequestGlobalMemoryDump service
API.
This object will contain the data that UMA/UKM need to upload.

As a bonus this CL removes the
memory_instrumentation.mojom.MemoryDumpCallbackResult mojo
type. That callback was never meant to be exposed on a service boundary. That
callback is only
used between base::MemoryDumpManager and the client library, which are by design
both hosted
in the same process.

BUG=703184
==========

[Primiano Tucci (use gerrit), 2017-05-10 15:25:35]

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

[commit-bot: I haz the power, 2017-05-10 15:25:43]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-10 16:18:12]

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

[commit-bot: I haz the power, 2017-05-10 16:19:19]

Dry run: CQ is trying da patch.

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

[fmeawad, 2017-05-10 17:09:38]

https://codereview.chromium.org/2871223002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom
(right):

https://codereview.chromium.org/2871223002/diff/40001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:62:
ProcessType process_type;
Can we send it as we register instead of sending it with every message?

[Primiano Tucci (use gerrit), 2017-05-10 17:17:19]

https://codereview.chromium.org/2871223002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom
(right):

https://codereview.chromium.org/2871223002/diff/40001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:62:
ProcessType process_type;
On 2017/05/10 17:09:38, fmeawad wrote:
> Can we send it as we register instead of sending it with every message?

the thing is that we want to expose it publicly to our clients, who obtain the
GlobalMemoryDUmp, so that UMA can make grouping based on this.

We could in theory send it only when registering, then keeping this cached on a
map in the coordinator_impl, than copy this back when we generate the
GlobalMemoryDump.Althgouh I am not sure all this is worth the effort of sending
an extra int :)

[hjd, 2017-05-10 17:31:02]

lgtm

[hjd, 2017-05-10 17:31:06]

lgtm

[commit-bot: I haz the power, 2017-05-10 17:36:56]

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

[commit-bot: I haz the power, 2017-05-10 17:36:57]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2017-05-10 17:44:40]

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

[Primiano Tucci (use gerrit), 2017-05-10 17:44:41]

+jam , in reference to the recent discussion on
https://groups.google.com/a/chromium.org/d/msg/services-dev/v85DHRCtrl4/XaSJ9...

[Primiano Tucci (use gerrit), 2017-05-10 17:50:37]

Description was changed from

==========
memory-infra: add ProcessType and expose data in RequestGlobalDump()

This CL addresses two distinct problems:
1) Adds a ProcessType in the per-process struct returned by ProcessMemoryDump.
This will be required in the next CLs for UMA / UKM aggregation.

2) Adds a GlobalMemoryDump return object to the RequestGlobalMemoryDump service
API.
This object will contain the data that UMA/UKM need to upload.

As a bonus this CL removes the
memory_instrumentation.mojom.MemoryDumpCallbackResult mojo
type. That callback was never meant to be exposed on a service boundary. That
callback is only
used between base::MemoryDumpManager and the client library, which are by design
both hosted
in the same process.

BUG=703184
==========

to

==========
memory-infra: add ProcessType and expose data in RequestGlobalDump()

This CL addresses two distinct problems:
1) Adds a ProcessType in the per-process struct returned by
ProcessMemoryDump. This will be required in the next CLs for
UMA / UKM aggregation.

2) Adds a GlobalMemoryDump return object to the RequestGlobalMemoryDump
service API. This object will contain the data that UMA/UKM need to
upload.

As a bonus this CL removes the mojo type
memory_instrumentation.mojom.MemoryDumpCallbackResult.
That callback was never meant to be exposed across a service boundary.
It is only used between base::MemoryDumpManager and the client library,
which are by design both hosted in the same process.

BUG=703184
==========

[fmeawad, 2017-05-10 18:00:16]

lgtm

https://codereview.chromium.org/2871223002/diff/40001/services/resource_coord...
File
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom
(right):

https://codereview.chromium.org/2871223002/diff/40001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:62:
ProcessType process_type;
On 2017/05/10 17:17:19, Primiano Tucci wrote:
> On 2017/05/10 17:09:38, fmeawad wrote:
> > Can we send it as we register instead of sending it with every message?
> 
> the thing is that we want to expose it publicly to our clients, who obtain the
> GlobalMemoryDUmp, so that UMA can make grouping based on this.
> 
> We could in theory send it only when registering, then keeping this cached on
a
> map in the coordinator_impl, than copy this back when we generate the
> GlobalMemoryDump.Althgouh I am not sure all this is worth the effort of
sending
> an extra int :)

Makes sense.

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:39: //
should be doing this. Fix in the next CL.
nit: Remove Fix in the next CL

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:166:
queued_memory_dump_requests_.front().process_memory_dumps.push_back(
We will probably need to pass the process_manager or the identity as well so
that we can resolve the extra_process_dumps in the Finalize method, but I can
add that to my CL.

[jam, 2017-05-10 22:15:07]

lgtm

[erikchen, 2017-05-10 22:21:56]

On 2017/05/10 22:15:07, jam wrote:
> lgtm

I'm going to hit the CQ button [already talked with primiano about this]. Note
that for fmeawad's 2 comments, 1 doesn't require a response, and the other is a
nit on a comment. I'll let primiano address in a follow up.

[erikchen, 2017-05-10 22:22:04]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-05-10 22:23:02]

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-10 22:39:55]

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

[commit-bot: I haz the power, 2017-05-10 22:39:56]

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

[erikchen, 2017-05-10 22:47:24]

erikchen@chromium.org changed reviewers:
+ dcheng@chromium.org

[erikchen, 2017-05-10 22:47:25]

dcheng: Please review as SECURITY_OWNER

[dcheng, 2017-05-11 05:55:26]

Do you mind having a quick VC tomorrow to help me understand the overall design
of the memory-infra services?

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:88:
mojom::GlobalMemoryDumpPtr());
Or nullptr (here and elsewhere)

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl_unittest.cc:39:
callback.Run(args.dump_guid, true, mojom::GlobalMemoryDumpPtr());
Or just nullptr

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
File
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom
(right):

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:81:
// remote process to contribute of the memory-infra dumps. There should be at
"to contribute of the": this is a bit hard to read

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:88:
(uint64 dump_guid, bool success, ProcessMemoryDump? process_memory_dump);
this isn't a guid? at least not in the traditional guid sense

[Primiano Tucci (use gerrit), 2017-05-11 12:48:44]

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

[commit-bot: I haz the power, 2017-05-11 12:49:03]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-11 12:50:03]

> Do you mind having a quick VC tomorrow to help me understand the overall
design of the memory-infra services?

I fully understand. Sadly in the current state we are in the worst state ever,
with code moving from base to service and existing tracing use cases that we
have to maintain working thorughout all the cls.
Right now there is an incredible amount of plumbing that makes me nervous and
I'm planning to get rid soon.

Happy to vc, in the meantime I dumped my brain on a design doc:
http://bit.ly/memory-infra-service

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:39: //
should be doing this. Fix in the next CL.
On 2017/05/10 18:00:16, fmeawad wrote:
> nit: Remove Fix in the next CL

Done.

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:88:
mojom::GlobalMemoryDumpPtr());
On 2017/05/11 05:55:26, dcheng wrote:
> Or nullptr (here and elsewhere)

Done.

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:166:
queued_memory_dump_requests_.front().process_memory_dumps.push_back(
On 2017/05/10 18:00:16, fmeawad wrote:
> We will probably need to pass the process_manager or the identity as well so
> that we can resolve the extra_process_dumps in the Finalize method, but I can
> add that to my CL.

Yeah let's see how the discussion on pid settles

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
File
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom
(right):

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:81:
// remote process to contribute of the memory-infra dumps. There should be at
On 2017/05/11 05:55:26, dcheng wrote:
> "to contribute of the": this is a bit hard to read

typo. done

https://codereview.chromium.org/2871223002/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:88:
(uint64 dump_guid, bool success, ProcessMemoryDump? process_memory_dump);
On 2017/05/11 05:55:26, dcheng wrote:
> this isn't a guid? at least not in the traditional guid sense

not in the base::guid. it's guid as in "globally unique identifier".
Essentially it's a monotonic counter that the service generates for each dump
request, and is used by telemetry/devtools etc to get a reference (in the
general sense, not c++ sense) to the object that has been dumped into the trace.

[commit-bot: I haz the power, 2017-05-11 14:33:50]

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

[commit-bot: I haz the power, 2017-05-11 14:33:51]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-12 08:07:42]

https://codereview.chromium.org/2871223002/diff/80001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
(right):

https://codereview.chromium.org/2871223002/diff/80001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:74:
process_memory_dump->os_dump = result->os_dump;
Is there a reason to switch from using StructTraits to marshalling this
manually?

[Primiano Tucci (use gerrit), 2017-05-12 11:42:46]

https://codereview.chromium.org/2871223002/diff/80001/services/resource_coord...
File
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
(right):

https://codereview.chromium.org/2871223002/diff/80001/services/resource_coord...
services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc:74:
process_memory_dump->os_dump = result->os_dump;
On 2017/05/12 08:07:41, dcheng wrote:
> Is there a reason to switch from using StructTraits to marshalling this
> manually?

Yes. the MemoryDumpManager::CreateProcessDump() callback itself is not meant to
go cross process boundaries. Was something I missed in my original review.
CreateProcessDump is an internal interface between MDM in base and this client
library, which are always hosted in the same process.

The concrete problem here is that the callback between PMD and this client
library doesn't contain all the data that needs to be put in the Mojo callback
between this client library and the Coordinator service.
So the model here is: This client library should get the callback from
base::MDM, enrich it with extra stuff (process_type) that base cannot know
about, and then return the final Mojo product to the service.

If I use struct traits, the only way to inject the process_type here is to then
force base::MDM to know about that. But base::MDM has no functional need of
knowing that, and doing such will just create extra layering.

[dcheng, 2017-05-12 17:10:20]

discussed offline. lgtm

[Primiano Tucci (use gerrit), 2017-05-12 17:17:27]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-05-12 17:17:27]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
fmeawad@chromium.org, hjd@chromium.org
Link to the patchset: https://codereview.chromium.org/2871223002/#ps80001
(title: "review comments")

[commit-bot: I haz the power, 2017-05-12 17:18:34]

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-12 19:46:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494609447236590,
"parent_rev": "7a2d075a08b521fd2d9102cf25d8aa92f811fff6", "commit_rev":
"5a7bd66da934d6decc3d60a8c0640a990fc0d0a8"}

[commit-bot: I haz the power, 2017-05-12 19:46:21]

Description was changed from

==========
memory-infra: add ProcessType and expose data in RequestGlobalDump()

This CL addresses two distinct problems:
1) Adds a ProcessType in the per-process struct returned by
ProcessMemoryDump. This will be required in the next CLs for
UMA / UKM aggregation.

2) Adds a GlobalMemoryDump return object to the RequestGlobalMemoryDump
service API. This object will contain the data that UMA/UKM need to
upload.

As a bonus this CL removes the mojo type
memory_instrumentation.mojom.MemoryDumpCallbackResult.
That callback was never meant to be exposed across a service boundary.
It is only used between base::MemoryDumpManager and the client library,
which are by design both hosted in the same process.

BUG=703184
==========

to

==========
memory-infra: add ProcessType and expose data in RequestGlobalDump()

This CL addresses two distinct problems:
1) Adds a ProcessType in the per-process struct returned by
ProcessMemoryDump. This will be required in the next CLs for
UMA / UKM aggregation.

2) Adds a GlobalMemoryDump return object to the RequestGlobalMemoryDump
service API. This object will contain the data that UMA/UKM need to
upload.

As a bonus this CL removes the mojo type
memory_instrumentation.mojom.MemoryDumpCallbackResult.
That callback was never meant to be exposed across a service boundary.
It is only used between base::MemoryDumpManager and the client library,
which are by design both hosted in the same process.

BUG=703184

Review-Url: https://codereview.chromium.org/2871223002
Cr-Commit-Position: refs/heads/master@{#471415}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a7bd66da934d6decc3d60a8c064...
==========

[commit-bot: I haz the power, 2017-05-12 19:46:22]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5a7bd66da934d6decc3d60a8c064...