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

src/profiler/tracing-cpu-profiler.cc

 // Copyright 2016 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/profiler/tracing-cpu-profiler.h"
 
 #include "src/profiler/cpu-profiler.h"
 #include "src/tracing/trace-event.h"
 #include "src/v8.h"
 
-#define PROFILER_TRACE_CATEGORY_ENABLED(cat) \
-  (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT(cat)))
+#define PROFILER_TRACE_CATEGORY_ENABLED(cat)            \
+  (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(         \
+      TRACE_DISABLED_BY_DEFAULT(cat)) &                \
+      (kEnabledForRecording_CategoryGroupEnabledFlags | \

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

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]

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]

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

[alph, 2017/02/07 22:04:31]

+1 for using TRACE_EVENT_CATEGORY_GROUP_ENABLED

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

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

+       kEnabledForEventCallback_CategoryGroupEnabledFlags))
 
 namespace v8 {
 
 std::unique_ptr<TracingCpuProfiler> TracingCpuProfiler::Create(
     v8::Isolate* isolate) {
   return std::unique_ptr<TracingCpuProfiler>(
       new internal::TracingCpuProfilerImpl(
           reinterpret_cast<internal::Isolate*>(isolate)));
 }
 
 namespace internal {
 
 TracingCpuProfilerImpl::TracingCpuProfilerImpl(Isolate* isolate)
     : isolate_(isolate), profiling_enabled_(false) {
   // Make sure tracing system notices profiler categories.
   PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profiler");

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

since you are here this should be
TRACE_EVENT_WARMUP_CATEGORY(TRACE_DISABLED_BY_DEFAULT("v8...."))

which is more readable

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

Yes done.

   PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profiler.hires");
   V8::GetCurrentPlatform()->AddTraceStateObserver(this);
 }
 
 TracingCpuProfilerImpl::~TracingCpuProfilerImpl() {
   StopProfiling();
   V8::GetCurrentPlatform()->RemoveTraceStateObserver(this);
 }
 
 void TracingCpuProfilerImpl::OnTraceEnabled() {
@@ skipping 30 matching lines @@
 
 void TracingCpuProfilerImpl::StopProfiling() {
   base::LockGuard<base::Mutex> lock(&mutex_);
   if (!profiler_) return;
   profiler_->StopProfiling("");
   profiler_.reset();
 }
 
 }  // namespace internal
 }  // namespace v8