Chrome tracing for system-wide performance stats.

Chrome tracing for system-wide performance stats.

Trace memory, swap, ZRAM, and disk activity and
place the data in JSON to be displayed by trace-viewer.

BUG=236763
TEST=base_unittests TraceSystemStatsMonitorTest.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223806

[nduca, 2013-08-28 23:45:40]

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.cc (right):

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:52: /*
dont commit dead code.

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:60: *prev_system_metrics =
system_metrics_;
Remove the prev_blahblah stuff until you add in the deltas in a follwoup patch

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:71:
TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("system_memory_and_disk"), "init");
You can do this just by getting the category itself ---
base::Debug::TraceLog()->GetCategorySomethingOrOther(DISABLED_BY_DEFAULT("..."));

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:90: DVLOG(1) <<
"OnTraceLogEnabled";
dont commit loggin

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:123: DVLOG(1) <<
"DumpSystemProfile";
dont commit logging

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:130: first_profile_ = false;
remove this until you add this stuff

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:134:
TRACE_DISABLED_BY_DEFAULT("system_memory_and_disk"),
you need a name for this system. oyu're calling it system_Stats some places and
system_memory_and_disk, and in js system stats monitor. I'd suggest
system_stats, and use it everywhere.

https://codereview.chromium.org/22836004/diff/10001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:135: "system_stats",
this should be base::TraceEventSystemStatsMonitor::SystemStats --- naked names
are discouraged in tracing

[jwmak, 2013-09-06 22:39:36]

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
File base/debug/trace_event_system_stats_monitor.cc (right):

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
base/debug/trace_event_system_stats_monitor.cc:52: /*
On 2013/08/28 23:45:40, nduca wrote:
> dont commit dead code.

Done.

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
base/debug/trace_event_system_stats_monitor.cc:60: *prev_system_metrics =
system_metrics_;
On 2013/08/28 23:45:40, nduca wrote:
> Remove the prev_blahblah stuff until you add in the deltas in a follwoup patch

Done.

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
base/debug/trace_event_system_stats_monitor.cc:71:
TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("system_memory_and_disk"), "init");
On 2013/08/28 23:45:40, nduca wrote:
> You can do this just by getting the category itself ---
>
base::Debug::TraceLog()->GetCategorySomethingOrOther(DISABLED_BY_DEFAULT("..."));

Done.

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
base/debug/trace_event_system_stats_monitor.cc:90: DVLOG(1) <<
"OnTraceLogEnabled";
On 2013/08/28 23:45:40, nduca wrote:
> dont commit loggin

Done.

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
base/debug/trace_event_system_stats_monitor.cc:123: DVLOG(1) <<
"DumpSystemProfile";
On 2013/08/28 23:45:40, nduca wrote:
> dont commit logging

Done.

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
base/debug/trace_event_system_stats_monitor.cc:130: first_profile_ = false;
On 2013/08/28 23:45:40, nduca wrote:
> remove this until you add this stuff

Done.

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
base/debug/trace_event_system_stats_monitor.cc:134:
TRACE_DISABLED_BY_DEFAULT("system_memory_and_disk"),
On 2013/08/28 23:45:40, nduca wrote:
> you need a name for this system. oyu're calling it system_Stats some places
and
> system_memory_and_disk, and in js system stats monitor. I'd suggest
> system_stats, and use it everywhere.

Done.

https://chromiumcodereview.appspot.com/22836004/diff/10001/base/debug/trace_e...
base/debug/trace_event_system_stats_monitor.cc:135: "system_stats",
On 2013/08/28 23:45:40, nduca wrote:
> this should be base::TraceEventSystemStatsMonitor::SystemStats --- naked names
> are discouraged in tracing

Done.

[nduca, 2013-09-07 07:19:20]

nearly lg, you just need the id to be set up right. using "this" should just
work. did it not?

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.cc (right):

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:108: const int kSnapshotId = 1;
just use this as your id. it wont collide that way.

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.h (right):

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.h:29: static const int
kSystemStatsMonitorIntervalSeconds = 2;
I'd suggest making this unit in milliseconds, e.g. 2000, and use
TimeTicks::FromMilliseconds in the other. That way if we ever want to turn it
up, we can turn it up faster than 1s intervals. :)

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.h:43: void StartProfiling();
this doesn't have to be public --- start/stop and dump can all be private.

I'dkill the comments. we generally only add comments when they add value. its
sort of like defining a word using itself, ya know?

[nduca, 2013-09-07 07:19:38]

(dsinclair is a good resource to im with while im in LON)

[jwmak, 2013-09-08 09:26:05]

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.cc (right):

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:108: const int kSnapshotId = 1;
On 2013/09/07 07:19:20, nduca wrote:
> just use this as your id. it wont collide that way.

Done.

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.h (right):

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.h:29: static const int
kSystemStatsMonitorIntervalSeconds = 2;
On 2013/09/07 07:19:20, nduca wrote:
> I'd suggest making this unit in milliseconds, e.g. 2000, and use
> TimeTicks::FromMilliseconds in the other. That way if we ever want to turn it
> up, we can turn it up faster than 1s intervals. :)

Done.

https://codereview.chromium.org/22836004/diff/17001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.h:43: void StartProfiling();
On 2013/09/07 07:19:20, nduca wrote:
> this doesn't have to be public --- start/stop and dump can all be private.
> 
> I'dkill the comments. we generally only add comments when they add value. its
> sort of like defining a word using itself, ya know?

Done.

[nduca, 2013-09-09 11:04:43]

lgtm

[dsinclair, 2013-09-09 14:14:22]

lgtm with nit.

https://codereview.chromium.org/22836004/diff/25001/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/22836004/diff/25001/content/browser/browser_m...
content/browser/browser_main_loop.cc:500:
base::MessageLoop::current()->message_loop_proxy()));
nit: indent 2 more.

[darin (slow to review), 2013-09-12 20:51:15]

https://codereview.chromium.org/22836004/diff/31001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.cc (right):

https://codereview.chromium.org/22836004/diff/31001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:46: system_stats_ =
SystemMetrics::Sample();
nit: insert new line before close of namespace.

https://codereview.chromium.org/22836004/diff/31001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor_unittest.cc (right):

https://codereview.chromium.org/22836004/diff/31001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor_unittest.cc:13: 
nit: extra new line

https://codereview.chromium.org/22836004/diff/31001/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/22836004/diff/31001/content/browser/browser_m...
content/browser/browser_main_loop.cc:500:
base::MessageLoop::current()->message_loop_proxy()));
nit: New code should really be using base::ThreadTaskRunnerHandle. That works
much the same way as MessageLoopProxy so it should be a simple change for this
CL.

[jwmak, 2013-09-13 20:57:10]

https://codereview.chromium.org/22836004/diff/31001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.cc (right):

https://codereview.chromium.org/22836004/diff/31001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.cc:46: system_stats_ =
SystemMetrics::Sample();
On 2013/09/12 20:51:15, darin wrote:
> nit: insert new line before close of namespace.

Done.

https://codereview.chromium.org/22836004/diff/31001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor_unittest.cc (right):

https://codereview.chromium.org/22836004/diff/31001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor_unittest.cc:13: 
On 2013/09/12 20:51:15, darin wrote:
> nit: extra new line

Done.

https://codereview.chromium.org/22836004/diff/31001/content/browser/browser_m...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/22836004/diff/31001/content/browser/browser_m...
content/browser/browser_main_loop.cc:500:
base::MessageLoop::current()->message_loop_proxy()));
On 2013/09/12 20:51:15, darin wrote:
> nit: New code should really be using base::ThreadTaskRunnerHandle. That works
> much the same way as MessageLoopProxy so it should be a simple change for this
> CL.

Done.

[darin (slow to review), 2013-09-17 04:17:58]

LGTM, but just one suggestion:

https://codereview.chromium.org/22836004/diff/37001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.h (right):

https://codereview.chromium.org/22836004/diff/37001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.h:25: class BASE_EXPORT
TraceEventSystemStatsMonitor
Had you considered a shorter name for this class? How about just calling it
TraceEventSystemMonitor? It seems like "Stats" doesn't add much color to the
name.

[darin (slow to review), 2013-09-17 04:18:54]

Also, I'm curious about the overall design here. Are we putting too much tracing
code into src/base? Have you all considered a design where src/base has only the
bits that are needed by src/base and the rest gets punted to a higher level
library? I could imagine src/debug might be warranted.

[jwmak, 2013-09-17 05:39:38]

https://codereview.chromium.org/22836004/diff/37001/base/debug/trace_event_sy...
File base/debug/trace_event_system_stats_monitor.h (right):

https://codereview.chromium.org/22836004/diff/37001/base/debug/trace_event_sy...
base/debug/trace_event_system_stats_monitor.h:25: class BASE_EXPORT
TraceEventSystemStatsMonitor
On 2013/09/17 04:17:59, darin wrote:
> Had you considered a shorter name for this class? How about just calling it
> TraceEventSystemMonitor? It seems like "Stats" doesn't add much color to the
> name.

The name could be shortened, but there's already a SystemMonitor class in base
that's not related to this one (the other one monitors subsystems such as power
management, network status etc.)

[commit-bot: I haz the power, 2013-09-17 19:06:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwmak@chromium.org/22836004/48001

[commit-bot: I haz the power, 2013-09-17 19:17:38]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-17 19:50:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwmak@chromium.org/22836004/58001

[commit-bot: I haz the power, 2013-09-17 20:01:56]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-18 01:18:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwmak@chromium.org/22836004/85009

[darin (slow to review), 2013-09-18 07:31:41]

On 2013/09/17 05:39:38, jwmak wrote:
>
https://codereview.chromium.org/22836004/diff/37001/base/debug/trace_event_sy...
> File base/debug/trace_event_system_stats_monitor.h (right):
> 
>
https://codereview.chromium.org/22836004/diff/37001/base/debug/trace_event_sy...
> base/debug/trace_event_system_stats_monitor.h:25: class BASE_EXPORT
> TraceEventSystemStatsMonitor
> On 2013/09/17 04:17:59, darin wrote:
> > Had you considered a shorter name for this class? How about just calling it
> > TraceEventSystemMonitor? It seems like "Stats" doesn't add much color to the
> > name.
> 
> The name could be shortened, but there's already a SystemMonitor class in base
> that's not related to this one (the other one monitors subsystems such as
power
> management, network status etc.)

I see. Since this one is prefixed with TraceEvent, I'm not sure that
it would necessarily feel related to the top-level SystemMonitor class.

What about my other question? Why does this code belong in src/base?

[commit-bot: I haz the power, 2013-09-18 07:33:30]

Change committed as 223806

[nduca, 2013-09-18 21:15:59]

I suspect we could put it in content/. I think this is something that just
slipped through the cracks in the hurry to get it and the tcmalloc sampler
working.