memory-infra: Introduce mojo interfaces and service stubs

memory-infra: Introduce mojo interfaces and service stubs

This is in preparation for the memory-infra refactoring (see design docs: http://bit.ly/tracing-unbundling,
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4)

A prototype of the end product: https://codereview.chromium.org/2571823002

BUG=679830
Review-Url: https://codereview.chromium.org/2621143002
Cr-Commit-Position: refs/heads/master@{#444398}
Committed: https://chromium.googlesource.com/chromium/src/+/b1fa91ef0d3812d9c25dee8d7845d4341747b71c

[chiniforooshan1, 2017-01-10 20:22:40]

Description was changed from

==========
Memory infra refactoring: Part 1

This is a minimal CL to get approval for the root directory and the
mojo interface.

A prototype of the end product: https://codereview.chromium.org/2571823002
Design doc:
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4

BUG=679830
==========

to

==========
Memory infra refactoring: Part 1

This is a minimal CL to get approval for the root directory and the
mojo interface.

A prototype of the end product: https://codereview.chromium.org/2571823002
Design doc:
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4

BUG=679830
==========

[chiniforooshan1, 2017-01-10 20:22:40]

chiniforooshan@google.com changed reviewers:
+ primiano@chromium.org, rockot@chromium.org

[chiniforooshan1, 2017-01-10 20:25:08]

On 2017/01/10 20:22:40, chiniforooshan1 wrote:
> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chiniforooshan@google.com
changed reviewers:
> + https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=primiano@chromium.org,
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=rockot@chromium.org

Primiano, could you please do the first pass? Thanks!

[ssid, 2017-01-11 00:13:25]

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

[ssid, 2017-01-11 00:13:25]

some naming suggestions.

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
File services/memory_infra/public/cpp/memory_infra_traits.cc (right):

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
services/memory_infra/public/cpp/memory_infra_traits.cc:60: case
base::trace_event::MemoryDumpLevelOfDetail::FIRST:
Drive-by: BACKGROUND would make more sense than FIRST here. Since it's actually
a mode and FIRST is just used for iterating somewhere.

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
File services/memory_infra/public/interfaces/memory_infra.mojom (right):

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
services/memory_infra/public/interfaces/memory_infra.mojom:31:
RequestLocalMemoryDump(RequestArgs args) => (uint64 dump_guid, bool success);
I'd prefer Process over Local here since the name of the interface (Client) does
not directly suggest it's per process.
So, Local could mean a Thread also. But, leave it to Primiano for the decision
on naming.

[chiniforooshan1, 2017-01-11 16:07:43]

chiniforooshan@google.com changed reviewers:
+ chiniforooshan@google.com

[chiniforooshan1, 2017-01-11 16:07:44]

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
File services/memory_infra/public/cpp/memory_infra_traits.cc (right):

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
services/memory_infra/public/cpp/memory_infra_traits.cc:60: case
base::trace_event::MemoryDumpLevelOfDetail::FIRST:
On 2017/01/11 00:13:25, ssid wrote:
> Drive-by: BACKGROUND would make more sense than FIRST here. Since it's
actually
> a mode and FIRST is just used for iterating somewhere.

Sure. But let me rename this in a separate CL: enums in the mojom file mirror
memory_dump_request_args.h and I prefer to not touch //base in this CL.

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
File services/memory_infra/public/interfaces/memory_infra.mojom (right):

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
services/memory_infra/public/interfaces/memory_infra.mojom:31:
RequestLocalMemoryDump(RequestArgs args) => (uint64 dump_guid, bool success);
On 2017/01/11 00:13:25, ssid wrote:
> I'd prefer Process over Local here since the name of the interface (Client)
does
> not directly suggest it's per process.
> So, Local could mean a Thread also. But, leave it to Primiano for the decision
> on naming.

Done.

[chiniforooshan1, 2017-01-11 16:19:15]

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
File services/memory_infra/public/cpp/memory_infra_traits.cc (right):

https://codereview.chromium.org/2621143002/diff/1/services/memory_infra/publi...
services/memory_infra/public/cpp/memory_infra_traits.cc:60: case
base::trace_event::MemoryDumpLevelOfDetail::FIRST:
On 2017/01/11 16:07:44, chiniforooshan1 wrote:
> On 2017/01/11 00:13:25, ssid wrote:
> > Drive-by: BACKGROUND would make more sense than FIRST here. Since it's
> actually
> > a mode and FIRST is just used for iterating somewhere.
> 
> Sure. But let me rename this in a separate CL: enums in the mojom file mirror
> memory_dump_request_args.h and I prefer to not touch //base in this CL.

I think I misunderstood your comment the first time. Done.

[Primiano Tucci (use gerrit), 2017-01-16 15:11:04]

really thanks a lot for taking care of this, would have taken me forever to
figure out all this.
Architecturally speaking LGTM with just one small name bike-shedding.

I am really new to mojo, so really have no way to give good inputs on the
mojo-related stuff. I did  just monkey-check that the enum names match
 I hope that Ken can cover that.
Also (but this is completely orthogonal with the CL) am a bit surprised that
carrying around enums and structs of PODs requires all this sort of boilerplate.
But if this is the mojo way of doing things I'm fine, we are not certainly going
to change it in this cl.

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
File services/memory_infra/public/cpp/memory_infra_traits.cc (right):

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.cc:17: switch (type) {
maybe add a comment to memory_dump_request_args.h saying "this must be kept
consistent with services/.../memory_infra_traits.cc"

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.cc:27: return
memory_infra::mojom::DumpType::PEAK_MEMORY_USAGE;
I don't know if there is a pattern here but silently retuning an arbitrary value
makes me a bit nervous (NOTREACHED is a noop in release builds).
Can this be a CHECK(false) ? same below

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
File services/memory_infra/public/cpp/memory_infra_traits.h (right):

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.h:13: template<>
(not related to your code, more about the mojo architecture)
it feels a bit odd that one has to manually wire up all this stuff. it seems to
me way more boilerplate from the previous IPC macros and hance more opportunity
to get this wrong.
But I also see that this is the way other pieces of the codebase seem to do
this, so I assume this is "the mojo way" of passing enums and structs. ¯\_(ツ)_/¯

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
File services/memory_infra/public/interfaces/memory_infra.mojom (right):

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/interfaces/memory_infra.mojom:26: // client per
process.
maybe expand a bit more saying. When successful, the dump is appended in the
process-local trace buffer of the target process and only a ACK is returned.

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/interfaces/memory_infra.mojom:27: interface Client
{
naming bike-shedding, yay: client may be a bit generic, because "memory infra
client" might also mean "something external which is totally unaware of the
internal operations of memory-infra and just wants to see an API to request a
dump". for instance, this is what devtools does today, in that sense they are a
"memory-infra client" although that "client" is semantically different from this
notion of client :)
could this be ProcessLocalDumpManager or something that emphasizes that this is
really bound to the notion of the current process?

[Primiano Tucci (use gerrit), 2017-01-16 15:12:29]

ah maybe I'd change the title of the CL saying something like

memory-infra: introduce mojo interfaces and service stubs

This is in preparation for the memory-infra refactoring, see design doc (...,
and the other links you have)

[chiniforooshan1, 2017-01-16 16:55:37]

Description was changed from

==========
Memory infra refactoring: Part 1

This is a minimal CL to get approval for the root directory and the
mojo interface.

A prototype of the end product: https://codereview.chromium.org/2571823002
Design doc:
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4

BUG=679830
==========

to

==========
memory-infra: Introduce mojo interfaces and service stubs

This is in preparation for the memory-infra refactoring (see design docs:
http://bit.ly/tracing-unbundling,
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4)

A prototype of the end product: https://codereview.chromium.org/2571823002

BUG=679830
==========

[chiniforooshan1, 2017-01-16 18:14:48]

Ken, could you please take a look? Thanks!

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
File services/memory_infra/public/cpp/memory_infra_traits.cc (right):

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.cc:17: switch (type) {
On 2017/01/16 15:11:04, Primiano Tucci wrote:
> maybe add a comment to memory_dump_request_args.h saying "this must be kept
> consistent with services/.../memory_infra_traits.cc"

Done.

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.cc:27: return
memory_infra::mojom::DumpType::PEAK_MEMORY_USAGE;
On 2017/01/16 15:11:04, Primiano Tucci wrote:
> I don't know if there is a pattern here but silently retuning an arbitrary
value
> makes me a bit nervous (NOTREACHED is a noop in release builds).
> Can this be a CHECK(false) ? same below

Done.

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
File services/memory_infra/public/cpp/memory_infra_traits.h (right):

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.h:13: template<>
On 2017/01/16 15:11:04, Primiano Tucci wrote:
> (not related to your code, more about the mojo architecture)
> it feels a bit odd that one has to manually wire up all this stuff. it seems
to
> me way more boilerplate from the previous IPC macros and hance more
opportunity
> to get this wrong.
> But I also see that this is the way other pieces of the codebase seem to do
> this, so I assume this is "the mojo way" of passing enums and structs.
¯\_(ツ)_/¯

Ken, am I doing this correctly?

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
File services/memory_infra/public/interfaces/memory_infra.mojom (right):

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/interfaces/memory_infra.mojom:26: // client per
process.
On 2017/01/16 15:11:04, Primiano Tucci wrote:
> maybe expand a bit more saying. When successful, the dump is appended in the
> process-local trace buffer of the target process and only a ACK is returned.

Done.

https://codereview.chromium.org/2621143002/diff/60001/services/memory_infra/p...
services/memory_infra/public/interfaces/memory_infra.mojom:27: interface Client
{
On 2017/01/16 15:11:04, Primiano Tucci wrote:
> naming bike-shedding, yay: client may be a bit generic, because "memory infra
> client" might also mean "something external which is totally unaware of the
> internal operations of memory-infra and just wants to see an API to request a
> dump". for instance, this is what devtools does today, in that sense they are
a
> "memory-infra client" although that "client" is semantically different from
this
> notion of client :)
> could this be ProcessLocalDumpManager or something that emphasizes that this
is
> really bound to the notion of the current process?

Done.

[Ken Rockot(use gerrit already), 2017-01-17 19:34:53]

LGTM with some small changes.

Also as a high-level comment (but as a non-owner), "memory_infra" seems like a
surprising/confusing/ambiguous name for a service. I also don't see any mention
of a memory_infra service in the linked design document. Is this distinct from a
"tracing" service? How does it fit into the bigger picture? Does it need to be a
separate service?

https://codereview.chromium.org/2621143002/diff/80001/services/memory_infra/p...
File services/memory_infra/public/cpp/memory_infra_traits.cc (right):

https://codereview.chromium.org/2621143002/diff/80001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.cc:120: if
(!EnumTraits<memory_infra::mojom::DumpType,
Use the dataview ReadFoo methods for reading enums, as they do the same thing
and are easier to read. e.g.  if (!input.ReadDumpType(&out->dump_type))

https://codereview.chromium.org/2621143002/diff/80001/services/memory_infra/p...
File services/memory_infra/public/cpp/memory_infra_traits.h (right):

https://codereview.chromium.org/2621143002/diff/80001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.h:38: static
memory_infra::mojom::DumpType dump_type(
This implementation (and level_of_detail below) is redundant. You can just
return the non-mojom enum type here without any explicit cast or conversion. The
EnumTraits will be used automatically.

[Primiano Tucci (use gerrit), 2017-01-17 19:41:29]

On 2017/01/17 19:34:53, Ken Rockot wrote:
> LGTM with some small changes.
> 
> Also as a high-level comment (but as a non-owner), "memory_infra" seems like a
> surprising/confusing/ambiguous name for a service. I also don't see any
mention
> of a memory_infra service in the linked design document. Is this distinct from
a
> "tracing" service? How does it fit into the bigger picture? Does it need to be
a
> separate service?

I can reply on this. Yeah so we are in the process of splitting memory-infra
from tracing. The driving motivation is UMA, where we want the memory
infrastructure part but without all the tracing machinery.
All the discussions we had in the past about tracing (the fact that it needs to
be bound to the hosting process) still hold. Is just that now we have two
services: tracing and the memory-tracking infrastructure, as they can be used
independently (or together).

If you don't like memory_infra maybe memory_instrumentation is a better name?

Longer description in this doc :
https://docs.google.com/document/d/1BQSdQ3GvH_n1xJPv_WYLDCa0h-EvL4gn4ppLvglS3...
eshan: update your doc to mention it and link to this?

[chiniforooshan1, 2017-01-17 21:26:23]

> eshan: update your doc to mention it and link to this?

Done. Mentioned this in the abstract and mojo-based redesign sections.

https://codereview.chromium.org/2621143002/diff/80001/services/memory_infra/p...
File services/memory_infra/public/cpp/memory_infra_traits.cc (right):

https://codereview.chromium.org/2621143002/diff/80001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.cc:120: if
(!EnumTraits<memory_infra::mojom::DumpType,
On 2017/01/17 19:34:53, Ken Rockot wrote:
> Use the dataview ReadFoo methods for reading enums, as they do the same thing
> and are easier to read. e.g.  if (!input.ReadDumpType(&out->dump_type))

Done. Thank you!

https://codereview.chromium.org/2621143002/diff/80001/services/memory_infra/p...
File services/memory_infra/public/cpp/memory_infra_traits.h (right):

https://codereview.chromium.org/2621143002/diff/80001/services/memory_infra/p...
services/memory_infra/public/cpp/memory_infra_traits.h:38: static
memory_infra::mojom::DumpType dump_type(
On 2017/01/17 19:34:53, Ken Rockot wrote:
> This implementation (and level_of_detail below) is redundant. You can just
> return the non-mojom enum type here without any explicit cast or conversion.
The
> EnumTraits will be used automatically.

Done.

[chiniforooshan1, 2017-01-17 21:27:22]

The CQ bit was checked by chiniforooshan@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:27:38]

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-01-17 23:01:33]

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

[commit-bot: I haz the power, 2017-01-17 23:01:34]

Dry run: This issue passed the CQ dry run.

[chiniforooshan1, 2017-01-17 23:03:46]

The CQ bit was checked by chiniforooshan@google.com

[chiniforooshan1, 2017-01-17 23:03:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2621143002/#ps100001
(title: "review")

[chiniforooshan1, 2017-01-17 23:04:07]

The CQ bit was unchecked by chiniforooshan@google.com

[chiniforooshan1, 2017-01-17 23:20:50]

Renamed memory_infra to memory_instrumentation everywhere.

[chiniforooshan1, 2017-01-18 15:36:52]

The CQ bit was checked by chiniforooshan@google.com

[chiniforooshan1, 2017-01-18 15:36:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2621143002/#ps120001
(title: "memory_infra -> memory_instrumentation")

[commit-bot: I haz the power, 2017-01-18 15:37:09]

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-01-18 16:57:02]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484753812402900,
"parent_rev": "e0629c442598055c7cc4b035eb27e5d593beb46d", "commit_rev":
"b1fa91ef0d3812d9c25dee8d7845d4341747b71c"}

[commit-bot: I haz the power, 2017-01-18 16:57:36]

Description was changed from

==========
memory-infra: Introduce mojo interfaces and service stubs

This is in preparation for the memory-infra refactoring (see design docs:
http://bit.ly/tracing-unbundling,
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4)

A prototype of the end product: https://codereview.chromium.org/2571823002

BUG=679830
==========

to

==========
memory-infra: Introduce mojo interfaces and service stubs

This is in preparation for the memory-infra refactoring (see design docs:
http://bit.ly/tracing-unbundling,
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4)

A prototype of the end product: https://codereview.chromium.org/2571823002

BUG=679830

Review-Url: https://codereview.chromium.org/2621143002
Cr-Commit-Position: refs/heads/master@{#444398}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1fa91ef0d3812d9c25dee8d7845...
==========

[commit-bot: I haz the power, 2017-01-18 16:57:37]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/b1fa91ef0d3812d9c25dee8d7845...

[Wez, 2017-02-08 01:00:06]

Description was changed from

==========
memory-infra: Introduce mojo interfaces and service stubs

This is in preparation for the memory-infra refactoring (see design docs:
http://bit.ly/tracing-unbundling,
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4)

A prototype of the end product: https://codereview.chromium.org/2571823002

BUG=679830

Review-Url: https://codereview.chromium.org/2621143002
Cr-Commit-Position: refs/heads/master@{#444398}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1fa91ef0d3812d9c25dee8d7845...
==========

to

==========
memory-infra: Introduce mojo interfaces and service stubs

This is in preparation for the memory-infra refactoring (see design docs:
http://bit.ly/tracing-unbundling,
https://docs.google.com/document/d/1Mz64egjuZ4WsYw9AKKWdTvl0Il706EWdCy24V87R_F4)

A prototype of the end product: https://codereview.chromium.org/2571823002

BUG=679830

Review-Url: https://codereview.chromium.org/2621143002
Cr-Commit-Position: refs/heads/master@{#444398}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1fa91ef0d3812d9c25dee8d7845...
==========

[Wez, 2017-02-08 01:00:06]

wez@chromium.org changed reviewers:
+ mbarbella@chromium.org

[Wez, 2017-02-08 02:05:23]

Hallo mbarbella@chromium.org!
Due to a depot_tools patch which mistakenly removed the OWNERS check for
non-source files (see crbug.com/684270), the following files landed in this CL
and need a retrospective review from you:
	services/memory_instrumentation/public/interfaces/memory_instrumentation.mojom
Thanks,
Wez