Bug 1347274 (part 3, attempt 2) - Make some hot profiler functions lockless. r=mstange.
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 01 Jun 2017 13:33:22 +1000
changeset 361977 90f080141b3d55ec2dde80ec2b03314f04838180
parent 361976 9f6c73141311dd0c4d5ad140ab056d77ef2956ab
child 361978 b0e196c8fcd770b9ce8be81ff0fbcffabc7b8808
push id31953
push usercbook@mozilla.com
push dateFri, 02 Jun 2017 12:22:33 +0000
treeherdermozilla-central@2a8478029a0c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmstange
bugs1347274
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1347274 (part 3, attempt 2) - Make some hot profiler functions lockless. r=mstange.
tools/profiler/core/platform.cpp
tools/profiler/public/GeckoProfiler.h
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -489,16 +489,62 @@ uint32_t ActivePS::sNextGeneration = 0;
 
 #undef PS_GET
 #undef PS_GET_LOCKLESS
 #undef PS_GET_AND_SET
 
 // The mutex that guards accesses to CorePS and ActivePS.
 static PSMutex gPSMutex;
 
+// The preferred way to check profiler activeness and features is via
+// ActivePS(). However, that requires locking gPSMutex. There are some hot
+// operations where absolute precision isn't required, so we duplicate the
+// activeness/feature state in a lock-free manner in this class.
+class RacyFeatures
+{
+public:
+  static void SetActive(uint32_t aFeatures)
+  {
+    sActiveAndFeatures = Active | aFeatures;
+  }
+
+  static void SetInactive() { sActiveAndFeatures = 0; }
+
+  static bool IsActive() { return uint32_t(sActiveAndFeatures) & Active; }
+
+  static bool IsActiveWithFeature(uint32_t aFeature)
+  {
+    uint32_t af = sActiveAndFeatures;  // copy it first
+    return (af & Active) && (af & aFeature);
+  }
+
+  static bool IsActiveWithoutPrivacy()
+  {
+    uint32_t af = sActiveAndFeatures;  // copy it first
+    return (af & Active) && !(af & ProfilerFeature::Privacy);
+  }
+
+private:
+  static const uint32_t Active = 1u << 31;
+
+  // Ensure Active doesn't overlap with any of the feature bits.
+  #define NO_OVERLAP(n_, str_, Name_) \
+    static_assert(ProfilerFeature::Name_ != Active, "bad Active value");
+
+  PROFILER_FOR_EACH_FEATURE(NO_OVERLAP);
+
+  #undef NO_OVERLAP
+
+  // We combine the active bit with the feature bits so they can be read or
+  // written in a single atomic operation.
+  static Atomic<uint32_t> sActiveAndFeatures;
+};
+
+Atomic<uint32_t> RacyFeatures::sActiveAndFeatures(0);
+
 // Each live thread has a ThreadInfo, and we store a reference to it in TLS.
 // This class encapsulates that TLS.
 class TLSInfo
 {
 public:
   static bool Init(PSLockRef)
   {
     bool ok1 = sThreadInfo.init();
@@ -2405,16 +2451,19 @@ locked_profiler_start(PSLockRef aLock, i
     int javaInterval = interval;
     // Java sampling doesn't accurately keep up with 1ms sampling.
     if (javaInterval < 10) {
       javaInterval = 10;
     }
     mozilla::java::GeckoJavaSampler::Start(javaInterval, 1000);
   }
 #endif
+
+  // At the very end, set up RacyFeatures.
+  RacyFeatures::SetActive(ActivePS::Features(aLock));
 }
 
 void
 profiler_start(int aEntries, double aInterval, uint32_t aFeatures,
                const char** aFilters, uint32_t aFilterCount)
 {
   LOG("profiler_start");
 
@@ -2450,16 +2499,19 @@ profiler_start(int aEntries, double aInt
 
 static MOZ_MUST_USE SamplerThread*
 locked_profiler_stop(PSLockRef aLock)
 {
   LOG("locked_profiler_stop");
 
   MOZ_RELEASE_ASSERT(CorePS::Exists() && ActivePS::Exists(aLock));
 
+  // At the very start, clear RacyFeatures.
+  RacyFeatures::SetInactive();
+
 #ifdef MOZ_TASK_TRACER
   if (ActivePS::FeatureTaskTracer(aLock)) {
     mozilla::tasktracer::StopLogging();
   }
 #endif
 
   // Stop sampling live threads.
   Thread::tid_t tid = Thread::GetCurrentId();
@@ -2593,35 +2645,29 @@ profiler_resume()
 
 bool
 profiler_feature_active(uint32_t aFeature)
 {
   // This function runs both on and off the main thread.
 
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  PSAutoLock lock(gPSMutex);
-
-  if (!ActivePS::Exists(lock)) {
-    return false;
-  }
-
-  return !!(ActivePS::Features(lock) & aFeature);
+  // This function is hot enough that we use RacyFeatures, not ActivePS.
+  return RacyFeatures::IsActiveWithFeature(aFeature);
 }
 
 bool
 profiler_is_active()
 {
   // This function runs both on and off the main thread.
 
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  PSAutoLock lock(gPSMutex);
-
-  return ActivePS::Exists(lock);
+  // This function is hot enough that we use RacyFeatures, notActivePS.
+  return RacyFeatures::IsActive();
 }
 
 void
 profiler_register_thread(const char* aName, void* aGuessStackTop)
 {
   DEBUG_LOG("profiler_register_thread(%s)", aName);
 
   MOZ_RELEASE_ASSERT(!NS_IsMainThread());
@@ -2846,26 +2892,31 @@ profiler_get_backtrace_noalloc(char *out
       output += labelLength;
     }
     *output++ = '\0';
     *output = '\0';
   }
 }
 
 static void
-locked_profiler_add_marker(PSLockRef aLock, const char* aMarkerName,
-                           ProfilerMarkerPayload* aPayload)
+racy_profiler_add_marker(const char* aMarkerName,
+                         ProfilerMarkerPayload* aPayload)
 {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
-  MOZ_RELEASE_ASSERT(ActivePS::Exists(aLock) &&
-                     !ActivePS::FeaturePrivacy(aLock));
 
   // aPayload must be freed if we return early.
   mozilla::UniquePtr<ProfilerMarkerPayload> payload(aPayload);
 
+  // We don't assert that RacyFeatures::IsActiveWithoutPrivacy() is true here,
+  // because it's possible that the result has changed since we tested it in
+  // the caller.
+  //
+  // Because of this imprecision it's possible to miss a marker or record one
+  // we shouldn't. Either way is not a big deal.
+
   RacyThreadInfo* racyInfo = TLSInfo::RacyInfo();
   if (!racyInfo) {
     return;
   }
 
   mozilla::TimeStamp origin = (payload && !payload->GetStartTime().IsNull())
                             ? payload->GetStartTime()
                             : mozilla::TimeStamp::Now();
@@ -2874,59 +2925,56 @@ locked_profiler_add_marker(PSLockRef aLo
                              delta.ToMilliseconds());
 }
 
 void
 profiler_add_marker(const char* aMarkerName, ProfilerMarkerPayload* aPayload)
 {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  PSAutoLock lock(gPSMutex);
-
   // aPayload must be freed if we return early.
   mozilla::UniquePtr<ProfilerMarkerPayload> payload(aPayload);
 
-  if (!ActivePS::Exists(lock) || ActivePS::FeaturePrivacy(lock)) {
+  // This function is hot enough that we use RacyFeatures, notActivePS.
+  if (!RacyFeatures::IsActiveWithoutPrivacy()) {
     return;
   }
 
-  locked_profiler_add_marker(lock, aMarkerName, payload.release());
+  racy_profiler_add_marker(aMarkerName, payload.release());
 }
 
 void
 profiler_tracing(const char* aCategory, const char* aMarkerName,
                  TracingKind aKind)
 {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  PSAutoLock lock(gPSMutex);
-
-  if (!ActivePS::Exists(lock) || ActivePS::FeaturePrivacy(lock)) {
+  // This function is hot enough that we use RacyFeatures, notActivePS.
+  if (!RacyFeatures::IsActiveWithoutPrivacy()) {
     return;
   }
 
   auto payload = new ProfilerMarkerTracing(aCategory, aKind);
-  locked_profiler_add_marker(lock, aMarkerName, payload);
+  racy_profiler_add_marker(aMarkerName, payload);
 }
 
 void
 profiler_tracing(const char* aCategory, const char* aMarkerName,
                  UniqueProfilerBacktrace aCause, TracingKind aKind)
 {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
-  PSAutoLock lock(gPSMutex);
-
-  if (!ActivePS::Exists(lock) || ActivePS::FeaturePrivacy(lock)) {
+  // This function is hot enough that we use RacyFeatures, notActivePS.
+  if (!RacyFeatures::IsActiveWithoutPrivacy()) {
     return;
   }
 
   auto payload =
     new ProfilerMarkerTracing(aCategory, aKind, mozilla::Move(aCause));
-  locked_profiler_add_marker(lock, aMarkerName, payload);
+  racy_profiler_add_marker(aMarkerName, payload);
 }
 
 void
 profiler_log(const char* aStr)
 {
   profiler_tracing("log", aStr);
 }
 
--- a/tools/profiler/public/GeckoProfiler.h
+++ b/tools/profiler/public/GeckoProfiler.h
@@ -285,17 +285,19 @@ ProfilerBacktraceDestructor::operator()(
 // us calling CreateExpensiveData() unnecessarily in most cases, but the
 // expensive data will end up being created but not used if another thread
 // stops the profiler between the CreateExpensiveData() and PROFILER_OPERATION
 // calls.
 //
 PROFILER_FUNC(bool profiler_is_active(), false)
 
 // Check if a profiler feature (specified via the ProfilerFeature type) is
-// active. Returns false if the profiler is inactive.
+// active. Returns false if the profiler is inactive. Note: the return value
+// can become immediately out-of-date, much like the return value of
+// profiler_is_active().
 PROFILER_FUNC(bool profiler_feature_active(uint32_t aFeature), false)
 
 // Get the profile encoded as a JSON string. A no-op (returning nullptr) if the
 // profiler is inactive.
 PROFILER_FUNC(mozilla::UniquePtr<char[]> profiler_get_profile(double aSinceTime = 0),
               nullptr)
 
 // Write the profile for this process (excluding subprocesses) into aWriter.