Access FileTracing::g_provider atomically to prevent data races

base/files/file_tracing.cc

Index: base/files/file_tracing.cc
diff --git a/base/files/file_tracing.cc b/base/files/file_tracing.cc
index 6d11cbc746ed268a357e0d2ba08d463c501e5d96..d5ce12c7901bfb27c5b37bb64931f011b091743e 100644
--- a/base/files/file_tracing.cc
+++ b/base/files/file_tracing.cc
@@ -4,39 +4,54 @@
 
 #include "base/files/file_tracing.h"
 
+#include "base/atomicops.h"
 #include "base/files/file.h"
 
+using base::subtle::AtomicWord;
+
 namespace base {
 
 namespace {
-FileTracing::Provider* g_provider = nullptr;
+AtomicWord g_provider = reinterpret_cast<AtomicWord>(nullptr);
+}
+
+FileTracing::Provider* GetProvider() {
+  AtomicWord provider = base::subtle::NoBarrier_Load(&g_provider);

[Alexander Potapenko, 2016/07/09 10:32:04]

You'll need Acquire_Load and Release_Store for getter and setter, because the
poiner has associated data.

[groby-ooo-7-16, 2016/07/13 20:05:04]

In a nutshell: Follow Alexander's suggestion if you want to avoid pain.
release_store/acquire_load is a well-understood pattern for setting globals, and
it works.

If you're still concerned about speed, I _think_ you can do better, but somebody
else should double-check my reasoning. (subtle is subtle for a reason. Stick
with Release_Store/Acquire_Load unless you *really* need those cycles)

Reasoning just about the logic:
* |g_provider| is only set once
* FileTracing::ProviderImpl is stateless, so there are no dependent read/writes
* out-of-order reads of |g_provider| are not critical. (i.e. Provider, nullptr,
Provider needs to be fine - the ScopedEnabled might be a problem here, because
Enable/Disable might get mismatched. Can any tracing ops happen before
SetProvider?)

That would mean you really don't care about barriers. Except, in the real world,
ProviderImpl carries around a vtbl pointer which is dependent data. It's
read-only, though.

Given all these, writing still needs Release_Store - you want to ensure the vtbl
isn't set after |g_provider| is set, that'd be embarrassing :)

Leaves reading. Acquire_Load prevents reordering with later reads or writes.
Later writes to the vtbl can't happen after it's set, and later writes to
|g_provider| (on a separate thread) don't matter because we'll always get
nullptr or a valid provider - and it's not a fatal flaw if you first get a
provider, then nullptr, then a provider. (AFAICT). Later dependent reads are
only the vtbl, and the set operation guaranteed that's set when |g_provider| is
set.

The cost of a memory barrier is (wild guess) on average some 100s of CPU cycles,
so this has to be on a *really* hot path to be worth it. 

+  return reinterpret_cast<FileTracing::Provider*>(provider);
 }
 
 // static
 bool FileTracing::IsCategoryEnabled() {
-  return g_provider && g_provider->FileTracingCategoryIsEnabled();
+  FileTracing::Provider* provider = GetProvider();
+  return provider && provider->FileTracingCategoryIsEnabled();
 }
 
 // static
 void FileTracing::SetProvider(FileTracing::Provider* provider) {
-  g_provider = provider;
+  base::subtle::NoBarrier_Store(
+      &g_provider, reinterpret_cast<AtomicWord>(provider));
 }
 
 FileTracing::ScopedEnabler::ScopedEnabler() {
-  if (g_provider)
-    g_provider->FileTracingEnable(this);
+  FileTracing::Provider* provider = GetProvider();
+  if (provider)
+    provider->FileTracingEnable(this);
 }
 
 FileTracing::ScopedEnabler::~ScopedEnabler() {
-  if (g_provider)
-    g_provider->FileTracingDisable(this);
+  FileTracing::Provider* provider = GetProvider();
+  if (provider)
+    provider->FileTracingDisable(this);
 }
 
 FileTracing::ScopedTrace::ScopedTrace() : id_(nullptr) {}
 
 FileTracing::ScopedTrace::~ScopedTrace() {
-  if (id_ && g_provider)
-    g_provider->FileTracingEventEnd(name_, id_);
+  if (id_) {
+    FileTracing::Provider* provider = GetProvider();
+    if (provider)
+      provider->FileTracingEventEnd(name_, id_);
+  }
 }
 
 void FileTracing::ScopedTrace::Initialize(const char* name,
@@ -44,7 +59,7 @@ void FileTracing::ScopedTrace::Initialize(const char* name,
                                           int64_t size) {
   id_ = &file->trace_enabler_;
   name_ = name;
-  g_provider->FileTracingEventBegin(name_, id_, file->tracing_path_, size);
+  GetProvider()->FileTracingEventBegin(name_, id_, file->tracing_path_, size);
 }
 
 }  // namespace base