memory-infra: Add a struct to carry memory use information

memory-infra: Add a struct to carry memory use information

This (currently unused) struct will carry information back to the
memory coordinator service where it will eventually be collated
and make its way to UMA.

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

[hjd, 2017-03-20 15:53:55]

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

[hjd, 2017-03-20 15:53:56]

ptal, thanks!

[fmeawad, 2017-03-20 18:24:32]

On 2017/03/20 15:53:56, hjd wrote:
> ptal, thanks!

Lets not land this yet, I will patch it locally and you do the same, and lets
land it once we have a use case for it?

[Primiano Tucci (use gerrit), 2017-03-21 15:26:06]

LGTM.
Maybe would be nicer from a coding viewpoint if we did wire this up to the
callback already (even if empty).
fine if this happens in a subsequent CL though

https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_request_args.h (right):

https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_request_args.h:77: struct MemoryDumpCallbackResult
{
maybe add a comment explaiing what this will be for.
Also add a TODO(hjd) to make it explicit that this is not used yet and will be
used soon.

https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_request_args.h:80: uint32_t private_dirty;
+kb

https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_request_args.h:96: std::map<ProcessId, OSMemDump>
os_dump_for_child_processes;
maybe call this |extra_processes_dump|

base is not really supposed to know about browser/"child" processes (although we
stretched this rule in comments a bit, so it's fine there).
I'd probably make the comment a bit more explicit about the use case thoguh. I'd
say something like:
--
In some cases, os stats can only be dumped from a priviledged process to get
around to sandboxing/selinux restrictions (see crbug.com/461788) 
--

[hjd, 2017-03-22 11:19:40]

On 2017/03/21 15:26:06, Primiano Tucci wrote:
> LGTM.
> Maybe would be nicer from a coding viewpoint if we did wire this up to the
> callback already (even if empty).
> fine if this happens in a subsequent CL though

fmeawad's coming CL plumbs the callback.

>
https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
> File base/trace_event/memory_dump_request_args.h (right):
> 
>
https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
> base/trace_event/memory_dump_request_args.h:77: struct
MemoryDumpCallbackResult
> {
> maybe add a comment explaiing what this will be for.
> Also add a TODO(hjd) to make it explicit that this is not used yet and will be
> used soon.

Done.

>
https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
> base/trace_event/memory_dump_request_args.h:80: uint32_t private_dirty;
> +kb

Done.

>
https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
> base/trace_event/memory_dump_request_args.h:96: std::map<ProcessId, OSMemDump>
> os_dump_for_child_processes;
> maybe call this |extra_processes_dump|

Done.

> base is not really supposed to know about browser/"child" processes (although
we
> stretched this rule in comments a bit, so it's fine there).
> I'd probably make the comment a bit more explicit about the use case thoguh.
I'd
> say something like:
> --
> In some cases, os stats can only be dumped from a priviledged process to get
> around to sandboxing/selinux restrictions (see crbug.com/461788) 
> --

Done.

Thanks!

[hjd, 2017-03-24 11:00:59]

Description was changed from

==========
Add a struct to carry memory use information

This (currently unused) struct will carry information back to the
memory coordinator service where it will eventually be collated
and make its way to UMA.

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

to

==========
memory-infra: Add a struct to carry memory use information

This (currently unused) struct will carry information back to the
memory coordinator service where it will eventually be collated
and make its way to UMA.

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

[hjd, 2017-03-24 11:02:14]

On 2017/03/22 11:19:40, hjd wrote:
> On 2017/03/21 15:26:06, Primiano Tucci wrote:
> > LGTM.
> > Maybe would be nicer from a coding viewpoint if we did wire this up to the
> > callback already (even if empty).
> > fine if this happens in a subsequent CL though
> 
> fmeawad's coming CL plumbs the callback.
> 
> >
>
https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
> > File base/trace_event/memory_dump_request_args.h (right):
> > 
> >
>
https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
> > base/trace_event/memory_dump_request_args.h:77: struct
> MemoryDumpCallbackResult
> > {
> > maybe add a comment explaiing what this will be for.
> > Also add a TODO(hjd) to make it explicit that this is not used yet and will
be
> > used soon.
> 
> Done.
> 
> >
>
https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
> > base/trace_event/memory_dump_request_args.h:80: uint32_t private_dirty;
> > +kb
> 
> Done.
> 
> >
>
https://codereview.chromium.org/2755343003/diff/1/base/trace_event/memory_dum...
> > base/trace_event/memory_dump_request_args.h:96: std::map<ProcessId,
OSMemDump>
> > os_dump_for_child_processes;
> > maybe call this |extra_processes_dump|
> 
> Done.
> 
> > base is not really supposed to know about browser/"child" processes
(although
> we
> > stretched this rule in comments a bit, so it's fine there).
> > I'd probably make the comment a bit more explicit about the use case thoguh.
> I'd
> > say something like:
> > --
> > In some cases, os stats can only be dumped from a priviledged process to get
> > around to sandboxing/selinux restrictions (see crbug.com/461788) 
> > --
> 
> Done.
> 
> Thanks!

I anticipate landing the filling CLs today so I'm going to try to land this now!

[hjd, 2017-03-24 11:02:19]

The CQ bit was checked by hjd@chromium.org

[hjd, 2017-03-24 11:02:19]

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/2755343003/#ps60001
(title: "remove one field and rename one field")

[commit-bot: I haz the power, 2017-03-24 11:02:35]

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-24 12:29:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490353339160360,
"parent_rev": "73f22088eb2c94df42edd55abd9cf1e345e18e7c", "commit_rev":
"cbfd644ca2d24fc324101ea15e6198ca71c804c1"}

[commit-bot: I haz the power, 2017-03-24 12:30:36]

Description was changed from

==========
memory-infra: Add a struct to carry memory use information

This (currently unused) struct will carry information back to the
memory coordinator service where it will eventually be collated
and make its way to UMA.

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

to

==========
memory-infra: Add a struct to carry memory use information

This (currently unused) struct will carry information back to the
memory coordinator service where it will eventually be collated
and make its way to UMA.

BUG=703184

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

[commit-bot: I haz the power, 2017-03-24 12:30:37]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/cbfd644ca2d24fc324101ea15e61...