[tracing] The CPU profiler should only be enabled for specific modes of tracing

[tracing] The CPU profiler should only be enabled for specific modes of tracing

We have different modes of tracing: recording, event callback and
filtering. The cpu profiler should not be enabled when tracing is
enabled with filtering mode.

BUG=688651
Review-Url: https://codereview.chromium.org/2676403002
Cr-Commit-Position: refs/heads/master@{#43119}
Committed: https://chromium.googlesource.com/v8/v8/+/21523c7832cdbd74fceacddf4660977eef02cd4c

[ssid, 2017-02-06 21:35:20]

Patchset #1 (id:1) has been deleted

[ssid, 2017-02-06 21:36:13]

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

[ssid, 2017-02-06 21:36:14]

+alph ptal, thanks

[ssid, 2017-02-06 21:36:34]

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

[commit-bot: I haz the power, 2017-02-06 21:36:43]

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

[ssid, 2017-02-06 21:37:03]

Description was changed from

==========
[tracing] The CPU profiler should only be enabled for specific modes of tracing

We have different modes of tracing: recording, event callback and
filtering. The cpu profiler should not be enabled when tracing is
enabled with filtering mode.

BUG=688651
==========

to

==========
[tracing] The CPU profiler should only be enabled for specific modes of tracing

We have different modes of tracing: recording, event callback and
filtering. The cpu profiler should not be enabled when tracing is
enabled with filtering mode.

BUG=688651
==========

[commit-bot: I haz the power, 2017-02-06 22:00:56]

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

[commit-bot: I haz the power, 2017-02-06 22:00:57]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2017-02-06 23:10:15]

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

[Primiano Tucci (use gerrit), 2017-02-06 23:10:16]

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
File src/profiler/tracing-cpu-profiler.cc (right):

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:14:
(kEnabledForRecording_CategoryGroupEnabledFlags | \
Don't we have the _enabled_for_recording macro  in trace event common? (on a
phone, can't check myself sorry)

[ssid, 2017-02-07 00:11:54]

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
File src/profiler/tracing-cpu-profiler.cc (right):

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:14:
(kEnabledForRecording_CategoryGroupEnabledFlags | \
On 2017/02/06 23:10:16, Primiano Tucci wrote:
> Don't we have the _enabled_for_recording macro  in trace event common? (on a
> phone, can't check myself sorry) 

ENABLED_FOR_RECORDING is defined in base/trace_event/trace_category.h. Not
included by trace_event_common.

V8 seems to define flags in libplatform
https://chromium.googlesource.com/v8/v8.git/+/667ea83e08afd70b9f9fa15ab304ac7...
and the flags I am using here are defined in v8/src/tracing/trace-event.h.
I am not sure if i can include trace_event_common.h in here.

If you prefer to have the flags moved to trace_event_common.h then i can do it
as different cl and fix V8. Right now I think we need this to fix the crash in
heap profiling.

[Primiano Tucci (use gerrit), 2017-02-07 10:50:14]

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

[Primiano Tucci (use gerrit), 2017-02-07 10:50:14]

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
File src/profiler/tracing-cpu-profiler.cc (right):

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:14:
(kEnabledForRecording_CategoryGroupEnabledFlags | \
On 2017/02/07 00:11:54, ssid wrote:
> On 2017/02/06 23:10:16, Primiano Tucci wrote:
> > Don't we have the _enabled_for_recording macro  in trace event common? (on a
> > phone, can't check myself sorry) 
> 
> ENABLED_FOR_RECORDING is defined in base/trace_event/trace_category.h. Not
> included by trace_event_common.
> 
> V8 seems to define flags in libplatform
>
https://chromium.googlesource.com/v8/v8.git/+/667ea83e08afd70b9f9fa15ab304ac7...
> and the flags I am using here are defined in v8/src/tracing/trace-event.h.
> I am not sure if i can include trace_event_common.h in here.
> 
> If you prefer to have the flags moved to trace_event_common.h then i can do it
> as different cl and fix V8. Right now I think we need this to fix the crash in
> heap profiling.

So I looked at the code, the situation is the following:
TRACE_EVENT_CATEGORY_GROUP_ENABLED in trace_event_common.h (which is acessible
by v8) is defined as:
...  if (INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE()) ...

which is precisely what we want here.

in turn INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE is
already 
defined in v8/src/tracing/trace-event.h as:
 *INTERNAL_TRACE_EVENT_UID(category_group_enabled) &                    \
      (kEnabledForRecording_CategoryGroupEnabledFlags |                  \
       kEnabledForEventCallback_CategoryGroupEnabledFlags)

so here you are just reinventing existing code.
Unless I am missing something, the right fix here is to:
#define PROFILER_TRACE_CATEGORY_ENABLED(cat, enabled) 
TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT(cat), enabled)

and fix the code below

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:31:
PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profiler");
since you are here this should be
TRACE_EVENT_WARMUP_CATEGORY(TRACE_DISABLED_BY_DEFAULT("v8...."))

which is more readable

[alph, 2017-02-07 22:04:32]

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
File src/profiler/tracing-cpu-profiler.cc (right):

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:14:
(kEnabledForRecording_CategoryGroupEnabledFlags | \
On 2017/02/07 10:50:14, Primiano Tucci wrote:
> On 2017/02/07 00:11:54, ssid wrote:
> > On 2017/02/06 23:10:16, Primiano Tucci wrote:
> > > Don't we have the _enabled_for_recording macro  in trace event common? (on
a
> > > phone, can't check myself sorry) 
> > 
> > ENABLED_FOR_RECORDING is defined in base/trace_event/trace_category.h. Not
> > included by trace_event_common.
> > 
> > V8 seems to define flags in libplatform
> >
>
https://chromium.googlesource.com/v8/v8.git/+/667ea83e08afd70b9f9fa15ab304ac7...
> > and the flags I am using here are defined in v8/src/tracing/trace-event.h.
> > I am not sure if i can include trace_event_common.h in here.
> > 
> > If you prefer to have the flags moved to trace_event_common.h then i can do
it
> > as different cl and fix V8. Right now I think we need this to fix the crash
in
> > heap profiling.
> 
> So I looked at the code, the situation is the following:
> TRACE_EVENT_CATEGORY_GROUP_ENABLED in trace_event_common.h (which is acessible
> by v8) is defined as:
> ...  if (INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE()) ...
> 
> which is precisely what we want here.
> 
> in turn INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE is
> already 
> defined in v8/src/tracing/trace-event.h as:
>  *INTERNAL_TRACE_EVENT_UID(category_group_enabled) &                    \
>       (kEnabledForRecording_CategoryGroupEnabledFlags |                  \
>        kEnabledForEventCallback_CategoryGroupEnabledFlags)
> 
> so here you are just reinventing existing code.
> Unless I am missing something, the right fix here is to:
> #define PROFILER_TRACE_CATEGORY_ENABLED(cat, enabled) 
> TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT(cat), enabled)
> 
> and fix the code below

+1 for using TRACE_EVENT_CATEGORY_GROUP_ENABLED

[ssid, 2017-02-08 02:24:12]

Thanks! Made changes, ptal.

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
File src/profiler/tracing-cpu-profiler.cc (right):

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:14:
(kEnabledForRecording_CategoryGroupEnabledFlags | \
On 2017/02/07 22:04:31, alph wrote:
> On 2017/02/07 10:50:14, Primiano Tucci wrote:
> > On 2017/02/07 00:11:54, ssid wrote:
> > > On 2017/02/06 23:10:16, Primiano Tucci wrote:
> > > > Don't we have the _enabled_for_recording macro  in trace event common?
(on
> a
> > > > phone, can't check myself sorry) 
> > > 
> > > ENABLED_FOR_RECORDING is defined in base/trace_event/trace_category.h. Not
> > > included by trace_event_common.
> > > 
> > > V8 seems to define flags in libplatform
> > >
> >
>
https://chromium.googlesource.com/v8/v8.git/+/667ea83e08afd70b9f9fa15ab304ac7...
> > > and the flags I am using here are defined in v8/src/tracing/trace-event.h.
> > > I am not sure if i can include trace_event_common.h in here.
> > > 
> > > If you prefer to have the flags moved to trace_event_common.h then i can
do
> it
> > > as different cl and fix V8. Right now I think we need this to fix the
crash
> in
> > > heap profiling.
> > 
> > So I looked at the code, the situation is the following:
> > TRACE_EVENT_CATEGORY_GROUP_ENABLED in trace_event_common.h (which is
acessible
> > by v8) is defined as:
> > ...  if (INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE())
...
> > 
> > which is precisely what we want here.
> > 
> > in turn INTERNAL_TRACE_EVENT_CATEGORY_GROUP_ENABLED_FOR_RECORDING_MODE is
> > already 
> > defined in v8/src/tracing/trace-event.h as:
> >  *INTERNAL_TRACE_EVENT_UID(category_group_enabled) &                    \
> >       (kEnabledForRecording_CategoryGroupEnabledFlags |                  \
> >        kEnabledForEventCallback_CategoryGroupEnabledFlags)
> > 
> > so here you are just reinventing existing code.
> > Unless I am missing something, the right fix here is to:
> > #define PROFILER_TRACE_CATEGORY_ENABLED(cat, enabled) 
> > TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT(cat), enabled)
> > 
> > and fix the code below
> 
> +1 for using TRACE_EVENT_CATEGORY_GROUP_ENABLED

Ah I didn't realize what you meant. Thanks fixed.

https://codereview.chromium.org/2676403002/diff/20001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:31:
PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profiler");
On 2017/02/07 10:50:14, Primiano Tucci wrote:
> since you are here this should be
> TRACE_EVENT_WARMUP_CATEGORY(TRACE_DISABLED_BY_DEFAULT("v8...."))
> 
> which is more readable

Yes done.

[Primiano Tucci (use gerrit), 2017-02-09 11:56:27]

non-owner LGTM from the trace macros viewpoint.
+alph for OWNERS

[alph, 2017-02-09 18:23:56]

lgtm % nit

https://codereview.chromium.org/2676403002/diff/40001/src/profiler/tracing-cp...
File src/profiler/tracing-cpu-profiler.cc (right):

https://codereview.chromium.org/2676403002/diff/40001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:11: #define
PROFILER_TRACE_CATEGORY_ENABLED(cat, ret) \
Don't think this macro makes sense now. You can just directly use
TRACE_EVENT_CATEGORY_GROUP_ENABLED in those remaining two places.

[ssid, 2017-02-10 01:47:41]

Thanks

https://codereview.chromium.org/2676403002/diff/40001/src/profiler/tracing-cp...
File src/profiler/tracing-cpu-profiler.cc (right):

https://codereview.chromium.org/2676403002/diff/40001/src/profiler/tracing-cp...
src/profiler/tracing-cpu-profiler.cc:11: #define
PROFILER_TRACE_CATEGORY_ENABLED(cat, ret) \
On 2017/02/09 18:23:56, alph wrote:
> Don't think this macro makes sense now. You can just directly use
> TRACE_EVENT_CATEGORY_GROUP_ENABLED in those remaining two places.

Done.

[ssid, 2017-02-10 01:47:46]

The CQ bit was checked by ssid@chromium.org

[ssid, 2017-02-10 01:47:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2676403002/#ps60001
(title: "Remove macro")

[commit-bot: I haz the power, 2017-02-10 01:47:53]

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-10 02:09:49]

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

[commit-bot: I haz the power, 2017-02-10 02:09:50]

Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20738)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[ssid, 2017-02-10 02:21:54]

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

[commit-bot: I haz the power, 2017-02-10 02:22:01]

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, 2017-02-10 02:40:06]

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

[commit-bot: I haz the power, 2017-02-10 02:40:07]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/20740)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[ssid, 2017-02-10 18:59:29]

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

[commit-bot: I haz the power, 2017-02-10 18:59:33]

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, 2017-02-10 19:20:28]

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

[commit-bot: I haz the power, 2017-02-10 19:20:29]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-02-10 19:21:53]

The CQ bit was checked by ssid@chromium.org

[commit-bot: I haz the power, 2017-02-10 19:21:59]

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-10 19:23:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486754513308280,
"parent_rev": "91b79fb789b120c5e11185ef42cd1b88068de922", "commit_rev":
"21523c7832cdbd74fceacddf4660977eef02cd4c"}

[commit-bot: I haz the power, 2017-02-10 19:23:36]

Description was changed from

==========
[tracing] The CPU profiler should only be enabled for specific modes of tracing

We have different modes of tracing: recording, event callback and
filtering. The cpu profiler should not be enabled when tracing is
enabled with filtering mode.

BUG=688651
==========

to

==========
[tracing] The CPU profiler should only be enabled for specific modes of tracing

We have different modes of tracing: recording, event callback and
filtering. The cpu profiler should not be enabled when tracing is
enabled with filtering mode.

BUG=688651

Review-Url: https://codereview.chromium.org/2676403002
Cr-Commit-Position: refs/heads/master@{#43119}
Committed:
https://chromium.googlesource.com/v8/v8/+/21523c7832cdbd74fceacddf4660977eef0...
==========

[commit-bot: I haz the power, 2017-02-10 19:23:38]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/21523c7832cdbd74fceacddf4660977eef0...