[OBSOLETE] tracing: split trace event filter logic out of TraceLog

[OBSOLETE] tracing: split trace event filter logic out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and simplifying dependencies of the tracing codebase.
This CL moves all the trace filters registry logic outside of the
TraceLog. On top of the mechanical refactoring, the major change
being introduced is event filters being now indefinitely lived.
This reduces the complexity of the interaction between TraceLog and
the filters, making them similar to the pattern used by TraceCategory.
There doesn't seem to be any actual need to unregister filters and hence
there is no need to keep worrying about their lifetime.

BUG=659689

[Primiano Tucci (use gerrit), 2016-11-11 19:36:29]

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

[commit-bot: I haz the power, 2016-11-11 19:36:47]

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

[Primiano Tucci (use gerrit), 2016-11-11 19:38:33]

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

[Primiano Tucci (use gerrit), 2016-11-11 19:38:34]

Simplifying the hell out of TraceLog. Take 2.

[Dirk Pranke, 2016-11-11 20:10:14]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-11-11 20:10:14]

bootstrap.py change lgtm :).

[commit-bot: I haz the power, 2016-11-11 23:14:27]

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

[commit-bot: I haz the power, 2016-11-11 23:14:28]

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

[Primiano Tucci (use gerrit), 2016-11-13 07:03:17]

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

[commit-bot: I haz the power, 2016-11-13 07:03:24]

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-11-13 09:27:13]

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

[commit-bot: I haz the power, 2016-11-13 09:27:16]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-11-14 13:28:52]

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

[commit-bot: I haz the power, 2016-11-14 13:29:06]

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

[Primiano Tucci (use gerrit), 2016-11-14 13:32:52]

Description was changed from

==========
tracing: split trace event filter logic out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and simplifying dependencies of the tracing codebase.
This CL moves all the trace filters registry logic outside of the
TraceLog. On top of the mechanical refactoring, event filters are
now undefinitely lived. This reduces the complexity of the
interaction between TraceLog and the filters, making them similar
to the pattern used by TraceCategory. There doesn't seem to be
any actual need to unregister filters and hence there is no need
to keep worrying about their lifetime.

BUG=659689
==========

to

==========
tracing: split trace event filter logic out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and simplifying dependencies of the tracing codebase.
This CL moves all the trace filters registry logic outside of the
TraceLog. On top of the mechanical refactoring, the major change
being introduced is event filters being now undefinitely lived.
This reduces the complexity of the interaction between TraceLog and
the filters, making them similar to the pattern used by TraceCategory.
There doesn't seem to be any actual need to unregister filters and hence
there is no need to keep worrying about their lifetime.

BUG=659689
==========

[Primiano Tucci (use gerrit), 2016-11-14 13:33:02]

Description was changed from

==========
tracing: split trace event filter logic out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and simplifying dependencies of the tracing codebase.
This CL moves all the trace filters registry logic outside of the
TraceLog. On top of the mechanical refactoring, the major change
being introduced is event filters being now undefinitely lived.
This reduces the complexity of the interaction between TraceLog and
the filters, making them similar to the pattern used by TraceCategory.
There doesn't seem to be any actual need to unregister filters and hence
there is no need to keep worrying about their lifetime.

BUG=659689
==========

to

==========
tracing: split trace event filter logic out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and simplifying dependencies of the tracing codebase.
This CL moves all the trace filters registry logic outside of the
TraceLog. On top of the mechanical refactoring, the major change
being introduced is event filters being now indefinitely lived.
This reduces the complexity of the interaction between TraceLog and
the filters, making them similar to the pattern used by TraceCategory.
There doesn't seem to be any actual need to unregister filters and hence
there is no need to keep worrying about their lifetime.

BUG=659689
==========

[Primiano Tucci (use gerrit), 2016-11-14 13:43:11]

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

[commit-bot: I haz the power, 2016-11-14 13:43:15]

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

[Primiano Tucci (use gerrit), 2016-11-14 14:16:14]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-11-14 15:07:40]

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

[commit-bot: I haz the power, 2016-11-14 15:07:41]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-11-16 10:19:44]

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
File base/trace_event/event_filter_registry.h (right):

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
base/trace_event/event_filter_registry.h:21: // registered are leaked forever.
This guarnatees that the clients can hold onto
The last time we discussed, Oystein was not happy about leaking the filter
objects. But, previously I had to leak more than one EventNameFilter.

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
base/trace_event/event_filter_registry.h:28: static FILTER_TYPE*
RegisterFilterLocked() {
Register feels like it should be called once, maybe GetOrRegister or
GetInstance?

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
File base/trace_event/event_name_filter.cc (right):

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
base/trace_event/event_name_filter.cc:27: return
event_names_whitelist_->count(trace_event.name()) != 0;
This is not thread safe. All threads will access the event_names_whitelist_
without lock when a TRACE_EVENT is hit. If filtering is enabled twice with
EventNameFilter then this list is changed and trace events could be accessing
the list.

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/trace_...
File base/trace_event/trace_event_filter.h (right):

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/trace_...
base/trace_event/trace_event_filter.h:26: // TraceEventFilter instances are
undefinitely lived: once created are leaked
indefinitely

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/trace_...
base/trace_event/trace_event_filter.h:27: // forever, although don't have to be
necessarily enabled.
A reference to event_filter_registry would be nice, to understand how this class
can be used?

[Primiano Tucci (use gerrit), 2016-11-16 16:56:22]

Hm hold on on revieweing this, I just realized that I need to revisit it a bit.
Right now this allows at most only one instance of each filter class.
Which is ok for the heap profiler, but for the Event name filter we might want
multiple instance, with different whitelist for each category.

Ok now I understand where the objection about leaking them came from. We cannot
definitely leak something that can have an arbitrary number of instances. I was
just thinking to the case of a fixed number of instances, but this is definitely
not enough.
Will think more on this.

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
File base/trace_event/event_filter_registry.h (right):

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
base/trace_event/event_filter_registry.h:21: // registered are leaked forever.
This guarnatees that the clients can hold onto
On 2016/11/16 10:19:44, ssid wrote:
> The last time we discussed, Oystein was not happy about leaking the filter
> objects. But, previously I had to leak more than one EventNameFilter.

Ah, I was not aware of the conversation. Honestly leaking them seems to me the
best way to not have races on stop/start.
How many filters do we realistically expect to have? If not that much this might
be the simples and most robust solution.

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
base/trace_event/event_filter_registry.h:28: static FILTER_TYPE*
RegisterFilterLocked() {
On 2016/11/16 10:19:44, ssid wrote:
> Register feels like it should be called once, maybe GetOrRegister or
> GetInstance?

Makes sense, will change this soon.

[DmitrySkiba, 2016-11-29 17:55:45]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2016-11-29 17:55:46]

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
File base/trace_event/event_name_filter.h (right):

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/event_...
base/trace_event/event_name_filter.h:37: std::unique_ptr<EventNamesWhitelist>
event_names_whitelist_;
Why unique_ptr indirection? What's wrong with just storing EventNamesWhilelist?

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/trace_...
File base/trace_event/trace_event_filter.h (right):

https://codereview.chromium.org/2499583003/diff/60001/base/trace_event/trace_...
base/trace_event/trace_event_filter.h:42: virtual void EndEvent(const char*
category_name,
What is the difference between FilterTraceEvent(TRACE_EVENT_PHASE_END) and
EndEvent() ?

[Primiano Tucci (use gerrit), 2016-12-06 12:27:01]

Abandoning this CL in favor of a slighly differnt and more gradual approach.
Apologies for the double review.

First some refactoring-only coming in 
https://codereview.chromium.org/2549103003/
And then real change in https://codereview.chromium.org/2557743002

[Primiano Tucci (use gerrit), 2016-12-06 12:27:17]

Description was changed from

==========
tracing: split trace event filter logic out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and simplifying dependencies of the tracing codebase.
This CL moves all the trace filters registry logic outside of the
TraceLog. On top of the mechanical refactoring, the major change
being introduced is event filters being now indefinitely lived.
This reduces the complexity of the interaction between TraceLog and
the filters, making them similar to the pattern used by TraceCategory.
There doesn't seem to be any actual need to unregister filters and hence
there is no need to keep worrying about their lifetime.

BUG=659689
==========

to

==========
[OBSOLETE] tracing: split trace event filter logic out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and simplifying dependencies of the tracing codebase.
This CL moves all the trace filters registry logic outside of the
TraceLog. On top of the mechanical refactoring, the major change
being introduced is event filters being now indefinitely lived.
This reduces the complexity of the interaction between TraceLog and
the filters, making them similar to the pattern used by TraceCategory.
There doesn't seem to be any actual need to unregister filters and hence
there is no need to keep worrying about their lifetime.

BUG=659689
==========