[ETW Export] Add polling for ETW keyword.

base/trace_event/trace_event_etw_export_win.cc

 // Copyright 2015 The Chromium 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 "base/trace_event/trace_event_etw_export_win.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #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"
 
 // The GetProcAddress technique is borrowed from
 // https://github.com/google/UIforETW/tree/master/ETWProviders
 //
 // EVNTAPI is used in evntprov.h which is included by chrome_events_win.h.
 // We define EVNTAPI without the DECLSPEC_IMPORT specifier so that we can
 // implement these functions locally instead of using the import library, and
 // can therefore still run on Windows XP.
@@ skipping 61 matching lines @@
     "toplevel",                                        // 0x200
     "v8",                                              // 0x400
     "disabled-by-default-cc.debug",                    // 0x800
     "disabled-by-default-cc.debug.picture",            // 0x1000
     "disabled-by-default-toplevel.flow"};              // 0x2000
 const char* other_events_group_name = "__OTHER_EVENTS";  // 0x2000000000000000
 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
 // chrome_events_win.h
 ULONG EVNTAPI EventRegister(LPCGUID ProviderId,
                             PENABLECALLBACK EnableCallback,
                             PVOID CallbackContext,
                             PREGHANDLE RegHandle) {
   if (EventRegisterProc)
     return EventRegisterProc(ProviderId, EnableCallback, CallbackContext,
@@ skipping 34 matching lines @@
         ::GetProcAddress(AdvapiDLL, "EventRegister"));
     EventWriteProc = reinterpret_cast<tEventWrite>(
         ::GetProcAddress(AdvapiDLL, "EventWrite"));
     EventUnregisterProc = reinterpret_cast<tEventUnregister>(
         ::GetProcAddress(AdvapiDLL, "EventUnregister"));
 
     // Register the ETW provider. If registration fails then the event logging
     // calls will fail (on XP this call will do nothing).
     EventRegisterChrome();
 
     UpdateEnabledCategories();

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

I removed this from here and moved it to EnableETWExport. It makes more sense
that way.

   }
 }
 
 TraceEventETWExport::~TraceEventETWExport() {
   EventUnregisterChrome();
 }
 
 // static
 TraceEventETWExport* TraceEventETWExport::GetInstance() {
   return Singleton<TraceEventETWExport,
                    StaticMemorySingletonTraits<TraceEventETWExport>>::get();
 }
 
 // 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
 void TraceEventETWExport::AddEvent(
     char phase,
     const unsigned char* category_group_enabled,
     const char* name,
     unsigned long long id,
     int num_args,
     const char** arg_names,
     const unsigned char* arg_types,
     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;
 
   const char* phase_string = nullptr;
   // Space to store the phase identifier and null-terminator, when needed.
   char phase_buffer[2];
   switch (phase) {
     case TRACE_EVENT_PHASE_BEGIN:
       phase_string = "Begin";
       break;
@@ skipping 86 matching lines @@
 
 // static
 void TraceEventETWExport::AddCustomEvent(const char* name,
                                          char const* phase,
                                          const char* arg_name_1,
                                          const char* arg_value_1,
                                          const char* arg_name_2,
                                          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;
 
   EventWriteChromeEvent(name, phase, arg_name_1, arg_value_1, arg_name_2,
                         arg_value_2, arg_name_3, arg_value_3);
 }
 
 // static
 bool TraceEventETWExport::IsCategoryGroupEnabled(
     const char* category_group_name) {
   DCHECK(category_group_name);
-  auto instance = GetInstance();
+  auto* instance = GetInstance();
   if (instance == nullptr)
     return false;
 
   if (!instance->IsETWExportEnabled())
     return false;
 
   CStringTokenizer category_group_tokens(
       category_group_name, category_group_name + strlen(category_group_name),
       ",");
   while (category_group_tokens.GetNext()) {
@@ skipping 49 matching lines @@
     DCHECK(categories_status_.find(disabled_other_events_group_name) !=
            categories_status_.end());
     return categories_status_.find(disabled_other_events_group_name)->second;
   } else {
     DCHECK(categories_status_.find(other_events_group_name) !=
            categories_status_.end());
     return categories_status_.find(other_events_group_name)->second;
   }
 }
 
+// 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