Bug 1382928 - Use atomics for thread-shared pieces of state in ThreadResponsiveness.cpp. r?njn draft
authorMarkus Stange <mstange@themasta.com>
Tue, 01 Aug 2017 15:32:18 -0400
changeset 619236 2b2af6993eda5b99e4ba30f490be61ff2007874c
parent 619235 d0fdbfbf5d07458fdd80d43908a5fb36e70b2bd9
child 640346 a6721456d75e057b9ef50ac44c46a44b53bb9fa2
push id71622
push userbmo:mstange@themasta.com
push dateTue, 01 Aug 2017 19:50:51 +0000
reviewersnjn
bugs1382928
milestone56.0a1
Bug 1382928 - Use atomics for thread-shared pieces of state in ThreadResponsiveness.cpp. r?njn MozReview-Commit-ID: LZJ4XHZPi7N
tools/profiler/core/platform.cpp
tools/profiler/gecko/ThreadResponsiveness.cpp
tools/profiler/gecko/ThreadResponsiveness.h
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -1283,18 +1283,19 @@ DoPeriodicSample(PSLockRef aLock, Thread
   while (pendingMarkersList && pendingMarkersList->peek()) {
     ProfilerMarker* marker = pendingMarkersList->popHead();
     buffer.AddStoredMarker(marker);
     buffer.AddEntry(ProfileBufferEntry::Marker(marker));
   }
 
   ThreadResponsiveness* resp = aThreadInfo.GetThreadResponsiveness();
   if (resp && resp->HasData()) {
-    TimeDuration delta = resp->GetUnresponsiveDuration(aNow);
-    buffer.AddEntry(ProfileBufferEntry::Responsiveness(delta.ToMilliseconds()));
+    double delta = resp->GetUnresponsiveDuration(
+      (aNow - CorePS::ProcessStartTime()).ToMilliseconds());
+    buffer.AddEntry(ProfileBufferEntry::Responsiveness(delta));
   }
 
   if (aRSSMemory != 0) {
     double rssMemory = static_cast<double>(aRSSMemory);
     buffer.AddEntry(ProfileBufferEntry::ResidentMemory(rssMemory));
   }
 
   if (aUSSMemory != 0) {
--- a/tools/profiler/gecko/ThreadResponsiveness.cpp
+++ b/tools/profiler/gecko/ThreadResponsiveness.cpp
@@ -1,49 +1,42 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "ThreadResponsiveness.h"
-#include "platform.h"
-#include "nsComponentManagerUtils.h"
-#include "nsThreadUtils.h"
-#include "nsITimer.h"
-#include "mozilla/Mutex.h"
-#include "mozilla/RefPtr.h"
+
+#include "mozilla/Atomics.h"
 #include "mozilla/SystemGroup.h"
 
-using mozilla::Mutex;
-using mozilla::MutexAutoLock;
-using mozilla::SystemGroup;
-using mozilla::TaskCategory;
-using mozilla::TimeStamp;
+#include "nsITimer.h"
+#include "platform.h"
 
-class CheckResponsivenessTask : public mozilla::Runnable,
+using namespace mozilla;
+
+class CheckResponsivenessTask : public Runnable,
                                 public nsITimerCallback {
 public:
   CheckResponsivenessTask()
-    : mozilla::Runnable("CheckResponsivenessTask")
-    , mLastTracerTime(TimeStamp::Now())
-    , mMutex("CheckResponsivenessTask")
-    , mTimer(nullptr)
+    : Runnable("CheckResponsivenessTask")
+    , mStartToPrevTracer_us(uint64_t(profiler_time() * 1000.0))
+    , mStop(false)
     , mHasEverBeenSuccessfullyDispatched(false)
-    , mStop(false)
   {
   }
 
 protected:
   ~CheckResponsivenessTask()
   {
   }
 
 public:
 
-  // Must be called from the same thread every time. Call that the "update"
+  // Must be called from the same thread every time. Call that the update
   // thread, because it's the thread that ThreadResponsiveness::Update() is
   // called on. In reality it's the profiler's sampler thread.
   void DoFirstDispatchIfNeeded()
   {
     if (mHasEverBeenSuccessfullyDispatched) {
       return;
     }
 
@@ -54,59 +47,68 @@ public:
     if (NS_SUCCEEDED(rv)) {
       mHasEverBeenSuccessfullyDispatched = true;
     }
   }
 
   // Can only run on the main thread.
   NS_IMETHOD Run() override
   {
-    MutexAutoLock mon(mMutex);
-    if (mStop)
-      return NS_OK;
+    mStartToPrevTracer_us = uint64_t(profiler_time() * 1000.0);
 
-    // This is raced on because we might pause the thread here
-    // for profiling so if we tried to use a monitor to protect
-    // mLastTracerTime we could deadlock. We're risking seeing
-    // a partial write which will show up as an outlier in our
-    // performance data.
-    mLastTracerTime = TimeStamp::Now();
-    if (!mTimer) {
-      mTimer = do_CreateInstance("@mozilla.org/timer;1");
-      mTimer->SetTarget(SystemGroup::EventTargetFor(TaskCategory::Other));
+    if (!mStop) {
+      if (!mTimer) {
+        mTimer = do_CreateInstance("@mozilla.org/timer;1");
+        mTimer->SetTarget(SystemGroup::EventTargetFor(TaskCategory::Other));
+      }
+      mTimer->InitWithCallback(this, 16, nsITimer::TYPE_ONE_SHOT);
     }
-    mTimer->InitWithCallback(this, 16, nsITimer::TYPE_ONE_SHOT);
 
     return NS_OK;
   }
 
+  // Main thread only
   NS_IMETHOD Notify(nsITimer* aTimer) final
   {
     SystemGroup::Dispatch(TaskCategory::Other,
                           do_AddRef(this));
     return NS_OK;
   }
 
+  // Can be called on any thread.
   void Terminate() {
-    MutexAutoLock mon(mMutex);
     mStop = true;
   }
 
-  const TimeStamp& GetLastTracerTime() const {
-    return mLastTracerTime;
+  // Can be called on any thread.
+  double GetStartToPrevTracer_ms() const {
+    return mStartToPrevTracer_us / 1000.0;
   }
 
   NS_DECL_ISUPPORTS_INHERITED
 
 private:
-  TimeStamp mLastTracerTime;
-  Mutex mMutex;
+  // The timer that's responsible for redispatching this event to the main
+  // thread. This field is only accessed on the main thread.
   nsCOMPtr<nsITimer> mTimer;
-  bool mHasEverBeenSuccessfullyDispatched; // only accessed on the "update" thread
-  bool mStop;
+
+  // The time (in integer microseconds since process startup) at which this
+  // event was last processed (Run() was last called).
+  // This field is written on the main thread and read on the update thread.
+  // This is stored as integer microseconds instead of double milliseconds
+  // because Atomic<double> is not available.
+  Atomic<uint64_t> mStartToPrevTracer_us;
+
+  // Whether we should stop redispatching this event once the timer fires the
+  // next time. Set to true by any thread when the profiler is stopped; read on
+  // the main thread.
+  Atomic<bool> mStop;
+
+  // Only accessed on the update thread.
+  bool mHasEverBeenSuccessfullyDispatched;
 };
 
 NS_IMPL_ISUPPORTS_INHERITED(CheckResponsivenessTask, mozilla::Runnable,
                             nsITimerCallback)
 
 ThreadResponsiveness::ThreadResponsiveness()
   : mActiveTracerEvent(new CheckResponsivenessTask())
 {
@@ -118,11 +120,11 @@ ThreadResponsiveness::~ThreadResponsiven
   MOZ_COUNT_DTOR(ThreadResponsiveness);
   mActiveTracerEvent->Terminate();
 }
 
 void
 ThreadResponsiveness::Update()
 {
   mActiveTracerEvent->DoFirstDispatchIfNeeded();
-  mLastTracerTime = mActiveTracerEvent->GetLastTracerTime();
+  mStartToPrevTracer_ms = Some(mActiveTracerEvent->GetStartToPrevTracer_ms());
 }
 
--- a/tools/profiler/gecko/ThreadResponsiveness.h
+++ b/tools/profiler/gecko/ThreadResponsiveness.h
@@ -2,36 +2,41 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef ThreadResponsiveness_h
 #define ThreadResponsiveness_h
 
 #include "nsISupports.h"
+#include "mozilla/Maybe.h"
 #include "mozilla/RefPtr.h"
 #include "mozilla/TimeStamp.h"
 
 class CheckResponsivenessTask;
 
 // This class should only be used for the main thread.
 class ThreadResponsiveness {
 public:
   explicit ThreadResponsiveness();
 
   ~ThreadResponsiveness();
 
   void Update();
 
-  mozilla::TimeDuration GetUnresponsiveDuration(const mozilla::TimeStamp& now) const {
-    return now - mLastTracerTime;
+  // The number of milliseconds that elapsed since the last
+  // CheckResponsivenessTask was processed.
+  double GetUnresponsiveDuration(double aStartToNow_ms) const {
+    return aStartToNow_ms - *mStartToPrevTracer_ms;
   }
 
   bool HasData() const {
-    return !mLastTracerTime.IsNull();
+    return mStartToPrevTracer_ms.isSome();
   }
 private:
   RefPtr<CheckResponsivenessTask> mActiveTracerEvent;
-  mozilla::TimeStamp mLastTracerTime;
+  // The time at which the last CheckResponsivenessTask was processed, in
+  // milliseconds since process start (i.e. what you get from profiler_time()).
+  mozilla::Maybe<double> mStartToPrevTracer_ms;
 };
 
 #endif