memory-infra: Assemble dump results in service

memory-infra: Assemble dump results in service

Do two new things:
1) Compute private memory footprint in a platform specific way.
2) Match up OSMemDumps with the correct ChromeMemDump.

Both of these require a bit more work, we need to get the correct
PIDs to do 2, 1 needs to be implemented on Windows & ChromeOS,
improved on OSX and checked on Android.

[hjd, 2017-05-09 18:43:56]

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

[hjd, 2017-05-09 18:43:56]

first pass on assembling dump results in service ptal, thanks! :)

[hjd, 2017-05-09 18:49:38]

https://codereview.chromium.org/2867213003/diff/1/services/resource_coordinat...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2867213003/diff/1/services/resource_coordinat...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:35: //
TODO(hjd): On On 10.12+, use phys_footprint_bytes.
Erik: Do you know off hand if the correct thing to use is
base::mac::IsAtLeastOS10_12()?

[erikchen, 2017-05-09 19:22:38]

lgtm

https://codereview.chromium.org/2867213003/diff/1/services/resource_coordinat...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):

https://codereview.chromium.org/2867213003/diff/1/services/resource_coordinat...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:35: //
TODO(hjd): On On 10.12+, use phys_footprint_bytes.
On 2017/05/09 18:49:38, hjd wrote:
> Erik: Do you know off hand if the correct thing to use is
> base::mac::IsAtLeastOS10_12()?

yes it is.

[Primiano Tucci (use gerrit), 2017-05-10 15:26:38]

As discussed offline, if https://codereview.chromium.org/2871223002/ goes
through rebase on top of that.

[hjd, 2017-05-10 17:27:30]

updated ptal

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

LGTM with some comments but this will have to wait on the PID stuff :)

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

https://codereview.chromium.org/2867213003/diff/80001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:221:
mojom::ProcessMemoryDumpPtr& pmd = finalized_pmds[0];
maybe for the moment, for testing, you can s/0/finalized_pmds.size()/

https://codereview.chromium.org/2867213003/diff/80001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:241:
global_dump->process_dumps.push_back(std::move(pmd));
can we also sanity dcheck that each finalized PMD has a chrome_dump as well. to
rule out the case of a browser process providing os_dump for processes that
don't exist.
actually, thinking more, I think that can genuinely happen. There might be a
short window of time when a renderer dies where we don't receive the chrome
dumps from the client library (Becvause the renderer is dead) but the browsder
process keeps emitting os dump (because the process_metrics_MDP hasn't been
notified yet).
So I think we need to factor this case in here and test it.

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

https://codereview.chromium.org/2867213003/diff/80001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:13: #include
"base/process/process_handle.h"
why do we need this here? can it be moved to the .cc file?

https://codereview.chromium.org/2867213003/diff/80001/services/resource_coord...
services/resource_coordinator/memory/coordinator/coordinator_impl.h:21: using
MemoryDumpCallbackResult = base::trace_event::MemoryDumpCallbackResult;
no "using" in headers 

https://google.github.io/styleguide/cppguide.html#Namespaces

but okay if you move this to the .cc file

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

https://codereview.chromium.org/2867213003/diff/80001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:66:
uint32 cmm_private_footprint = 0;
cmm is going to be an obscure acronym for everybody % me, you and erikchen. Just
call it private_footprint and add a goog.gl link to the CMM doc.

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

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

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

+dcheng for a 1line uint32 in
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom

[dcheng, 2017-05-12 07:51:01]

LGTM with comment addressed

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

https://codereview.chromium.org/2867213003/diff/80001/services/resource_coord...
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:66:
uint32 cmm_private_footprint = 0;
On 2017/05/10 17:42:43, Primiano Tucci wrote:
> cmm is going to be an obscure acronym for everybody % me, you and erikchen.
Just
> call it private_footprint and add a goog.gl link to the CMM doc.

+1

[erikchen, 2017-05-12 20:02:34]

On 2017/05/12 07:51:01, dcheng wrote:
> LGTM with comment addressed
> 
>
https://codereview.chromium.org/2867213003/diff/80001/services/resource_coord...
> File
>
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom
> (right):
> 
>
https://codereview.chromium.org/2867213003/diff/80001/services/resource_coord...
>
services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom:66:
> uint32 cmm_private_footprint = 0;
> On 2017/05/10 17:42:43, Primiano Tucci wrote:
> > cmm is going to be an obscure acronym for everybody % me, you and erikchen.
> Just
> > call it private_footprint and add a goog.gl link to the CMM doc.
> 
> +1

I filed a bug to track the nits in the CL, and am going to send it through the
CQ.
https://bugs.chromium.org/p/chromium/issues/detail?id=721882

[This CL is blocking other CLs that the memory UMA team is trying to get landed
by Sunday].

[erikchen, 2017-05-12 20:02:43]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-12 20:02:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2867213003/#ps80001
(title: "update some autp types")

[commit-bot: I haz the power, 2017-05-12 20:03:53]

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 20:05:52]

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

[commit-bot: I haz the power, 2017-05-12 20:05:53]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hjd, 2017-05-15 12:06:19]

On 2017/05/12 20:05:53, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
>   android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
>   android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
>   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
>   mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Landed by Erik in https://codereview.chromium.org/2876733005, thanks!

[hjd, 2017-05-15 12:06:26]

Description was changed from

==========
memory-infra: Assemble dump results in service

Do two new things:
1) Compute private memory footprint in a platform specific way.
2) Match up OSMemDumps with the correct ChromeMemDump.

Both of these require a bit more work, we need to get the correct
PIDs to do 2, 1 needs to be implemented on Windows & ChromeOS,
improved on OSX and checked on Android.
==========

to

==========
memory-infra: Assemble dump results in service

Do two new things:
1) Compute private memory footprint in a platform specific way.
2) Match up OSMemDumps with the correct ChromeMemDump.

Both of these require a bit more work, we need to get the correct
PIDs to do 2, 1 needs to be implemented on Windows & ChromeOS,
improved on OSX and checked on Android.
==========