[tracing] Add trace events filtering predicate for heap profiler

[tracing] Add trace events filtering predicate for heap profiler

The heap profiler gets context for each allocation only from trace
events whose category is enabled for recording. This CL changes:
1. The AddTraceEvent was called only for trace events with categories
   that are enabled for recording modes, but now it gets called for all
   categories including ones specified in filters.
2. Trace events are not added to traces if the category is enabled for
   filtering but not enabled for recording, irrespective of the filter
   result.
3. Add new filter "heap_profiler_predicate" that tracks trace events and
   adds to heap profiler pseudo stack. This filter returns true only for
   the events enabled for recording mode (only the recording mode is
   affected by filters, not etw and event callback modes).
4. When "memory-infra" is enabled with heap profiler flag turned on,
   then the heap profiler filter is installed by default and filters all
   categories and adds to trace only if the category is enabled for
   recording.

This ensures that the heap profiler filter always gets all trace events
and they only get added to trace file if they are enabled for recording,
or they are enabled by other filters.

BUG=598426, 625170
Committed: https://crrev.com/f35fbb8a6b310e118dc2e04d29849f27613a4754
Cr-Commit-Position: refs/heads/master@{#414210}

[ssid, 2016-08-18 02:11:14]

Patchset #1 (id:1) has been deleted

[ssid, 2016-08-18 02:18:17]

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

[commit-bot: I haz the power, 2016-08-18 02:18:43]

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-08-18 02:52:13]

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

[commit-bot: I haz the power, 2016-08-18 02:52:16]

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

[ssid, 2016-08-18 03:44:47]

Patchset #1 (id:20001) has been deleted

[ssid, 2016-08-18 03:45:02]

Patchset #1 (id:40001) has been deleted

[ssid, 2016-08-18 03:45:10]

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

[commit-bot: I haz the power, 2016-08-18 03:45:37]

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-08-18 04:46:04]

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

[commit-bot: I haz the power, 2016-08-18 04:46:05]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-08-18 17:57:35]

Patchset #1 (id:60001) has been deleted

[ssid, 2016-08-18 18:03:12]

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

[ssid, 2016-08-18 18:03:12]

ptal, thanks.

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

https://codereview.chromium.org/2259493003/diff/80001/base/trace_event/trace_...
base/trace_event/trace_event.h:80:
base::trace_event::TraceLog::ENABLED_FOR_FILTERING))
As far as I can see there is no harm in adding this line here since there is a
check in AddTraceEventWithThreadIdAndTimestamp and metadata events aren't
enabled for filtering.
I cannot change the name of this macro since it is being used by
trace_event_common.h

[ssid, 2016-08-22 21:32:23]

Description was changed from

==========
[tracing] Add trace events filtering predicate for heap profiler

The heap profiler gets context for each allocation only from trace
events whose category is enabled. This CL changes:
1. The AddTraceEvent was called only for trace events with categories
   that are enabled for recording modes, but now it gets called for all
   categories including ones specified in filters.
2. Add new filter "heap_profiler_predicate" that tracks trace events and
   adds to heap profiler pseudo stack. This filter returns true only for
   the events enabled for recording mode (only the recording mode is
   affected by filters, not etw and event callback modes).
3. When "memory-infra" is enabled with heap profiler flag turned on,
   then the heap profiler filter is installed by default and filters all
   categories and adds to trace only if the category is enabled for
   recording.

This ensures that the heap profiler filter always gets all trace events
and they only get added to trace file if they are enabled for recording,
or they are enabled by other filters.

BUG=598426,625170
==========

to

==========
[tracing] Add trace events filtering predicate for heap profiler

The heap profiler gets context for each allocation only from trace
events whose category is enabled for recording. This CL changes:
1. The AddTraceEvent was called only for trace events with categories
   that are enabled for recording modes, but now it gets called for all
   categories including ones specified in filters.
2. Trace events are not added to traces if the category is enabled for
   filtering but not enabled for recording, irrespective of the filter
   result.
3. Add new filter "heap_profiler_predicate" that tracks trace events and
   adds to heap profiler pseudo stack. This filter returns true only for
   the events enabled for recording mode (only the recording mode is
   affected by filters, not etw and event callback modes).
4. When "memory-infra" is enabled with heap profiler flag turned on,
   then the heap profiler filter is installed by default and filters all
   categories and adds to trace only if the category is enabled for
   recording.

This ensures that the heap profiler filter always gets all trace events
and they only get added to trace file if they are enabled for recording,
or they are enabled by other filters.

BUG=598426, 625170
==========

[ssid, 2016-08-22 21:58:35]

Patchset #2 (id:100001) has been deleted

[oystein (OOO til 10th of July), 2016-08-23 17:42:43]

trace_log bits lgtm w/ a nit, can't speak for the memory-infra-specific bits!

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

https://codereview.chromium.org/2259493003/diff/120001/base/trace_event/trace...
base/trace_event/trace_log.cc:576: new_filter = WrapUnique(new
HeapProfilerFilter());
nit: Recently learned from chromium-dev that MakeUnique is preferred over
WrapUnique

[ssid, 2016-08-23 23:05:25]

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

[commit-bot: I haz the power, 2016-08-23 23:06:16]

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-08-24 00:22:56]

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

[commit-bot: I haz the power, 2016-08-24 00:22:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ssid, 2016-08-24 01:26:35]

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

[commit-bot: I haz the power, 2016-08-24 01:27: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-08-24 02:41:52]

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

[commit-bot: I haz the power, 2016-08-24 02:41:53]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Primiano Tucci (use gerrit), 2016-08-24 10:33:47]

Ok this tripped my mind for an hour, but I'm ultimately convinced that is
correct. Thanks a lot for doing this.
Mostly LGTM (just minor comments, looks like you forgot an if-check along the
road).

Can I just ask you to add a comment to TraceEventFilter that explains the
filtering semantic. I could review this because we had a fresh discussion. But
I'm pretty sure that if I try to reconstruct the intent of this filter 2 months
from now I will end up sitting in a corner and crying :)

Please explain in that comment:
- what a filter does
- what is the semantic of the returned bool
- what is the different semantic of RECORDING vs FILTERING and what happens if
you have only one of them.
- clarify that the returned value of the filter has effect only if the category
is enabled for recording.

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

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
base/trace_event/trace_config.cc:641: // Note that the heap profiler predicate
does not filter any events.
s/filter/filter-out/, so it's easier to understand that what you mean here is
the fact that you let all filters pass through.

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
base/trace_event/trace_config.cc:645:
heap_profiler_config.AddIncludedCategory(MemoryDumpManager::kTraceCategory);
do you really need to see the memory-infra events for the heap profiler?

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
base/trace_event/trace_log.cc:142: if (!(trace_event.flags() &
TRACE_EVENT_FLAG_COPY)) {
Can you reverse the condition of this if and early out?
In general makes code more readable and reduces indentation.
I'd write this as:
if (capture_mode() != AllocationContextTracker::CaptureMode::PSEUDO_STACK)
 return true;

if (trace_event.flags() & TRACE_EVENT_FLAG_COPY)
  return true;

if (BEGIN||COMPLETE) ...
if (PHASE_END) ...

return true;

Also add a comment explaining why we always return true.

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
base/trace_event/trace_log.cc:158: void EndEvent(const char* name, const char*
category_group) override {
dont' you need to check for CaptureMode::PSEUDO_STACK also here?

[ssid, 2016-08-24 18:01:26]

On 2016/08/24 10:33:47, Primiano Tucci wrote:
> Ok this tripped my mind for an hour, but I'm ultimately convinced that is
> correct. Thanks a lot for doing this.
> Mostly LGTM (just minor comments, looks like you forgot an if-check along the
> road).
> 
> Can I just ask you to add a comment to TraceEventFilter that explains the
> filtering semantic. I could review this because we had a fresh discussion. But
> I'm pretty sure that if I try to reconstruct the intent of this filter 2
months
> from now I will end up sitting in a corner and crying :)
> 
> Please explain in that comment:
> - what a filter does
> - what is the semantic of the returned bool
> - what is the different semantic of RECORDING vs FILTERING and what happens if
> you have only one of them.
> - clarify that the returned value of the filter has effect only if the
category
> is enabled for recording.

Yes, I should have added more comments. Done.

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

https://codereview.chromium.org/2259493003/diff/120001/base/trace_event/trace...
base/trace_event/trace_log.cc:576: new_filter = WrapUnique(new
HeapProfilerFilter());
On 2016/08/23 17:42:42, oystein wrote:
> nit: Recently learned from chromium-dev that MakeUnique is preferred over
> WrapUnique

Done.

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

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
base/trace_event/trace_config.cc:641: // Note that the heap profiler predicate
does not filter any events.
On 2016/08/24 10:33:46, Primiano Tucci wrote:
> s/filter/filter-out/, so it's easier to understand that what you mean here is
> the fact that you let all filters pass through.

Done.

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
base/trace_event/trace_config.cc:645:
heap_profiler_config.AddIncludedCategory(MemoryDumpManager::kTraceCategory);
On 2016/08/24 10:33:46, Primiano Tucci wrote:
> do you really need to see the memory-infra events for the heap profiler?

Actually this line is not required. But I added this just to be safe.
There could have been a case where the "memory-infra" events were enabled for
filtering but not being filtered by "heap_profiler_predicate" and some other
filter removed these events. This might cause the memory dump events to go
missing from the trace.

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
base/trace_event/trace_log.cc:142: if (!(trace_event.flags() &
TRACE_EVENT_FLAG_COPY)) {
On 2016/08/24 10:33:47, Primiano Tucci wrote:
> Can you reverse the condition of this if and early out?
> In general makes code more readable and reduces indentation.
> I'd write this as:
> if (capture_mode() != AllocationContextTracker::CaptureMode::PSEUDO_STACK)
>  return true;
> 
> if (trace_event.flags() & TRACE_EVENT_FLAG_COPY)
>   return true;
> 
> if (BEGIN||COMPLETE) ...
> if (PHASE_END) ...
> 
> return true;
> 
> Also add a comment explaining why we always return true.

Makes sense. Thanks.

https://codereview.chromium.org/2259493003/diff/140001/base/trace_event/trace...
base/trace_event/trace_log.cc:158: void EndEvent(const char* name, const char*
category_group) override {
On 2016/08/24 10:33:46, Primiano Tucci wrote:
> dont' you need to check for CaptureMode::PSEUDO_STACK also here?

Ouch, fixed!

[ssid, 2016-08-24 18:01:55]

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

[commit-bot: I haz the power, 2016-08-24 18:03:00]

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-08-24 19:40:28]

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

[commit-bot: I haz the power, 2016-08-24 19:40:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ssid, 2016-08-24 22:13:47]

The CQ bit was checked by ssid@chromium.org

[ssid, 2016-08-24 22:13:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2259493003/#ps160001
(title: "Primiano's comments.")

[commit-bot: I haz the power, 2016-08-24 22:15:16]

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-08-25 00:04:46]

Description was changed from

==========
[tracing] Add trace events filtering predicate for heap profiler

The heap profiler gets context for each allocation only from trace
events whose category is enabled for recording. This CL changes:
1. The AddTraceEvent was called only for trace events with categories
   that are enabled for recording modes, but now it gets called for all
   categories including ones specified in filters.
2. Trace events are not added to traces if the category is enabled for
   filtering but not enabled for recording, irrespective of the filter
   result.
3. Add new filter "heap_profiler_predicate" that tracks trace events and
   adds to heap profiler pseudo stack. This filter returns true only for
   the events enabled for recording mode (only the recording mode is
   affected by filters, not etw and event callback modes).
4. When "memory-infra" is enabled with heap profiler flag turned on,
   then the heap profiler filter is installed by default and filters all
   categories and adds to trace only if the category is enabled for
   recording.

This ensures that the heap profiler filter always gets all trace events
and they only get added to trace file if they are enabled for recording,
or they are enabled by other filters.

BUG=598426, 625170
==========

to

==========
[tracing] Add trace events filtering predicate for heap profiler

The heap profiler gets context for each allocation only from trace
events whose category is enabled for recording. This CL changes:
1. The AddTraceEvent was called only for trace events with categories
   that are enabled for recording modes, but now it gets called for all
   categories including ones specified in filters.
2. Trace events are not added to traces if the category is enabled for
   filtering but not enabled for recording, irrespective of the filter
   result.
3. Add new filter "heap_profiler_predicate" that tracks trace events and
   adds to heap profiler pseudo stack. This filter returns true only for
   the events enabled for recording mode (only the recording mode is
   affected by filters, not etw and event callback modes).
4. When "memory-infra" is enabled with heap profiler flag turned on,
   then the heap profiler filter is installed by default and filters all
   categories and adds to trace only if the category is enabled for
   recording.

This ensures that the heap profiler filter always gets all trace events
and they only get added to trace file if they are enabled for recording,
or they are enabled by other filters.

BUG=598426, 625170
==========

[commit-bot: I haz the power, 2016-08-25 00:04:47]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-08-25 00:07:41]

Description was changed from

==========
[tracing] Add trace events filtering predicate for heap profiler

The heap profiler gets context for each allocation only from trace
events whose category is enabled for recording. This CL changes:
1. The AddTraceEvent was called only for trace events with categories
   that are enabled for recording modes, but now it gets called for all
   categories including ones specified in filters.
2. Trace events are not added to traces if the category is enabled for
   filtering but not enabled for recording, irrespective of the filter
   result.
3. Add new filter "heap_profiler_predicate" that tracks trace events and
   adds to heap profiler pseudo stack. This filter returns true only for
   the events enabled for recording mode (only the recording mode is
   affected by filters, not etw and event callback modes).
4. When "memory-infra" is enabled with heap profiler flag turned on,
   then the heap profiler filter is installed by default and filters all
   categories and adds to trace only if the category is enabled for
   recording.

This ensures that the heap profiler filter always gets all trace events
and they only get added to trace file if they are enabled for recording,
or they are enabled by other filters.

BUG=598426, 625170
==========

to

==========
[tracing] Add trace events filtering predicate for heap profiler

The heap profiler gets context for each allocation only from trace
events whose category is enabled for recording. This CL changes:
1. The AddTraceEvent was called only for trace events with categories
   that are enabled for recording modes, but now it gets called for all
   categories including ones specified in filters.
2. Trace events are not added to traces if the category is enabled for
   filtering but not enabled for recording, irrespective of the filter
   result.
3. Add new filter "heap_profiler_predicate" that tracks trace events and
   adds to heap profiler pseudo stack. This filter returns true only for
   the events enabled for recording mode (only the recording mode is
   affected by filters, not etw and event callback modes).
4. When "memory-infra" is enabled with heap profiler flag turned on,
   then the heap profiler filter is installed by default and filters all
   categories and adds to trace only if the category is enabled for
   recording.

This ensures that the heap profiler filter always gets all trace events
and they only get added to trace file if they are enabled for recording,
or they are enabled by other filters.

BUG=598426, 625170

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

[commit-bot: I haz the power, 2016-08-25 00:07:42]

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