[Memory UMA] Return a memory summary struct with the ack message

[Memory UMA] Return a memory summary struck with the ack message

This CL updates the returned mojo message to include the MemoryDumpCallbackResult. That struct will be used to create a memory summary that can be sent through UMA and used for other purposes.

A follow up CL will add the extra_processes_dump map.

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

[fmeawad, 2017-03-29 16:24:51]

Description was changed from

==========
[Memory UMA] Return a memory summary struck with the ack message

NOT READY FOR REVIEW YET
Also missing tests

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

to

==========
[Memory UMA] Return a memory summary struck with the ack message

NOT READY FOR REVIEW YET
Also missing tests

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

[Primiano Tucci (use gerrit), 2017-03-30 15:04:17]

Just looked into this with hector, makes lot of sense.
The only thing that seems missing is the "extra_processes_dump" which is there
in the MemoryDumpCallbackResult  but not in the corresponding mojo struct.
Unfortunatelly we'll really need that as that is the thing that allows us to
workaround the sandbox (but fine if you want to introduce that in a separate CL)

[fmeawad, 2017-03-30 21:36:00]

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

[commit-bot: I haz the power, 2017-03-30 21:37:16]

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-03-30 23:06:37]

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

[commit-bot: I haz the power, 2017-03-30 23:06:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fmeawad, 2017-04-06 04:55:06]

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

[fmeawad, 2017-04-06 04:55:06]

I think this might be ready for an official review, can you do a first pass?

[hjd, 2017-04-06 11:18:45]

Seems good to me, the mojo boilerplate seems sad but I guess that is just the
way it is...

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:766:
base::Optional<MemoryDumpCallbackResult> result_opt;
Should we have #include "base/optional.h"?

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

https://codereview.chromium.org/2777093009/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:31:
uint32 partition_alloc_total_kb = 0;
Maybe our structs should be uint64? uint32 only goes up to 4TB... ;P

https://codereview.chromium.org/2777093009/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:38:
ChromeMemDump chrome_dump;
With the map to be added in a follow up?

[fmeawad, 2017-04-06 14:40:25]

Description was changed from

==========
[Memory UMA] Return a memory summary struck with the ack message

NOT READY FOR REVIEW YET
Also missing tests

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

to

==========
[Memory UMA] Return a memory summary struck with the ack message

This CL updates the returned mojo message to include the
MemoryDumpCallbackResult. That struct will be used to create a memory summary
that can be sent through UMA and used for other purposes.

A follow up CL will add the extra_processes_dump map.

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

[fmeawad, 2017-04-06 15:02:00]

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

[commit-bot: I haz the power, 2017-04-06 15:02:26]

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-04-06 15:36:29]

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

[commit-bot: I haz the power, 2017-04-06 15:36:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Primiano Tucci (use gerrit), 2017-04-07 15:31:37]

Thanks! glad to see all this coming together.
LGTM with minor comment addressed.

I think the bots are red just because of a missing BASE_EXPORT (See comment
below inline.

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:766:
base::Optional<MemoryDumpCallbackResult> result_opt;
On 2017/04/06 11:18:45, hjd wrote:
> Should we have #include "base/optional.h"?

+1, IWYU (Include What You Use, i.e. don't depen don that being pulled in by
dependencies)

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:139: this,
&MemoryDumpManagerDelegateForTesting::CreateGlobalDump));
Insrtead of defining an extra metod just to wrap the callback,  you can just do
it in a lambda here. This would avoid creating a method with a confusing name
(you call it "CreateglobalDump" but in reality calls CreateProcessDump with a
wrapped callback).
I think you can just do here:

.WillByDefault(Invoke([] const MemoryDumpRequestArgs& args, const
GlobalMemoryDumpCallback& callback) {
ProcessMemoryDumpCallback process_callback  = Bind(
Bind(&ProcessDumpCallbackAdapter, callback);
 CreateProcessDump(args, process_callback);
})

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_request_args.h (right):

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_request_args.h:82: struct MemoryDumpCallbackResult
{
I think you now need BASE_EXPORT here.

The bots are red with

memory_instrumentation.mojom.obj : error LNK2001: unresolved external symbol
"public: __cdecl
base::trace_event::MemoryDumpCallbackResult::MemoryDumpCallbackResult(void)"
(??0MemoryDumpCallbackResult@trace_event@base@@QEAA@XZ)

Which suggests that the new const constructor now requires actual code to be
exported by base in component builds.

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_request_args.h:112: const
base::Optional<MemoryDumpCallbackResult>& result)>;
nit: no need for base:: this is already in "namespace base"

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

https://codereview.chromium.org/2777093009/diff/60001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:71: result);
you can omit the argument name ("result") to keep it on the same name. The type
makes it quite evident that it is a result :)

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

https://codereview.chromium.org/2777093009/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:31:
uint32 partition_alloc_total_kb = 0;
On 2017/04/06 11:18:45, hjd wrote:
> Maybe our structs should be uint64? uint32 only goes up to 4TB... ;P

I'd hope they do suffice, if we go over 4TB we have bigger problems :)

https://codereview.chromium.org/2777093009/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:38:
ChromeMemDump chrome_dump;
On 2017/04/06 11:18:45, hjd wrote:
> With the map to be added in a follow up?

given the cl description I guess so .)

[fmeawad, 2017-04-10 20:16:14]

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

[commit-bot: I haz the power, 2017-04-10 20:16:55]

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-04-10 20:19:36]

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

[commit-bot: I haz the power, 2017-04-10 20:19:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[fmeawad, 2017-04-10 20:20:19]

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:766:
base::Optional<MemoryDumpCallbackResult> result_opt;
On 2017/04/07 15:31:36, Primiano Tucci wrote:
> On 2017/04/06 11:18:45, hjd wrote:
> > Should we have #include "base/optional.h"?
> 
> +1, IWYU (Include What You Use, i.e. don't depen don that being pulled in by
> dependencies)

Done.

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:139: this,
&MemoryDumpManagerDelegateForTesting::CreateGlobalDump));
On 2017/04/07 15:31:36, Primiano Tucci wrote:
> Insrtead of defining an extra metod just to wrap the callback,  you can just
do
> it in a lambda here. This would avoid creating a method with a confusing name
> (you call it "CreateglobalDump" but in reality calls CreateProcessDump with a
> wrapped callback).
> I think you can just do here:
> 
> .WillByDefault(Invoke([] const MemoryDumpRequestArgs& args, const
> GlobalMemoryDumpCallback& callback) {
> ProcessMemoryDumpCallback process_callback  = Bind(
> Bind(&ProcessDumpCallbackAdapter, callback);
>  CreateProcessDump(args, process_callback);
> })

Done.

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_request_args.h (right):

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_request_args.h:82: struct MemoryDumpCallbackResult
{
On 2017/04/07 15:31:36, Primiano Tucci wrote:
> I think you now need BASE_EXPORT here.
> 
> The bots are red with
> 
> memory_instrumentation.mojom.obj : error LNK2001: unresolved external symbol
> "public: __cdecl
> base::trace_event::MemoryDumpCallbackResult::MemoryDumpCallbackResult(void)"
> (??0MemoryDumpCallbackResult@trace_event@base@@QEAA@XZ)
> 
> Which suggests that the new const constructor now requires actual code to be
> exported by base in component builds.

Done.

https://codereview.chromium.org/2777093009/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_request_args.h:112: const
base::Optional<MemoryDumpCallbackResult>& result)>;
On 2017/04/07 15:31:36, Primiano Tucci wrote:
> nit: no need for base:: this is already in "namespace base"

Done.

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

https://codereview.chromium.org/2777093009/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:31:
uint32 partition_alloc_total_kb = 0;
On 2017/04/06 11:18:45, hjd wrote:
> Maybe our structs should be uint64? uint32 only goes up to 4TB... ;P

Happy to make the change on both side if we decide to do that.

https://codereview.chromium.org/2777093009/diff/60001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:38:
ChromeMemDump chrome_dump;
On 2017/04/06 11:18:45, hjd wrote:
> With the map to be added in a follow up?

Yes.

[fmeawad, 2017-04-10 20:51:10]

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

[commit-bot: I haz the power, 2017-04-10 20:51:52]

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-04-10 23:19:12]

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

[commit-bot: I haz the power, 2017-04-10 23:19:13]

Dry run: This issue passed the CQ dry run.

[hjd, 2017-04-11 09:39:42]

lgtm

[fmeawad, 2017-04-11 15:43:51]

fmeawad@chromium.org changed reviewers:
+ oysteine@chromium.org, rockot@chromium.org

[fmeawad, 2017-04-11 15:43:52]

Adding more owners

[Ken Rockot(use gerrit already), 2017-04-11 17:39:24]

lgtm

[fmeawad, 2017-04-11 20:35:01]

fmeawad@chromium.org changed reviewers:
+ mkwst@chromium.org

[fmeawad, 2017-04-11 20:35:01]

+ mkwst for IPC review

[Mike West, 2017-04-12 06:33:43]

mojom lgtm

[fmeawad, 2017-04-12 14:35:23]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2017-04-12 14:35:23]

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

[commit-bot: I haz the power, 2017-04-12 14:35:48]

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-04-12 14:38:21]

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

[commit-bot: I haz the power, 2017-04-12 14:38:22]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[fmeawad, 2017-04-12 15:57:28]

The CQ bit was checked by fmeawad@chromium.org

[fmeawad, 2017-04-12 15:57:29]

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

[commit-bot: I haz the power, 2017-04-12 15:57:51]

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-04-12 18:05:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492012648703870,
"parent_rev": "7c5d4db1a21cb156b3881bf67233e3004bf17852", "commit_rev":
"bd1f17f86efd1346e16628622ddde56d560d4a4b"}

[commit-bot: I haz the power, 2017-04-12 18:06:22]

Description was changed from

==========
[Memory UMA] Return a memory summary struck with the ack message

This CL updates the returned mojo message to include the
MemoryDumpCallbackResult. That struct will be used to create a memory summary
that can be sent through UMA and used for other purposes.

A follow up CL will add the extra_processes_dump map.

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

to

==========
[Memory UMA] Return a memory summary struck with the ack message

This CL updates the returned mojo message to include the
MemoryDumpCallbackResult. That struct will be used to create a memory summary
that can be sent through UMA and used for other purposes.

A follow up CL will add the extra_processes_dump map.

BUG=703184

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

[commit-bot: I haz the power, 2017-04-12 18:06:24]

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