Bug 1458839 - Improve state assertions in GCParallelTask r=jandem
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 03 May 2018 17:51:58 +0100
changeset 472960 a35392007e5d22f3debdd7f53f65405b4231ffe4
parent 472959 e4f8d6d96eaf1cedb579f8b2b9eff6a5949711e3
child 472961 e33c9d2925843d53f27c4cac609b07e79d2a881a
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1458839
milestone61.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 1458839 - Improve state assertions in GCParallelTask r=jandem
js/src/gc/GC.cpp
js/src/gc/GCParallelTask.h
js/src/gc/GCRuntime.h
js/src/vm/HelperThreads.cpp
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -1316,18 +1316,18 @@ GCRuntime::finish()
     }
 
     /*
      * Wait until the background finalization and allocation stops and the
      * helper thread shuts down before we forcefully release any remaining GC
      * memory.
      */
     helperState.finish();
-    allocTask.cancel(GCParallelTask::CancelAndWait);
-    decommitTask.cancel(GCParallelTask::CancelAndWait);
+    allocTask.cancelAndWait();
+    decommitTask.cancelAndWait();
 
 #ifdef JS_GC_ZEAL
     /* Free memory associated with GC verification. */
     finishVerifier();
 #endif
 
     /* Delete all remaining zones. */
     if (rt->gcInitialized) {
@@ -7454,17 +7454,17 @@ GCRuntime::gcCycle(bool nonincrementalBy
             assertBackgroundSweepingFinished();
             MOZ_ASSERT(!decommitTask.isRunning());
         }
 
         // We must also wait for background allocation to finish so we can
         // avoid taking the GC lock when manipulating the chunks during the GC.
         // The background alloc task can run between slices, so we must wait
         // for it at the start of every slice.
-        allocTask.cancel(GCParallelTask::CancelAndWait);
+        allocTask.cancelAndWait();
     }
 
     // We don't allow off-thread parsing to start while we're doing an
     // incremental GC.
     MOZ_ASSERT_IF(rt->activeGCInAtomsZone(), !rt->hasHelperThreadZones());
 
     auto result = budgetIncrementalGC(nonincrementalByAPI, reason, budget, session);
 
@@ -7777,17 +7777,17 @@ js::PrepareForDebugGC(JSRuntime* rt)
     if (!ZonesSelected(rt))
         JS::PrepareForFullGC(rt->mainContextFromOwnThread());
 }
 
 void
 GCRuntime::onOutOfMallocMemory()
 {
     // Stop allocating new chunks.
-    allocTask.cancel(GCParallelTask::CancelAndWait);
+    allocTask.cancelAndWait();
 
     // Make sure we release anything queued for release.
     decommitTask.join();
 
     // Wait for background free of nursery huge slots to finish.
     nursery().waitBackgroundFreeEnd();
 
     AutoLockGC lock(rt);
--- a/js/src/gc/GCParallelTask.h
+++ b/js/src/gc/GCParallelTask.h
@@ -15,39 +15,44 @@ namespace js {
 // A generic task used to dispatch work to the helper thread system.
 // Users should derive from GCParallelTask add what data they need and
 // override |run|.
 class GCParallelTask
 {
     JSRuntime* const runtime_;
 
     // The state of the parallel computation.
-    enum TaskState {
+    enum class State {
         NotStarted,
         Dispatched,
-        Finished,
+        Finished
     };
-    UnprotectedData<TaskState> state;
+    UnprotectedData<State> state_;
 
     // Amount of time this task took to execute.
     MainThreadOrGCTaskData<mozilla::TimeDuration> duration_;
 
     explicit GCParallelTask(const GCParallelTask&) = delete;
 
   protected:
     // A flag to signal a request for early completion of the off-thread task.
-    mozilla::Atomic<bool> cancel_;
+    mozilla::Atomic<bool, mozilla::MemoryOrdering::ReleaseAcquire> cancel_;
 
     virtual void run() = 0;
 
   public:
-    explicit GCParallelTask(JSRuntime* runtime) : runtime_(runtime), state(NotStarted), duration_(nullptr) {}
+    explicit GCParallelTask(JSRuntime* runtime)
+      : runtime_(runtime),
+        state_(State::NotStarted),
+        duration_(nullptr),
+        cancel_(false)
+    {}
     GCParallelTask(GCParallelTask&& other)
       : runtime_(other.runtime_),
-        state(other.state),
+        state_(other.state_),
         duration_(nullptr),
         cancel_(false)
     {}
 
     // Derived classes must override this to ensure that join() gets called
     // before members get destructed.
     virtual ~GCParallelTask();
 
@@ -64,27 +69,55 @@ class GCParallelTask
     // efficient to take the helper thread lock once and use these methods.
     bool startWithLockHeld(AutoLockHelperThreadState& locked);
     void joinWithLockHeld(AutoLockHelperThreadState& locked);
 
     // Instead of dispatching to a helper, run the task on the current thread.
     void runFromMainThread(JSRuntime* rt);
 
     // Dispatch a cancelation request.
-    enum CancelMode { CancelNoWait, CancelAndWait};
-    void cancel(CancelMode mode = CancelNoWait) {
+    void cancelAndWait() {
         cancel_ = true;
-        if (mode == CancelAndWait)
-            join();
+        join();
     }
 
     // Check if a task is actively running.
-    bool isRunningWithLockHeld(const AutoLockHelperThreadState& locked) const;
+    bool isRunningWithLockHeld(const AutoLockHelperThreadState& lock) const {
+        return isDispatched(lock);
+    }
     bool isRunning() const;
 
+  private:
+    void assertNotStarted() const {
+        // Don't lock here because that adds extra synchronization in debug
+        // builds that may hide bugs. There's no race if the assertion passes.
+        MOZ_ASSERT(state_ == State::NotStarted);
+    }
+    bool isNotStarted(const AutoLockHelperThreadState& lock) const {
+        return state_ == State::NotStarted;
+    }
+    bool isDispatched(const AutoLockHelperThreadState& lock) const {
+        return state_ == State::Dispatched;
+    }
+    bool isFinished(const AutoLockHelperThreadState& lock) const {
+        return state_ == State::Finished;
+    }
+    void setDispatched(const AutoLockHelperThreadState& lock) {
+        MOZ_ASSERT(state_ == State::NotStarted);
+        state_ = State::Dispatched;
+    }
+    void setFinished(const AutoLockHelperThreadState& lock) {
+        MOZ_ASSERT(state_ == State::Dispatched);
+        state_ = State::Finished;
+    }
+    void setNotStarted(const AutoLockHelperThreadState& lock) {
+        MOZ_ASSERT(state_ == State::Finished);
+        state_ = State::NotStarted;
+    }
+
     // This should be friended to HelperThread, but cannot be because it
     // would introduce several circular dependencies.
   public:
     void runFromHelperThread(AutoLockHelperThreadState& locked);
 };
 
 } /* namespace js */
 #endif /* gc_GCParallelTask_h */
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -313,17 +313,17 @@ class GCRuntime
     // Internal public interface
     State state() const { return incrementalState; }
     bool isHeapCompacting() const { return state() == State::Compact; }
     bool isForegroundSweeping() const { return state() == State::Sweep; }
     bool isBackgroundSweeping() { return helperState.isBackgroundSweeping(); }
     void waitBackgroundSweepEnd() { helperState.waitBackgroundSweepEnd(); }
     void waitBackgroundSweepOrAllocEnd() {
         helperState.waitBackgroundSweepEnd();
-        allocTask.cancel(GCParallelTask::CancelAndWait);
+        allocTask.cancelAndWait();
     }
 
 #ifdef DEBUG
     bool onBackgroundThread() { return helperState.onBackgroundThread(); }
 #endif // DEBUG
 
     void lockGC() {
         lock.lock();
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -1448,61 +1448,56 @@ GlobalHelperThreadState::canStartGCParal
 }
 
 js::GCParallelTask::~GCParallelTask()
 {
     // Only most-derived classes' destructors may do the join: base class
     // destructors run after those for derived classes' members, so a join in a
     // base class can't ensure that the task is done using the members. All we
     // can do now is check that someone has previously stopped the task.
-#ifdef DEBUG
-    Maybe<AutoLockHelperThreadState> helperLock;
-    if (!HelperThreadState().isLockedByCurrentThread())
-        helperLock.emplace();
-    MOZ_ASSERT(state == NotStarted);
-#endif
+    assertNotStarted();
 }
 
 bool
 js::GCParallelTask::startWithLockHeld(AutoLockHelperThreadState& lock)
 {
-    // Tasks cannot be started twice.
-    MOZ_ASSERT(state == NotStarted);
+    assertNotStarted();
 
     // If we do the shutdown GC before running anything, we may never
     // have initialized the helper threads. Just use the serial path
     // since we cannot safely intialize them at this point.
     if (!HelperThreadState().threads)
         return false;
 
     if (!HelperThreadState().gcParallelWorklist(lock).append(this))
         return false;
-    state = Dispatched;
+    setDispatched(lock);
 
     HelperThreadState().notifyOne(GlobalHelperThreadState::PRODUCER, lock);
 
     return true;
 }
 
 bool
 js::GCParallelTask::start()
 {
     AutoLockHelperThreadState helperLock;
     return startWithLockHeld(helperLock);
 }
 
 void
-js::GCParallelTask::joinWithLockHeld(AutoLockHelperThreadState& locked)
+js::GCParallelTask::joinWithLockHeld(AutoLockHelperThreadState& lock)
 {
-    if (state == NotStarted)
+    if (isNotStarted(lock))
         return;
 
-    while (state != Finished)
-        HelperThreadState().wait(locked, GlobalHelperThreadState::CONSUMER);
-    state = NotStarted;
+    while (!isFinished(lock))
+        HelperThreadState().wait(lock, GlobalHelperThreadState::CONSUMER);
+
+    setNotStarted(lock);
     cancel_ = false;
 }
 
 void
 js::GCParallelTask::join()
 {
     AutoLockHelperThreadState helperLock;
     joinWithLockHeld(helperLock);
@@ -1518,66 +1513,62 @@ TimeSince(TimeStamp prev)
     if (now < prev)
         now = prev;
     return now - prev;
 }
 
 void
 js::GCParallelTask::runFromMainThread(JSRuntime* rt)
 {
-    MOZ_ASSERT(state == NotStarted);
+    assertNotStarted();
     MOZ_ASSERT(js::CurrentThreadCanAccessRuntime(rt));
     TimeStamp timeStart = TimeStamp::Now();
     run();
     duration_ = TimeSince(timeStart);
 }
 
 void
-js::GCParallelTask::runFromHelperThread(AutoLockHelperThreadState& locked)
+js::GCParallelTask::runFromHelperThread(AutoLockHelperThreadState& lock)
 {
+    MOZ_ASSERT(isDispatched(lock));
+
     AutoSetContextRuntime ascr(runtime());
     gc::AutoSetThreadIsPerformingGC performingGC;
 
     {
-        AutoUnlockHelperThreadState parallelSection(locked);
+        AutoUnlockHelperThreadState parallelSection(lock);
         TimeStamp timeStart = TimeStamp::Now();
         TlsContext.get()->heapState = JS::HeapState::MajorCollecting;
         run();
         TlsContext.get()->heapState = JS::HeapState::Idle;
         duration_ = TimeSince(timeStart);
     }
 
-    state = Finished;
-    HelperThreadState().notifyAll(GlobalHelperThreadState::CONSUMER, locked);
-}
-
-bool
-js::GCParallelTask::isRunningWithLockHeld(const AutoLockHelperThreadState& locked) const
-{
-    return state == Dispatched;
+    setFinished(lock);
+    HelperThreadState().notifyAll(GlobalHelperThreadState::CONSUMER, lock);
 }
 
 bool
 js::GCParallelTask::isRunning() const
 {
-    AutoLockHelperThreadState helperLock;
-    return isRunningWithLockHeld(helperLock);
+    AutoLockHelperThreadState lock;
+    return isRunningWithLockHeld(lock);
 }
 
 void
-HelperThread::handleGCParallelWorkload(AutoLockHelperThreadState& locked)
+HelperThread::handleGCParallelWorkload(AutoLockHelperThreadState& lock)
 {
-    MOZ_ASSERT(HelperThreadState().canStartGCParallelTask(locked));
+    MOZ_ASSERT(HelperThreadState().canStartGCParallelTask(lock));
     MOZ_ASSERT(idle());
 
     TraceLoggerThread* logger = TraceLoggerForCurrentThread();
     AutoTraceLog logCompile(logger, TraceLogger_GC);
 
-    currentTask.emplace(HelperThreadState().gcParallelWorklist(locked).popCopy());
-    gcParallelTask()->runFromHelperThread(locked);
+    currentTask.emplace(HelperThreadState().gcParallelWorklist(lock).popCopy());
+    gcParallelTask()->runFromHelperThread(lock);
     currentTask.reset();
 }
 
 static void
 LeaveParseTaskZone(JSRuntime* rt, ParseTask* task)
 {
     // Mark the zone as no longer in use by a helper thread, and available
     // to be collected by the GC.