From cfee6598e084821f305210c34ade66843a67a3df Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Tue, 1 Aug 2017 15:32:18 -0400 Subject: [PATCH] Bug 1382928 - Use atomics for thread-shared pieces of state in ThreadResponsiveness.cpp. r=njn MozReview-Commit-ID: LZJ4XHZPi7N --HG-- extra : rebase_source : 0b66d03165571c39948076961c69c40bb35a37af --- tools/profiler/core/platform.cpp | 5 +- tools/profiler/gecko/ThreadResponsiveness.cpp | 80 ++++++++++--------- tools/profiler/gecko/ThreadResponsiveness.h | 13 ++- 3 files changed, 53 insertions(+), 45 deletions(-) diff --git a/tools/profiler/core/platform.cpp b/tools/profiler/core/platform.cpp index c42a2d06b3b9..f8e6ea6949ca 100644 --- a/tools/profiler/core/platform.cpp +++ b/tools/profiler/core/platform.cpp @@ -1332,8 +1332,9 @@ DoPeriodicSample(PSLockRef aLock, ThreadInfo& aThreadInfo, 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) { diff --git a/tools/profiler/gecko/ThreadResponsiveness.cpp b/tools/profiler/gecko/ThreadResponsiveness.cpp index 60ce1718b98b..7a1a16337f7b 100644 --- a/tools/profiler/gecko/ThreadResponsiveness.cpp +++ b/tools/profiler/gecko/ThreadResponsiveness.cpp @@ -4,30 +4,23 @@ * 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) - , mHasEverBeenSuccessfullyDispatched(false) + : Runnable("CheckResponsivenessTask") + , mStartToPrevTracer_us(uint64_t(profiler_time() * 1000.0)) , mStop(false) + , mHasEverBeenSuccessfullyDispatched(false) { } @@ -38,7 +31,7 @@ protected: 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() @@ -59,25 +52,20 @@ public: // 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, @@ -85,23 +73,37 @@ public: 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 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 is not available. + Atomic 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 mStop; + + // Only accessed on the update thread. + bool mHasEverBeenSuccessfullyDispatched; }; NS_IMPL_ISUPPORTS_INHERITED(CheckResponsivenessTask, mozilla::Runnable, @@ -123,6 +125,6 @@ void ThreadResponsiveness::Update() { mActiveTracerEvent->DoFirstDispatchIfNeeded(); - mLastTracerTime = mActiveTracerEvent->GetLastTracerTime(); + mStartToPrevTracer_ms = Some(mActiveTracerEvent->GetStartToPrevTracer_ms()); } diff --git a/tools/profiler/gecko/ThreadResponsiveness.h b/tools/profiler/gecko/ThreadResponsiveness.h index 86d06a183972..c0dfcd38335b 100644 --- a/tools/profiler/gecko/ThreadResponsiveness.h +++ b/tools/profiler/gecko/ThreadResponsiveness.h @@ -7,6 +7,7 @@ #define ThreadResponsiveness_h #include "nsISupports.h" +#include "mozilla/Maybe.h" #include "mozilla/RefPtr.h" #include "mozilla/TimeStamp.h" @@ -21,16 +22,20 @@ public: 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 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 mStartToPrevTracer_ms; }; #endif