[ETW Export] Add polling for ETW keyword.

base/trace_event/trace_event_etw_export_win.cc

Index: base/trace_event/trace_event_etw_export_win.cc
diff --git a/base/trace_event/trace_event_etw_export_win.cc b/base/trace_event/trace_event_etw_export_win.cc
index e61a7b29645992d0e160d3d25cd91e2bc5fb58f9..017a9eca93e5628687969f483172cc3b6ee40594 100644
--- a/base/trace_event/trace_event_etw_export_win.cc
+++ b/base/trace_event/trace_event_etw_export_win.cc
@@ -9,6 +9,7 @@
 #include "base/memory/singleton.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/threading/platform_thread.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_event_impl.h"
 
@@ -90,6 +91,29 @@ const char* disabled_other_events_group_name =
     "__DISABLED_OTHER_EVENTS";  // 0x4000000000000000
 uint64 other_events_keyword_bit = 1ULL << 61;
 uint64 disabled_other_events_keyword_bit = 1ULL << 62;
+
+// This object will be created by each process. It's a background (low-priority)
+// thread that will monitor the ETW keyword for any changes..

[dsinclair, 2015/08/11 19:33:14]

nit: double .'s at end.

[Georges Khalil, 2015/08/12 17:24:25]

Done.

+class ETWKeywordUpdateThread : public base::PlatformThread::Delegate {
+ public:
+  ETWKeywordUpdateThread() {}
+  ~ETWKeywordUpdateThread() override {}
+
+  // Implementation of PlatformThread::Delegate:
+  void ThreadMain() override {
+    base::PlatformThread::SetName("ETW Keyword Update Thread");
+    while (1) {
+      base::PlatformThread::Sleep(
+          base::TimeDelta::FromMilliseconds(kUpdateTimerDelayMs));

[dsinclair, 2015/08/11 19:33:14]

Why do the milliseconds conversion every iteration? Why not store it outside the
while loop?

[Georges Khalil, 2015/08/12 17:24:25]

Done.

+      base::trace_event::TraceEventETWExport::UpdateETWKeyword();
+    }
+  }
+
+ private:
+  // Time between checks for ETW keyword changes (in milliseconds).
+  unsigned int kUpdateTimerDelayMs = 1000;
+};
+
 }  // namespace
 
 // Redirector function for EventRegister. Called by macros in
@@ -160,19 +184,32 @@ TraceEventETWExport* TraceEventETWExport::GetInstance() {
 
 // static
 void TraceEventETWExport::EnableETWExport() {
-  if (GetInstance())
-    GetInstance()->etw_export_enabled_ = true;
+  auto* instance = GetInstance();
+  if (instance && !instance->etw_export_enabled_) {
+    instance->etw_export_enabled_ = true;
+    // We only create the update thread once and it lives for as long as Chrome
+    // is running.
+    if (instance->keyword_update_thread_handle_.is_null()) {
+      instance->keyword_update_thread_.reset(new ETWKeywordUpdateThread);
+      PlatformThread::CreateWithPriority(
+          0, instance ->keyword_update_thread_.get(),

[dsinclair, 2015/08/11 19:33:14]

nit: Remove space between instance and ->

[Georges Khalil, 2015/08/12 17:24:25]

Done.

+          &instance->keyword_update_thread_handle_,
+          ThreadPriority::BACKGROUND);
+    }
+  }
 }
 
 // static
 void TraceEventETWExport::DisableETWExport() {
-  if (GetInstance())
-    GetInstance()->etw_export_enabled_ = false;
+  auto* instance = GetInstance();
+  if (instance && instance->etw_export_enabled_)
+    instance->etw_export_enabled_ = false;
 }

[dsinclair, 2015/08/11 19:33:14]

Why don't we shut down the thread at the point we stop tracing? Seems wasteful
to leave it running and not doing anything?

[brucedawson, 2015/08/11 20:24:02]

Currently this function is never called - there is no mechanism to disable
tracing. What does happen is that the ETW keyword goes to zero (because nobody
is listening) so exporting of ETW events stops. At that point the only overhead
is the thread continuing to poll once a second, watching for the ETW keyword to
change to non-zero again.

There are really three states: disabled (etw_export_enable == false), enabled
but inactive (etw_export_enable == true, etw keyword == 0), and active
(etw_export_enable == true, etw keyword != 0).

[Georges Khalil, 2015/08/12 17:24:25]

Exactly. This might change down the road when we can enable it dynamically.

Should I just remove this function for now and we add it back when needed?

[dsinclair, 2015/08/13 19:03:17]

If it's unused, we should probably use it as it's confusing to see it and not
have it used.

 
 // static
 bool TraceEventETWExport::IsETWExportEnabled() {
-  return (GetInstance() && GetInstance()->etw_export_enabled_);
+  auto* instance = GetInstance();
+  return (instance && instance->etw_export_enabled_);
 }
 
 // static
@@ -187,7 +224,8 @@ void TraceEventETWExport::AddEvent(
     const unsigned long long* arg_values,
     const scoped_refptr<ConvertableToTraceFormat>* convertable_values) {
   // We bail early in case exporting is disabled or no consumer is listening.
-  if (!GetInstance() || !GetInstance()->etw_export_enabled_ ||
+  auto* instance = GetInstance();
+  if (!instance || !instance->etw_export_enabled_ ||
       !EventEnabledChromeEvent())
     return;
 
@@ -294,7 +332,8 @@ void TraceEventETWExport::AddCustomEvent(const char* name,
                                          const char* arg_value_2,
                                          const char* arg_name_3,
                                          const char* arg_value_3) {
-  if (!GetInstance() || !GetInstance()->etw_export_enabled_ ||
+  auto* instance = GetInstance();
+  if (!instance || !instance->etw_export_enabled_ ||
       !EventEnabledChromeEvent())
     return;
 
@@ -306,7 +345,7 @@ void TraceEventETWExport::AddCustomEvent(const char* name,
 bool TraceEventETWExport::IsCategoryGroupEnabled(
     const char* category_group_name) {
   DCHECK(category_group_name);
-  auto instance = GetInstance();
+  auto* instance = GetInstance();
   if (instance == nullptr)
     return false;
 
@@ -376,5 +415,14 @@ bool TraceEventETWExport::IsCategoryEnabled(const char* category_name) const {
   }
 }
 
+// static
+void TraceEventETWExport::UpdateETWKeyword() {
+  auto* instance = GetInstance();
+  DCHECK(instance);
+  if (IsETWExportEnabled()) {

[dsinclair, 2015/08/11 19:33:14]

if (!IsETWExportEnabled())
  return;
if (!instance->UpdateEnabledCategories())
  return;

[Georges Khalil, 2015/08/12 17:24:25]

I changed this. Now UpdateEnabledCategories calls directly the TraceLog API.

+    if (instance->UpdateEnabledCategories())
+      TraceLog::GetInstance()->UpdateCategoryGroupEnabledFlags();
+  }
+}
 }  // namespace trace_event
 }  // namespace base