[tracing] Add filtering mode in TraceLog

base/trace_event/trace_log.cc

Index: base/trace_event/trace_log.cc
diff --git a/base/trace_event/trace_log.cc b/base/trace_event/trace_log.cc
index 866111a4d29620abac1835c6ddbb2ec43ce11070..3a6d2fab10c49e6ba2e2068823882b4f7c24f03a 100644
--- a/base/trace_event/trace_log.cc
+++ b/base/trace_event/trace_log.cc
@@ -465,7 +465,7 @@ TraceLog* TraceLog::GetInstance() {
 }
 
 TraceLog::TraceLog()
-    : mode_(DISABLED),
+    : enabled_modes_(0),
       num_traces_recorded_(0),
       event_callback_(0),
       dispatching_to_observer_list_(false),
@@ -564,7 +564,7 @@ const char* TraceLog::GetCategoryGroupName(
 void TraceLog::UpdateCategoryGroupEnabledFlag(size_t category_index) {
   unsigned char enabled_flag = 0;
   const char* category_group = g_category_groups[category_index];
-  if (mode_ == RECORDING_MODE &&
+  if (enabled_modes_ & RECORDING_MODE &&
       trace_config_.IsCategoryGroupEnabled(category_group)) {
     enabled_flag |= ENABLED_FOR_RECORDING;
   }
@@ -584,12 +584,12 @@ void TraceLog::UpdateCategoryGroupEnabledFlag(size_t category_index) {
   // TODO(primiano): this is a temporary workaround for catapult:#2341,
   // to guarantee that metadata events are always added even if the category
   // filter is "-*". See crbug.com/618054 for more details and long-term fix.
-  if (mode_ == RECORDING_MODE && !strcmp(category_group, "__metadata"))
+  if (enabled_modes_ & RECORDING_MODE && !strcmp(category_group, "__metadata"))
     enabled_flag |= ENABLED_FOR_RECORDING;
 
   uint32_t enabled_filters_bitmap = 0;
   int index = 0;
-  for (const auto& event_filter : trace_config_.event_filters()) {
+  for (const auto& event_filter : event_filters_enabled_) {
     if (event_filter.IsCategoryGroupEnabled(category_group)) {
       enabled_flag |= ENABLED_FOR_FILTERING;
       DCHECK(g_category_group_filters.Get()[index]);
@@ -618,7 +618,7 @@ void TraceLog::CreateFiltersForTraceConfig() {
   if (g_category_group_filters.Get().size())
     return;
 
-  for (auto& event_filter : trace_config_.event_filters()) {
+  for (auto& event_filter : event_filters_enabled_) {
     if (g_category_group_filters.Get().size() >= MAX_TRACE_EVENT_FILTERS) {
       NOTREACHED()
           << "Too many trace event filters installed in the current session";
@@ -731,7 +731,7 @@ void TraceLog::GetKnownCategoryGroups(
     category_groups->push_back(g_category_groups[i]);
 }
 
-void TraceLog::SetEnabled(const TraceConfig& trace_config, Mode mode) {
+void TraceLog::SetEnabled(const TraceConfig& trace_config, Mode new_mode) {

[Primiano Tucci (use gerrit), 2016/09/23 18:34:32]

add DCHECK to |new_mode| being exclusive and not or-ed (See other comment, sorry
for commenting out of order)

[ssid, 2016/09/26 18:39:53]

Do I need this dcheck even when passing an enum argument?

   std::vector<EnabledStateObserver*> observer_list;
   std::map<AsyncEnabledStateObserver*, RegisteredAsyncObserver> observer_map;
   {
@@ -745,32 +745,54 @@ void TraceLog::SetEnabled(const TraceConfig& trace_config, Mode mode) {
 
     InternalTraceOptions old_options = trace_options();
 
-    if (IsEnabled()) {
-      if (new_options != old_options) {
-        DLOG(ERROR) << "Attempting to re-enable tracing with a different "
-                    << "set of options.";
-      }
-
-      if (mode != mode_) {
-        DLOG(ERROR) << "Attempting to re-enable tracing with a different mode.";
-      }
-
-      DCHECK(!trace_config.event_filters().size())
-          << "Adding new filters while tracing was already enabled is not "
-             "supported.";
-
-      trace_config_.Merge(trace_config);
-      UpdateCategoryGroupEnabledFlags();
+    if (dispatching_to_observer_list_) {
+      NOTREACHED()
+          << "Cannot manipulate TraceLog::Enabled state from an observer.";
       return;
     }
 
-    if (dispatching_to_observer_list_) {
-      DLOG(ERROR)
-          << "Cannot manipulate TraceLog::Enabled state from an observer.";
+    // Update event filters:
+    DCHECK(
+        !(new_mode == FILTERING_MODE && trace_config.event_filters().empty()))
+        << "Attempting to enable filtering without any filters";
+    if (!enabled_modes_) {
+      // Clear all filters from previous tracing session. These filters are not
+      // cleared at the end of tracing because some threads which hit trace
+      // event when disabling, could try to use the filters.
+      g_category_group_filters.Get().clear();
+    }
+    if (event_filters_enabled_.empty()) {
+      // Use the given event filters only if filtering was not enabled.
+      event_filters_enabled_ = trace_config.event_filters();
+    } else if (new_mode == FILTERING_MODE) {
+      NOTREACHED() << "Attempting to re-enable filtering when filters are "
+                      "already enabled.";
       return;
     }

[Primiano Tucci (use gerrit), 2016/09/23 18:34:32]

what happens if tracing is already enabled for FILTERING_MODE and then you
enable for TRACING passing extra filters?

[ssid, 2016/09/26 18:39:53]

I did not want to cause race conditions on the std::vector by adding more
filters to the list. Ideally I could use an array and add more elements to the
array of filters, but then I would also have to keep track of which filters were
enabled by recording_mode and which filters were enabled by filtering_mode, so
that after recording_mode i will keep the filtering_mode filters enabled. To
avoid this complication i do not support adding / removing filters when
filtering is enabled by any mode. So, if filters were added by recording_mode
when filtering_mode was already enabled, then we ignore those filters and use
the existing filters. 

 
-    mode_ = mode;
+    // Update trace config for recording.
+    bool already_recording = enabled_modes_ & RECORDING_MODE;
+    if (new_mode == RECORDING_MODE) {
+      if (already_recording) {
+        DCHECK_EQ(new_options, old_options) << "Attempting to re-enable "
+                                               "tracing with a different set "
+                                               "of options.";
+        trace_config_.Merge(trace_config);
+      } else {
+        trace_config_ = trace_config;
+      }
+    }
+    // Keep the |trace_config_| updated with only enabled filters in case anyone
+    // tries to read it using |GetCurrentTraceConfig|
+    trace_config_.SetEventFilterConfigs(event_filters_enabled_);
+
+    enabled_modes_ |= new_mode;
+    UpdateCategoryGroupEnabledFlags();
+
+    // Do not notify observers and create trace buffer if enabled for filtering
+    // or if recording was already enabled.
+    if (new_mode == FILTERING_MODE || already_recording)
+      return;
 
     if (new_options != old_options) {
       subtle::NoBarrier_Store(&trace_options_, new_options);
@@ -779,12 +801,6 @@ void TraceLog::SetEnabled(const TraceConfig& trace_config, Mode mode) {
 
     num_traces_recorded_++;
 
-    // Clear all filters from previous tracing session. These filters are not
-    // cleared at the end of tracing because some threads which hit trace event
-    // when disabling, could try to use the filters.
-    g_category_group_filters.Get().clear();
-
-    trace_config_ = TraceConfig(trace_config);
     UpdateCategoryGroupEnabledFlags();
     UpdateSyntheticDelaysFromTraceConfig();
 
@@ -859,23 +875,26 @@ TraceConfig TraceLog::GetCurrentTraceConfig() const {
 
 void TraceLog::SetDisabled() {
   AutoLock lock(lock_);
-  SetDisabledWhileLocked();
+  SetDisabledWhileLocked(RECORDING_MODE);
+}
+
+void TraceLog::SetDisabled(Mode mode) {
+  AutoLock lock(lock_);
+  SetDisabledWhileLocked(mode);
 }
 
-void TraceLog::SetDisabledWhileLocked() {
+void TraceLog::SetDisabledWhileLocked(Mode mode) {

[Primiano Tucci (use gerrit), 2016/09/23 18:34:32]

can you SetDisabled(RECORDING | FILTERING) ?
If not add a DCHECK(mode & (mode - 1); ( mode being a pow of two)  here and in
SetEnabled.

[ssid, 2016/09/26 18:39:53]

Um, the argument says "Mode" and not uint8_t. I think it is clear that the user
should pass only only mode right?

   lock_.AssertAcquired();
 
-  if (!IsEnabled())
+  if (!(enabled_modes_ & mode))
     return;
 
   if (dispatching_to_observer_list_) {
-    DLOG(ERROR)
+    NOTREACHED()
         << "Cannot manipulate TraceLog::Enabled state from an observer.";
     return;
   }
 
-  mode_ = DISABLED;
-
   if (sampling_thread_) {
     // Stop the sampling thread.

[Primiano Tucci (use gerrit), 2016/09/23 18:34:32]

should all this stuff happen even if you are just disabling FILTERING mode? 
I think this is relevant only for recording, right?

[ssid, 2016/09/26 18:39:53]

Done.

     sampling_thread_->Stop();
@@ -886,10 +905,21 @@ void TraceLog::SetDisabledWhileLocked() {
     sampling_thread_.reset();
   }
 
-  trace_config_.Clear();
+  enabled_modes_ &= ~mode;

[Primiano Tucci (use gerrit), 2016/09/23 18:34:31]

is there a reason why you moved this statement before the if {} block above? I
wonder if any of those dtors make any assumptions on the mode.

[ssid, 2016/09/26 18:39:53]

Done.

+  if (mode == RECORDING_MODE)

[Primiano Tucci (use gerrit), 2016/09/23 18:34:32]

should this be s/==/&/ ?

[ssid, 2016/09/26 18:39:53]

mode is enum value.

+    trace_config_.Clear();
+  // Clear the filters if tracing is no longer enabled or filtering is disabled.
+  if (!mode || mode == FILTERING_MODE)

[Primiano Tucci (use gerrit), 2016/09/23 18:34:32]

In which case mode is expected to be == 0? I though you always pass somethign
here.

[ssid, 2016/09/26 18:39:53]

Sorry, mixed mode and mode_ here when i renamed mode_ to enabled_modes_.

+    event_filters_enabled_.clear();
+
   subtle::NoBarrier_Store(&watch_category_, 0);
   watch_event_name_.clear();
   UpdateCategoryGroupEnabledFlags();
+
+  // Skip adding metadata events and notifying observers for filtering mode.
+  if (mode == FILTERING_MODE)
+    return;
+
   AddMetadataEventsWhileLocked();
 
   // Remove metadata events so they will not get added to a subsequent trace.
@@ -916,6 +946,11 @@ void TraceLog::SetDisabledWhileLocked() {
   dispatching_to_observer_list_ = false;
 }
 
+bool TraceLog::HaveActiveFilters() {
+  AutoLock lock(lock_);
+  return !event_filters_enabled_.empty();
+}
+
 int TraceLog::GetNumTracesRecorded() {
   AutoLock lock(lock_);
   if (!IsEnabled())
@@ -989,7 +1024,7 @@ void TraceLog::CheckIfBufferIsFullWhileLocked() {
     if (buffer_limit_reached_timestamp_.is_null()) {
       buffer_limit_reached_timestamp_ = OffsetNow();
     }
-    SetDisabledWhileLocked();
+    SetDisabledWhileLocked(RECORDING_MODE);
   }
 }
 
@@ -1376,11 +1411,6 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
   TimeTicks offset_event_timestamp = OffsetTimestamp(timestamp);
   ThreadTicks thread_now = ThreadNow();
 
-  // |thread_local_event_buffer_| can be null if the current thread doesn't have
-  // a message loop or the message loop is blocked.
-  InitializeThreadLocalEventBufferIfSupported();
-  auto* thread_local_event_buffer = thread_local_event_buffer_.Get();
-
   // Check and update the current thread name only if the event is for the
   // current thread to avoid locks in most cases.
   if (thread_id == static_cast<int>(PlatformThread::CurrentId())) {
@@ -1452,6 +1482,11 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
   // filters indicates or category is not enabled for filtering.
   if ((*category_group_enabled & ENABLED_FOR_RECORDING) &&
       !disabled_by_filters) {
+    // |thread_local_event_buffer_| can be null if the current thread doesn't
+    // have a message loop or the message loop is blocked.
+    InitializeThreadLocalEventBufferIfSupported();
+    auto* thread_local_event_buffer = thread_local_event_buffer_.Get();
+
     OptionalAutoLock lock(&lock_);
 
     TraceEvent* trace_event = NULL;