[tracing] Add a dump provider to dump process memory totals.

[tracing] Add a dump provider to dump process memory totals.

This CL introduces a dump provider which is able to capture
the process Resident Set Size and the corresponding unittest.

BUG=442731, 458295
Committed: https://crrev.com/c96eca7cc1b3a9c6de89b4928637dfa9412e74b5
Cr-Commit-Position: refs/heads/master@{#317336}

[Primiano Tucci (use gerrit), 2015-02-18 18:24:21]

Patchset #1 (id:1) has been deleted

[Primiano Tucci (use gerrit), 2015-02-18 18:24:28]

Patchset #1 (id:20001) has been deleted

[Primiano Tucci (use gerrit), 2015-02-18 18:24:49]

primiano@chromium.org changed reviewers:
+ dsinclair@chromium.org, nduca@chromium.org

[dsinclair, 2015-02-18 18:42:31]

https://codereview.chromium.org/934323002/diff/40001/base/trace_event/process...
File base/trace_event/process_memory_totals_dump_provider.cc (right):

https://codereview.chromium.org/934323002/diff/40001/base/trace_event/process...
base/trace_event/process_memory_totals_dump_provider.cc:33: const uint64
rss_bytes = process_metrics_->GetWorkingSetSize();
So, do the process_metrics_ get updated or are they set at the time when we
create the ProcessMemoryTotalsDumpProvider? If they are set, can we just store
rss_size instead of the whole object?

[Primiano Tucci (use gerrit), 2015-02-18 18:48:00]

https://codereview.chromium.org/934323002/diff/40001/base/trace_event/process...
File base/trace_event/process_memory_totals_dump_provider.cc (right):

https://codereview.chromium.org/934323002/diff/40001/base/trace_event/process...
base/trace_event/process_memory_totals_dump_provider.cc:33: const uint64
rss_bytes = process_metrics_->GetWorkingSetSize();
On 2015/02/18 18:42:30, dsinclair wrote:
> So, do the process_metrics_ get updated or are they set at the time when we
> create the ProcessMemoryTotalsDumpProvider? If they are set, can we just store
> rss_size instead of the whole object?

process_metrics_ is the (base::) object which is able to retrieve memory
counters for the system (i.e. the same that chrome://memory uses)
For what concerns memory, its only state is the ProcessHandle (i.e. the PID) of
the target process (self). It keeps also other state for CPU measurements (the
last ticks to get the delta) but we never use that part.

At the end, anyways, we have only one instance of ProcessMetrics per chrome
process, which is costing few some extra bytes (~6 PODs), so it shouldn't be a
great deal.

[dsinclair, 2015-02-18 18:51:49]

lgtm

[nduca, 2015-02-18 20:10:52]

lgtm assuming the AsValueInto part happens

https://codereview.chromium.org/934323002/diff/40001/base/trace_event/process...
File base/trace_event/process_memory_totals.h (right):

https://codereview.chromium.org/934323002/diff/40001/base/trace_event/process...
base/trace_event/process_memory_totals.h:24: scoped_ptr<Value> AsValue() const;
Please switch to the the AsValueInto design pattern from cc/. MDM would create
the toplevel trace value builder, then call AsValueinto passing around the value
object. There are eventual performance reasons why that approach is better:
namely, with value construction, the actual operator-new becomes the
[horrifyingly heavy] bottleneck. The builder interface can be changed to not
actually have a value internally and instead push into a pickle, eliminating all
the mallocs.

[Primiano Tucci (use gerrit), 2015-02-19 10:04:27]

New patchsets have been uploaded after l-g-t-m from
dsinclair@chromium.org,nduca@chromium.org

[Primiano Tucci (use gerrit), 2015-02-19 10:56:26]

Patchset #2 (id:60001) has been deleted

[Primiano Tucci (use gerrit), 2015-02-19 10:56:39]

Patchset #2 (id:80001) has been deleted

[Primiano Tucci (use gerrit), 2015-02-19 11:46:44]

Patchset #2 (id:100001) has been deleted

[Primiano Tucci (use gerrit), 2015-02-19 11:49:31]

Patchset #2 (id:120001) has been deleted

[Primiano Tucci (use gerrit), 2015-02-19 11:50:27]

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

[Primiano Tucci (use gerrit), 2015-02-19 11:50:28]

+skyostil for the AsValueInto migration

[Sami, 2015-02-19 12:07:05]

AsValueInto pattern lgtm.

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

https://codereview.chromium.org/934323002/diff/140001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:103: scoped_ptr<ProcessMemoryDump>
pmd(new ProcessMemoryDump());
Is this cast trickery now obsolete?

https://codereview.chromium.org/934323002/diff/140001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/934323002/diff/140001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:30: ProcessMemoryTotals* process_totals()
{ return &process_totals_; }
nit: both of these could be const?

[Primiano Tucci (use gerrit), 2015-02-19 12:17:54]

New patchsets have been uploaded after l-g-t-m from skyostil@chromium.org

[Primiano Tucci (use gerrit), 2015-02-19 12:18:13]

https://codereview.chromium.org/934323002/diff/40001/base/trace_event/process...
File base/trace_event/process_memory_totals.h (right):

https://codereview.chromium.org/934323002/diff/40001/base/trace_event/process...
base/trace_event/process_memory_totals.h:24: scoped_ptr<Value> AsValue() const;
On 2015/02/18 20:10:52, nduca wrote:
> Please switch to the the AsValueInto design pattern from cc/. MDM would create
> the toplevel trace value builder, then call AsValueinto passing around the
value
> object. There are eventual performance reasons why that approach is better:
> namely, with value construction, the actual operator-new becomes the
> [horrifyingly heavy] bottleneck. The builder interface can be changed to not
> actually have a value internally and instead push into a pickle, eliminating
all
> the mallocs.

Done. thanks for the observation.
Yeah, actually there was no need to reinvent AppendAsTraceFormat as TracedValue
does that for free and keeps the code here more generic.

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

https://codereview.chromium.org/934323002/diff/140001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:103: scoped_ptr<ProcessMemoryDump>
pmd(new ProcessMemoryDump());
On 2015/02/19 12:07:05, Sami wrote:
> Is this cast trickery now obsolete?

Oh right (FTR: skyostil@ was referring to line 106 below)

https://codereview.chromium.org/934323002/diff/140001/base/trace_event/proces...
File base/trace_event/process_memory_dump.h (right):

https://codereview.chromium.org/934323002/diff/140001/base/trace_event/proces...
base/trace_event/process_memory_dump.h:30: ProcessMemoryTotals* process_totals()
{ return &process_totals_; }
On 2015/02/19 12:07:05, Sami wrote:
> nit: both of these could be const?

Only the 2nd. process_totals is meant to be modified. hence it has to return a
non-const ProcessMemoryTotals*, hence the method itself must be non const
(otherwise "this" would be const)

[Primiano Tucci (use gerrit), 2015-02-19 12:18:16]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-02-19 12:18:29]

The CQ bit was unchecked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-02-19 12:21:24]

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

[Primiano Tucci (use gerrit), 2015-02-19 12:21:25]

+danakj for gyp/gn build files OWNERS stamp.

[Sami, 2015-02-19 16:16:36]

Still lgtm.

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

https://codereview.chromium.org/934323002/diff/140001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:103: scoped_ptr<ProcessMemoryDump>
pmd(new ProcessMemoryDump());
On 2015/02/19 12:18:12, Primiano Tucci wrote:
> On 2015/02/19 12:07:05, Sami wrote:
> > Is this cast trickery now obsolete?
> 
> Oh right (FTR: skyostil@ was referring to line 106 below)

Right, and also to the comment above this line :) I guess you can update it
while adding the code to actually save the dump into the trace.

[danakj, 2015-02-19 16:50:37]

lgtm

[Primiano Tucci (use gerrit), 2015-02-20 15:29:17]

New patchsets have been uploaded after l-g-t-m from
skyostil@chromium.org,danakj@chromium.org

[Primiano Tucci (use gerrit), 2015-02-20 15:29:57]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2015-02-20 15:30:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/934323002/200001

[commit-bot: I haz the power, 2015-02-20 16:41:23]

Committed patchset #5 (id:200001)

[commit-bot: I haz the power, 2015-02-20 16:42:08]

Patchset 5 (id:??) landed as
https://crrev.com/c96eca7cc1b3a9c6de89b4928637dfa9412e74b5
Cr-Commit-Position: refs/heads/master@{#317336}