[tracing] Introduce config to setup memory-infra peak detection

[tracing] Introduce config to setup memory-infra peak detection

BUG=607533
Committed: https://crrev.com/bca7b746fb1be034e5073b123ff76524ef0018f3
Cr-Commit-Position: refs/heads/master@{#439380}

[ssid, 2016-12-02 02:16:26]

Patchset #2 (id:20001) has been deleted

[ssid, 2016-12-02 02:17:05]

Patchset #1 (id:1) has been deleted

[ssid, 2016-12-02 02:20:46]

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

[ssid, 2016-12-02 02:20:46]

ptal thanks

[Primiano Tucci (use gerrit), 2016-12-05 17:57:35]

Thanks a lot for gradually split the CL.
Instead of having use_peak_detection at the globlal triggers level can we have
it within each trigger?
Let me explain:
I understand today we don't want to support periodic + peak, it is fine. But
tomorrow we might want to get there (for telemetry).
However in this way we are screwed because you are saying: either all triggers
are periodic or peak.
Instead can we have something explicit *within* each trigger.
Maybe a string is more future proof at this point. I'd go for
triggers: [
 { 
   mode: "light"/"detailed",
   min_time_between_peak_dumps: 1234,
   type: "periodic"/"peak"
 }
]


For backwards compatibility type will be "periodic" if you specified an
interval.
Also it is fine today to add a check that prevents having a config where you
have both a periodic and a peak trigger.
But at least if we'll change our mind we can relax that in future.

[ssid, 2016-12-12 22:28:43]

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

[ssid, 2016-12-12 22:29:16]

Made changes as suggested, ptal. thanks

[commit-bot: I haz the power, 2016-12-12 22:33:10]

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, 2016-12-12 23:36:03]

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

[commit-bot: I haz the power, 2016-12-12 23:36:04]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ssid, 2016-12-13 01:29:17]

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

[commit-bot: I haz the power, 2016-12-13 01:30:11]

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, 2016-12-13 01:42:00]

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

[commit-bot: I haz the power, 2016-12-13 01:42:00]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ssid, 2016-12-14 06:46:41]

Description was changed from

==========
[tracing] Introduce config to setup memory-infra peak detection

BUG=
==========

to

==========
[tracing] Introduce config to setup memory-infra peak detection

BUG=607533
==========

[Primiano Tucci (use gerrit), 2016-12-15 15:40:43]

Thanks, it all makes great sense (I don't even have objections on the names :P)
I just have one question on the parsing logic in trace_config.cc, see below
about the missing continue.

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

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_request_args.h:23: TASK_BEGIN,         // Dumping
memory at the beginning of a message-loop task.
since you are touching this, can you plz remove TASK_BEGIN/END. We ended up
never using them (imagine triggering a dump on each task, LOL) and I always
forgot to remove them.
Don't worry about numeric values of the enum changing (i.e. PERIODIC now being
0). We use the strings in trace-viewer and nothing should rely on the real int
value of this.

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/trace_...
File base/trace_event/trace_config.cc (right):

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:54: const char kPeriodicIntervalParam[] =
"periodic_interval_ms";  // legacy config
maybe add Legacy to the name (kPeriodicIntervalLegacyParam)?

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:602:
trigger->GetInteger(kPeriodicIntervalParam, &interval);
don't you have to keep the path where you skip if you don't pass neither the new
(min_time) nor the legacy param?
I.e. shouldn't this be

if (!trigger->getInteger(kPeriodicIntervalParam)
  continue
?

[ssid, 2016-12-15 19:48:07]

done, thanks!

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

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_request_args.h:23: TASK_BEGIN,         // Dumping
memory at the beginning of a message-loop task.
On 2016/12/15 15:40:43, Primiano Tucci wrote:
> since you are touching this, can you plz remove TASK_BEGIN/END. We ended up
> never using them (imagine triggering a dump on each task, LOL) and I always
> forgot to remove them.
> Don't worry about numeric values of the enum changing (i.e. PERIODIC now being
> 0). We use the strings in trace-viewer and nothing should rely on the real int
> value of this.

Done.

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/trace_...
File base/trace_event/trace_config.cc (right):

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:54: const char kPeriodicIntervalParam[] =
"periodic_interval_ms";  // legacy config
On 2016/12/15 15:40:43, Primiano Tucci wrote:
> maybe add Legacy to the name (kPeriodicIntervalLegacyParam)?

Done.

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:602:
trigger->GetInteger(kPeriodicIntervalParam, &interval);
On 2016/12/15 15:40:43, Primiano Tucci wrote:
> don't you have to keep the path where you skip if you don't pass neither the
new
> (min_time) nor the legacy param?
> I.e. shouldn't this be
> 
> if (!trigger->getInteger(kPeriodicIntervalParam)
>   continue
> ?

um, this will happen if someone writes:
memory_dump_config : { triggers [
  { "something_else": 12 }
]}
There is a dcheck to verify if the interval is set. So, lets just crash instead.

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/trace...
File base/trace_event/trace_config_memory_test_util.h (right):

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/trace...
base/trace_event/trace_config_memory_test_util.h:2: // Use of this source code
is governed by a BSD-style license that can be
I had to format this file. The presubmit doesn't let it go anymore :/

[ssid, 2016-12-16 03:17:01]

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

[commit-bot: I haz the power, 2016-12-16 03:17:50]

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, 2016-12-16 06:27:01]

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

[commit-bot: I haz the power, 2016-12-16 06:27:02]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-12-16 12:13:55]

Ok some final comments. thanks

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/trace_...
File base/trace_event/trace_config.cc (right):

https://codereview.chromium.org/2542853002/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:602:
trigger->GetInteger(kPeriodicIntervalParam, &interval);
On 2016/12/15 19:48:07, ssid wrote:
> On 2016/12/15 15:40:43, Primiano Tucci wrote:
> > don't you have to keep the path where you skip if you don't pass neither the
> new
> > (min_time) nor the legacy param?
> > I.e. shouldn't this be
> > 
> > if (!trigger->getInteger(kPeriodicIntervalParam)
> >   continue
> > ?
> 
> um, this will happen if someone writes:
> memory_dump_config : { triggers [
>   { "something_else": 12 }
> ]}
> There is a dcheck to verify if the interval is set. So, lets just crash
instead.

Yeah I just didn't see the "!", sorry ignore my previous comment.
Just find this backward compat logic a bit hard to reason about.
Wouldn't be a bit more logical saying:

"if you are not providing the |type| you must be using the old-style config (in
which case you check for the presence of the legacy param). Otherwise you must
be passing a type and the new |min_interval| key"

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:846:
DCHECK_EQ(MemoryDumpType::PERIODIC_INTERVAL, config.trigger_type)
just double checking, this is something that will go away soon (with peaks),
right?

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/trace...
File base/trace_event/trace_config_memory_test_util.h (right):

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/trace...
base/trace_event/trace_config_memory_test_util.h:2: // Use of this source code
is governed by a BSD-style license that can be
On 2016/12/15 19:48:07, ssid wrote:
> I had to format this file. The presubmit doesn't let it go anymore :/

As per recently we can now use C++11 string literals (I think this is why clang
format now is stricter).
Since you are touching this can you make these string literals?
e.g.:

return StringPrintf(R"(
  {
     "enable_argument_filter": false 
     ...
   }
)", ...)

see
https://cs.chromium.org/chromium/src/gpu/command_buffer/tests/gl_program_unit...
as an example

[ssid, 2016-12-16 19:19:52]

made no changes. please see comment.

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:846:
DCHECK_EQ(MemoryDumpType::PERIODIC_INTERVAL, config.trigger_type)
On 2016/12/16 12:13:55, Primiano Tucci wrote:
> just double checking, this is something that will go away soon (with peaks),
> right?

yes this will be removed in next cls.

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/trace...
File base/trace_event/trace_config_memory_test_util.h (right):

https://codereview.chromium.org/2542853002/diff/100001/base/trace_event/trace...
base/trace_event/trace_config_memory_test_util.h:2: // Use of this source code
is governed by a BSD-style license that can be
On 2016/12/16 12:13:55, Primiano Tucci wrote:
> On 2016/12/15 19:48:07, ssid wrote:
> > I had to format this file. The presubmit doesn't let it go anymore :/
> 
> As per recently we can now use C++11 string literals (I think this is why
clang
> format now is stricter).
> Since you are touching this can you make these string literals?
> e.g.:
> 
> return StringPrintf(R"(
>   {
>      "enable_argument_filter": false 
>      ...
>    }
> )", ...)
> 
> see
>
https://cs.chromium.org/chromium/src/gpu/command_buffer/tests/gl_program_unit...
> as an example

Nope that does not work because the tests are doing:
TraceConfig(str).ToString() == str
But declaring string with this style gives spaces in the strings.

[Primiano Tucci (use gerrit), 2016-12-16 20:25:18]

Ahh right. Lgtm then

[ssid, 2016-12-17 06:14:55]

ssid@chromium.org changed reviewers:
+ dgozman@chromium.org

[ssid, 2016-12-17 06:14:56]

dgozman@chromium.org: Ptal small changes in devtools test.

[dgozman, 2016-12-17 15:53:31]

devtools lgtm

[ssid, 2016-12-18 20:23:45]

The CQ bit was checked by ssid@chromium.org

[commit-bot: I haz the power, 2016-12-18 20:23:52]

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, 2016-12-18 22:13:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482092625348100,
"parent_rev": "e79f0e54425f1116957164211674e5351895f940", "commit_rev":
"f9f46f2136973c83ca86a8b18855eea9a3d2c058"}

[commit-bot: I haz the power, 2016-12-18 22:13:38]

Description was changed from

==========
[tracing] Introduce config to setup memory-infra peak detection

BUG=607533
==========

to

==========
[tracing] Introduce config to setup memory-infra peak detection

BUG=607533

Review-Url: https://codereview.chromium.org/2542853002
==========

[commit-bot: I haz the power, 2016-12-18 22:13:39]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-12-18 22:15:55]

Description was changed from

==========
[tracing] Introduce config to setup memory-infra peak detection

BUG=607533

Review-Url: https://codereview.chromium.org/2542853002
==========

to

==========
[tracing] Introduce config to setup memory-infra peak detection

BUG=607533

Committed: https://crrev.com/bca7b746fb1be034e5073b123ff76524ef0018f3
Cr-Commit-Position: refs/heads/master@{#439380}
==========

[commit-bot: I haz the power, 2016-12-18 22:15:56]

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