[tracing] Add memory dump config to TraceConfig

[tracing] Add memory dump config to TraceConfig

Currently memory-infra creates dump at fixed rate. The rate and level
of detail should be configurable for telemetry and slow-reports. This
CL introduces memory dump config in TraceConfig, to be able to control
the periodic interval and dump type.
See design doc https://goo.gl/5Hj3o0.

BUG=513692
TBR=dsinclair@chromium.org, jhawkins@chromium.org

Committed: https://crrev.com/00d111c262fc3bc3b0354ef747bb714c109634fa
Cr-Commit-Position: refs/heads/master@{#345651}

[ssid, 2015-08-21 11:43:13]

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

[ssid, 2015-08-21 11:43:13]

PTAL, Thanks

[Primiano Tucci (use gerrit), 2015-08-24 11:58:36]

Makes sense to me with some comments here and there.
Once you address them, please add zhenw@ and have him review the CL as well.
Even if I am an owner here I don't know the full details of trace_config, so
would be nice to have a review from him as well.

Thanks.

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

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:66: // TODO(primiano): this should be
smarter and should do something similar to
You can remove this todo in the next cl when you actually switch to traceconfig.
The "smarter" thing here was precisely the TraceConfig :)

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:73: const char* kTraceCategory
= MemoryDumpManager::kTraceCategory;
At this point this level of indirection is not needed anymore, as test fixtures
can access kTraceCategory themselves.

i.e. EnableTracing(MemoryDumpManager::kTraceCategory)

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

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:315: return;
probably in this case you might not want an early return, better an if.
It looks likely that sometbody might add code below. if that is the case, this
early return might subtly break things.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:453: base::DictionaryValue& memory_dump_config)
{
+const

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:457: // If triggers are not set, use the
default config.
Don't you want to just disable the periodic dumps if they are empty?

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:470: int interval;
= 0;

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:486: dump_config.dump_level_of_detail =
MemoryDumpArgs::LevelOfDetail::LOW;
I'd probably rework this by making LOW the default assignment (before the if of
line 478) and then assign high only if the dump_type is not null and == "heavy"

This should be backwards compatible, if in future we introduce level="foo" and
foo does not exist in this version of chrome, level of detail here will not be
left unassigned

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:536: return;
same reason of above here, not sure if I'f I'd do the early return here.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:555: NOTREACHED();
I think that you don't need the default: case here, as per our code style:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...

* If not conditional on an enumerated value, switch statements should always
have a default case (in the case of an enumerated value, the compiler will warn
you if any values are not handled).

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

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:40: // DISABLED_BY_DEFAULT("memory-infra")
category is enabled.
I'd just say "memory-infra" category, so if tomorrow become enabled by default
this comment isn't left behind inconsistent.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:41: struct MemoryDumpConfig {
Shouldn't this be named MemoryDumpTriggerConfig?

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:43: MemoryDumpArgs::LevelOfDetail
dump_level_of_detail;
i'd omit the "dump_" part in the name (i.e. just level_of_defail), as this is
already a Memory**Dump**Config

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:46: typedef std::vector<MemoryDumpConfig>
MemoryDumpConfigList;
And this just MemoryDumpConfig?

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:118: //           "dump_type": "heavy",
Why not "level_of_detail"? So it is consistent with the above.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:196: void
SetMemoryDumpConfigList(base::DictionaryValue& memory_dump_config);
Shouldn't this be just SetMemoryDumpConfig (without "List")? It is not just a
list.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:196: void
SetMemoryDumpConfigList(base::DictionaryValue& memory_dump_config);
+const base::....

[ssid, 2015-08-24 15:29:31]

Thanks, made changes.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:73: const char* kTraceCategory
= MemoryDumpManager::kTraceCategory;
On 2015/08/24 11:58:35, Primiano Tucci wrote:
> At this point this level of indirection is not needed anymore, as test
fixtures
> can access kTraceCategory themselves.
> 
> i.e. EnableTracing(MemoryDumpManager::kTraceCategory)

Done.

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

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:315: return;
On 2015/08/24 11:58:35, Primiano Tucci wrote:
> probably in this case you might not want an early return, better an if.
> It looks likely that sometbody might add code below. if that is the case, this
> early return might subtly break things.

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:453: base::DictionaryValue& memory_dump_config)
{
On 2015/08/24 11:58:35, Primiano Tucci wrote:
> +const

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:457: // If triggers are not set, use the
default config.
On 2015/08/24 11:58:36, Primiano Tucci wrote:
> Don't you want to just disable the periodic dumps if they are empty?

yes, I forgot that part, changed.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:470: int interval;
On 2015/08/24 11:58:35, Primiano Tucci wrote:
> = 0;

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:486: dump_config.dump_level_of_detail =
MemoryDumpArgs::LevelOfDetail::LOW;
On 2015/08/24 11:58:36, Primiano Tucci wrote:
> I'd probably rework this by making LOW the default assignment (before the if
of
> line 478) and then assign high only if the dump_type is not null and ==
"heavy"
> 
> This should be backwards compatible, if in future we introduce level="foo" and
> foo does not exist in this version of chrome, level of detail here will not be
> left unassigned

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:536: return;
On 2015/08/24 11:58:35, Primiano Tucci wrote:
> same reason of above here, not sure if I'f I'd do the early return here.

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.cc:555: NOTREACHED();
On 2015/08/24 11:58:35, Primiano Tucci wrote:
> I think that you don't need the default: case here, as per our code style:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...
> 
> * If not conditional on an enumerated value, switch statements should always
> have a default case (in the case of an enumerated value, the compiler will
warn
> you if any values are not handled).

Yes, but I added the default case since the other enums in this file have
default case. Just added to be consistent with them.

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

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:40: // DISABLED_BY_DEFAULT("memory-infra")
category is enabled.
On 2015/08/24 11:58:36, Primiano Tucci wrote:
> I'd just say "memory-infra" category, so if tomorrow become enabled by default
> this comment isn't left behind inconsistent.

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:41: struct MemoryDumpConfig {
On 2015/08/24 11:58:36, Primiano Tucci wrote:
> Shouldn't this be named MemoryDumpTriggerConfig?

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:43: MemoryDumpArgs::LevelOfDetail
dump_level_of_detail;
On 2015/08/24 11:58:36, Primiano Tucci wrote:
> i'd omit the "dump_" part in the name (i.e. just level_of_defail), as this is
> already a Memory**Dump**Config

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:46: typedef std::vector<MemoryDumpConfig>
MemoryDumpConfigList;
On 2015/08/24 11:58:36, Primiano Tucci wrote:
> And this just MemoryDumpConfig?

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:118: //           "dump_type": "heavy",
On 2015/08/24 11:58:36, Primiano Tucci wrote:
> Why not "level_of_detail"? So it is consistent with the above.

Done.

https://codereview.chromium.org/1306753005/diff/80001/base/trace_event/trace_...
base/trace_event/trace_config.h:196: void
SetMemoryDumpConfigList(base::DictionaryValue& memory_dump_config);
On 2015/08/24 11:58:36, Primiano Tucci wrote:
> Shouldn't this be just SetMemoryDumpConfig (without "List")? It is not just a
> list.

Done.

[ssid, 2015-08-24 15:30:04]

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

[ssid, 2015-08-24 15:30:04]

PTAL. Thanks

[Zhen Wang, 2015-08-24 18:38:24]

lgtm for trace_config.h/cc with nits.

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

https://codereview.chromium.org/1306753005/diff/100001/base/trace_event/trace...
base/trace_event/trace_config.h:112: //                            
"disabled-by-default-memory-infra"],
disabled-by-default-memory-infra must be specified if one wants to use
memory_dump_config, right? It is not clear in the example if those two are
tightly related. Can you add a note after the example?

[ssid, 2015-08-24 19:48:10]

Made changes. Thanks.
primiano@ PTAL.

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

https://codereview.chromium.org/1306753005/diff/100001/base/trace_event/trace...
base/trace_event/trace_config.h:112: //                            
"disabled-by-default-memory-infra"],
On 2015/08/24 18:38:24, Zhen Wang wrote:
> disabled-by-default-memory-infra must be specified if one wants to use
> memory_dump_config, right? It is not clear in the example if those two are
> tightly related. Can you add a note after the example?

Done.

[Primiano Tucci (use gerrit), 2015-08-24 21:26:47]

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config.h (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:125: // Note: memory_dump_config can be
specified only if
ssid: according to our last chat offline this comment is not true anymore,
right?
Ditto for the if check below

[ssid, 2015-08-24 22:46:26]

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config.h (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:125: // Note: memory_dump_config can be
specified only if
On 2015/08/24 21:26:47, Primiano Tucci wrote:
> ssid: according to our last chat offline this comment is not true anymore,
> right?
> Ditto for the if check below

For this CL this is true. This will be removed once I change the function of the
category. This means telemetry has to be updated to not include memory-infra,
which i am not changing in this CL. So, will prepare new CL for that.

[ssid, 2015-08-24 22:53:23]

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config.h (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:125: // Note: memory_dump_config can be
specified only if
On 2015/08/24 22:46:26, ssid wrote:
> On 2015/08/24 21:26:47, Primiano Tucci wrote:
> > ssid: according to our last chat offline this comment is not true anymore,
> > right?
> > Ditto for the if check below
> 
> For this CL this is true. This will be removed once I change the function of
the
> category. This means telemetry has to be updated to not include memory-infra,
> which i am not changing in this CL. So, will prepare new CL for that.

Hm, this is true even after the change, right? The change is going to be:
if memory-infra is enabled, if dump config is specified, use that, if not
specified, do not disable periodic dumps and use default config. This still
means if config is specified then category should be enabled.

[Primiano Tucci (use gerrit), 2015-08-25 18:27:37]

Had some long offline chat on this, just few minimal changes. 
thanks

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

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:66: // TODO(primiano): this should be
smarter and should do something similar to
This comment should be: remove this soon as we migrate to TraceConfig

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config.cc (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.cc:313: 
This should gracefully handle also the legacy category.
Something like:

  if (GetDictionary) {
     just read the dictionary, ignore disabled-by-default-memory-infra category
  } else if (disabled-by-default-memory-infra) {
    SetDefaultDumpCOnfig...
  }

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.cc:387: memory_dump_config_.clear();
Move these 3 lines into a SetDefaultMemoryDefaultConfig or similar, as you will
need this up there.

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.cc:527: if
(IsCategoryEnabled(MemoryDumpManager::kTraceCategory) &&
remove this check here.
instead you have to make sure that you propagate (only if set) also the
disabled-by-default-memory-infra category

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config.h (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:112: //                            
"disabled-by-default-memory-infra"],
remove this line

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:118: //           "level_of_detail": "high",
s/level_of_detail/mode/
high -> detailed
low -> light

I'll rename the rest of the codebase to match.

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:126: // disabled-by-default-memory-infra
category is enabled.
Had offline chat, remove this comment

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config_unittest.cc (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config_unittest.cc:545: TEST(TraceConfigTest,
InvalidMemoryConfigString) {
Not sure if this makes sense anymore. Instead have a check that ensures that if
you start with a string and then to toString <> fromstring, the legacy category
is still there.

[ssid, 2015-08-26 10:19:51]

PTAL, Thanks

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config.cc (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.cc:313: 
On 2015/08/25 18:27:37, Primiano Tucci wrote:
> This should gracefully handle also the legacy category.
> Something like:
> 
>   if (GetDictionary) {
>      just read the dictionary, ignore disabled-by-default-memory-infra
category
>   } else if (disabled-by-default-memory-infra) {
>     SetDefaultDumpCOnfig...
>   }

Done.

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.cc:387: memory_dump_config_.clear();
On 2015/08/25 18:27:37, Primiano Tucci wrote:
> Move these 3 lines into a SetDefaultMemoryDefaultConfig or similar, as you
will
> need this up there.

Done.

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.cc:527: if
(IsCategoryEnabled(MemoryDumpManager::kTraceCategory) &&
On 2015/08/25 18:27:37, Primiano Tucci wrote:
> remove this check here.
> instead you have to make sure that you propagate (only if set) also the
> disabled-by-default-memory-infra category

Done.

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config.h (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:112: //                            
"disabled-by-default-memory-infra"],
On 2015/08/25 18:27:37, Primiano Tucci wrote:
> remove this line

Done.

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:118: //           "level_of_detail": "high",
On 2015/08/25 18:27:37, Primiano Tucci wrote:
> s/level_of_detail/mode/
> high -> detailed
> low -> light
> 
> I'll rename the rest of the codebase to match.

Done.

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config.h:126: // disabled-by-default-memory-infra
category is enabled.
On 2015/08/25 18:27:37, Primiano Tucci wrote:
> Had offline chat, remove this comment

Done.

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
File base/trace_event/trace_config_unittest.cc (right):

https://codereview.chromium.org/1306753005/diff/120001/base/trace_event/trace...
base/trace_event/trace_config_unittest.cc:545: TEST(TraceConfigTest,
InvalidMemoryConfigString) {
On 2015/08/25 18:27:37, Primiano Tucci wrote:
> Not sure if this makes sense anymore. Instead have a check that ensures that
if
> you start with a string and then to toString <> fromstring, the legacy
category
> is still there.

Made new test for this reason.

[ssid, 2015-08-26 12:04:48]

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

[commit-bot: I haz the power, 2015-08-26 12:04:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306753005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306753005/160001

[Primiano Tucci (use gerrit), 2015-08-26 12:44:40]

LGTM ship it :)
sorry for the long back and forths.

[Primiano Tucci (use gerrit), 2015-08-26 12:44:54]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-08-26 12:44:54]

The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org
Link to the patchset: https://codereview.chromium.org/1306753005/#ps200001
(title: "Build fix.")

[commit-bot: I haz the power, 2015-08-26 12:44:57]

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

[Primiano Tucci (use gerrit), 2015-08-26 12:45:50]

TBR-ing dsinclair as the change in
content/browser/tracing/memory_tracing_browsertest.cc is just removing a
"forTest" string.

[Primiano Tucci (use gerrit), 2015-08-26 12:46:11]

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

[ssid, 2015-08-26 14:48:56]

On 2015/08/26 12:45:50, Primiano Tucci wrote:
> TBR-ing dsinclair as the change in
> content/browser/tracing/memory_tracing_browsertest.cc is just removing a
> "forTest" string.

TBR-ing jhawkins for the same reason.

[ssid, 2015-08-26 14:49:01]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-08-26 14:49:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, zhenw@chromium.org
Link to the patchset: https://codereview.chromium.org/1306753005/#ps220001
(title: "Fix build.")

[commit-bot: I haz the power, 2015-08-26 14:49:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306753005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306753005/220001

[commit-bot: I haz the power, 2015-08-26 16:33:47]

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

[commit-bot: I haz the power, 2015-08-26 16:33:48]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Primiano Tucci (use gerrit), 2015-08-26 20:05:36]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2015-08-26 20:06:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306753005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306753005/220001

[commit-bot: I haz the power, 2015-08-26 20:12:46]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-08-26 20:14:03]

Patchset 12 (id:??) landed as
https://crrev.com/00d111c262fc3bc3b0354ef747bb714c109634fa
Cr-Commit-Position: refs/heads/master@{#345651}