Only enable heap tracking under --enable-heap-profiling=task-profiler.

Only enable heap tracking under --enable-heap-profiling=task-profiler.

BUG=692045
Review-Url: https://codereview.chromium.org/2695013005
Cr-Commit-Position: refs/heads/master@{#450984}
Committed: https://chromium.googlesource.com/chromium/src/+/014a8015a054e4f8b21f7b1a163297bd9f12c22a

[Sigurður Ásgeirsson, 2017-02-14 18:36:22]

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

[commit-bot: I haz the power, 2017-02-14 18:37:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2017-02-14 18:44:27]

siggi@chromium.org changed reviewers:
+ gab@chromium.org, primiano@chromium.org

[Sigurður Ásgeirsson, 2017-02-14 18:44:27]

Hey Primiano,

this enables heap tracking under a flag, as you suggested in the bug. Seems to
work for browser and renderers, but I get nothing from utility process(es),
which is presumably an existing problem with propagating the state to that
process type? Shouldn't block this fix, though.

Siggi

[commit-bot: I haz the power, 2017-02-14 20:14:39]

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

[commit-bot: I haz the power, 2017-02-14 20:14:40]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2017-02-15 14:18:03]

Awesome, thanks so much for the promptness :)
LGTM with a minor comment.

+dskiba/ssid to keep them in the loop: business as usual here, nothing new. We
are just overloading the --enable-heap-profiler=task-profiler to enable the
siggi's monitor without introducing yet another cmdline flag. It's not really a
"heap profiler" in the conventional sense, as in, does not keep track of
per-allocations callsites/types etc.

https://codereview.chromium.org/2695013005/diff/1/base/base_switches.cc
File base/base_switches.cc (right):

https://codereview.chromium.org/2695013005/diff/1/base/base_switches.cc#newco...
base/base_switches.cc:27: // Report heap usage and churn in the task profiler
(chrome://profiler).
can you add: "Does not keep track of individual allocations unlike the default
and native mode. Keeps only track of summarized churn stats in the task profiler
(chrome://profiler)"

https://codereview.chromium.org/2695013005/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2695013005/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:204: #if
BUILDFLAG(ENABLE_MEMORY_TASK_PROFILER)
I'd probably put the #if around the "else if", so you can avoid the further
CHECK below and let it just fall through the other CHECK in the else path
already there in lines 214-217

[ssid, 2017-02-15 14:24:43]

I am trying to change where heap profiler is initialized in this cl. I don't see
any issues, just making sure it's fine.
https://codereview.chromium.org/2683913002/

[Sigurður Ásgeirsson, 2017-02-15 15:30:42]

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

[commit-bot: I haz the power, 2017-02-15 15:30:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2017-02-15 15:35:45]

Thanks - committing...

https://codereview.chromium.org/2695013005/diff/1/base/base_switches.cc
File base/base_switches.cc (right):

https://codereview.chromium.org/2695013005/diff/1/base/base_switches.cc#newco...
base/base_switches.cc:27: // Report heap usage and churn in the task profiler
(chrome://profiler).
On 2017/02/15 14:18:03, Primiano Tucci wrote:
> can you add: "Does not keep track of individual allocations unlike the default
> and native mode. Keeps only track of summarized churn stats in the task
profiler
> (chrome://profiler)"

Done.

https://codereview.chromium.org/2695013005/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2695013005/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:204: #if
BUILDFLAG(ENABLE_MEMORY_TASK_PROFILER)
On 2017/02/15 14:18:03, Primiano Tucci wrote:
> I'd probably put the #if around the "else if", so you can avoid the further
> CHECK below and let it just fall through the other CHECK in the else path
> already there in lines 214-217

Done.

[commit-bot: I haz the power, 2017-02-15 17:38:14]

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

[commit-bot: I haz the power, 2017-02-15 17:38:15]

Dry run: This issue passed the CQ dry run.

[gab, 2017-02-15 21:07:53]

//base rubberstamp lgtm

https://codereview.chromium.org/2695013005/diff/20001/base/base_switches.cc
File base/base_switches.cc (right):

https://codereview.chromium.org/2695013005/diff/20001/base/base_switches.cc#n...
base/base_switches.cc:30: // (chrome://profiler)"
s/"/./ ?

[Sigurður Ásgeirsson, 2017-02-16 13:42:43]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-02-16 13:42:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2695013005/#ps40001
(title: "Address Gab's nit.")

[Sigurður Ásgeirsson, 2017-02-16 13:42:51]

Thanks.

https://codereview.chromium.org/2695013005/diff/20001/base/base_switches.cc
File base/base_switches.cc (right):

https://codereview.chromium.org/2695013005/diff/20001/base/base_switches.cc#n...
base/base_switches.cc:30: // (chrome://profiler)"
On 2017/02/15 21:07:53, gab wrote:
> s/"/./ ?

Oops, copy/paste gaffe. Thanks.

[commit-bot: I haz the power, 2017-02-16 13:43:02]

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-02-16 14:57:32]

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

[commit-bot: I haz the power, 2017-02-16 14:57:33]

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

[Sigurður Ásgeirsson, 2017-02-16 15:13:16]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2017-02-16 15:13:37]

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-02-16 16:23:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487257996695870,
"parent_rev": "6ac59c8f2a159832fa261d9bfe2d027953ee7641", "commit_rev":
"014a8015a054e4f8b21f7b1a163297bd9f12c22a"}

[commit-bot: I haz the power, 2017-02-16 16:24:41]

Description was changed from

==========
Only enable heap tracking under --enable-heap-profiling=task-profiler.

BUG=692045
==========

to

==========
Only enable heap tracking under --enable-heap-profiling=task-profiler.

BUG=692045

Review-Url: https://codereview.chromium.org/2695013005
Cr-Commit-Position: refs/heads/master@{#450984}
Committed:
https://chromium.googlesource.com/chromium/src/+/014a8015a054e4f8b21f7b1a1632...
==========

[commit-bot: I haz the power, 2017-02-16 16:24:42]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/014a8015a054e4f8b21f7b1a1632...