Bug 1518193 - Move BackgroundSweepTask functionality into GCParallelTask base class r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 10 Jan 2019 11:00:19 +0000
changeset 453215 86f8236cd20fa9f805cc78b8138b1f7a7fb81220
parent 453214 676b002d0640f8bc91806f65043f5bdf28f93556
child 453216 1bdbec17ea7c84775814586a6b21536c6f12d7c2
push id111060
push userjcoppeard@mozilla.com
push dateThu, 10 Jan 2019 11:00:42 +0000
treeherdermozilla-inbound@3922da7f8c51 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1518193
milestone66.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 1518193 - Move BackgroundSweepTask functionality into GCParallelTask base class r=sfink
js/src/gc/GC.cpp
js/src/gc/GCParallelTask.h
js/src/gc/GCRuntime.h
js/src/gc/Nursery.cpp
js/src/vm/HelperThreads.cpp
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -3552,17 +3552,17 @@ void GCRuntime::assertBackgroundSweeping
   }
 #endif
 }
 
 void GCRuntime::queueZonesForBackgroundSweep(ZoneList& zones) {
   AutoLockHelperThreadState lock;
   backgroundSweepZones.ref().transferFrom(zones);
   if (sweepOnBackgroundThread) {
-    sweepTask.startIfIdle(lock);
+    sweepTask.startOrRunIfIdle(lock);
   }
 }
 
 void GCRuntime::freeUnusedLifoBlocksAfterSweeping(LifoAlloc* lifo) {
   MOZ_ASSERT(JS::RuntimeHeapIsBusy());
   AutoLockHelperThreadState lock;
   blocksToFreeAfterSweeping.ref().transferUnusedFrom(lifo);
 }
@@ -3572,73 +3572,28 @@ void GCRuntime::freeAllLifoBlocksAfterSw
   AutoLockHelperThreadState lock;
   blocksToFreeAfterSweeping.ref().transferFrom(lifo);
 }
 
 void GCRuntime::freeAllLifoBlocksAfterMinorGC(LifoAlloc* lifo) {
   blocksToFreeAfterMinorGC.ref().transferFrom(lifo);
 }
 
-BackgroundSweepTask::BackgroundSweepTask(JSRuntime* rt)
-    : GCParallelTaskHelper(rt), done(false) {}
-
-bool BackgroundSweepTask::isRunning() const {
-  AutoLockHelperThreadState lock;
-  return isRunningWithLockHeld(lock);
-}
-
-bool BackgroundSweepTask::isRunningWithLockHeld(
-    const AutoLockHelperThreadState& lock) const {
-  return Base::isRunningWithLockHeld(lock) && !done;
-}
-
-void BackgroundSweepTask::startIfIdle(AutoLockHelperThreadState& lock) {
-  MOZ_ASSERT(CanUseExtraThreads());
-
-  if (isRunningWithLockHeld(lock)) {
-    return;
-  }
-
-  // Join the previous invocation of the task. This will return immediately
-  // if the thread has never been started.
-  joinWithLockHeld(lock);
-
-  done = false;
-
-  if (!startWithLockHeld(lock)) {
-    AutoUnlockHelperThreadState unlock(lock);
-    runFromMainThread(runtime());
-  }
-}
-
-void BackgroundSweepTask::runFromMainThread(JSRuntime* rt) {
-  {
-    AutoLockHelperThreadState lock;
-    MOZ_ASSERT(!isRunningWithLockHeld(lock));
-    joinWithLockHeld(lock);
-    done = false;
-  }
-
-  Base::runFromMainThread(rt);
-}
-
 void BackgroundSweepTask::run() {
   AutoTraceLog logSweeping(TraceLoggerForCurrentThread(),
                            TraceLogger_GCSweeping);
 
   AutoLockHelperThreadState lock;
   AutoSetThreadIsSweeping threadIsSweeping;
 
-  MOZ_ASSERT(!done);
-
   runtime()->gc.sweepFromBackgroundThread(lock);
 
-  // Signal to the main thread that we're finished, because we release the
-  // lock again before GCParallelTask's state is changed to finished.
-  done = true;
+  // Signal to the main thread that we're about to finish, because we release
+  // the lock again before GCParallelTask's state is changed to finished.
+  setFinishing(lock);
 }
 
 void GCRuntime::sweepFromBackgroundThread(AutoLockHelperThreadState& lock) {
   do {
     ZoneList zones;
     zones.transferFrom(backgroundSweepZones.ref());
     LifoAlloc freeLifoAlloc(JSContext::TEMP_LIFO_ALLOC_PRIMARY_CHUNK_SIZE);
     freeLifoAlloc.transferFrom(&blocksToFreeAfterSweeping.ref());
@@ -5447,17 +5402,17 @@ static void SweepWeakMaps(GCParallelTask
 static void SweepUniqueIds(GCParallelTask* task) {
   for (SweepGroupZonesIter zone(task->runtime()); !zone.done(); zone.next()) {
     zone->sweepUniqueIds();
   }
 }
 
 void GCRuntime::startTask(GCParallelTask& task, gcstats::PhaseKind phase,
                           AutoLockHelperThreadState& locked) {
-  if (!task.startWithLockHeld(locked)) {
+  if (!CanUseExtraThreads() || !task.startWithLockHeld(locked)) {
     AutoUnlockHelperThreadState unlock(locked);
     gcstats::AutoPhase ap(stats(), phase);
     task.runFromMainThread(rt);
   }
 }
 
 void GCRuntime::joinTask(GCParallelTask& task, gcstats::PhaseKind phase,
                          AutoLockHelperThreadState& locked) {
@@ -5785,17 +5740,17 @@ IncrementalProgress GCRuntime::endSweepi
   }
   if (sweepAtomsZone) {
     zones.append(atomsZone);
   }
 
   queueZonesForBackgroundSweep(zones);
 
   if (!sweepOnBackgroundThread) {
-    sweepTask.runFromMainThread(rt);
+    sweepTask.joinAndRunFromMainThread(rt);
   }
 
   return Finished;
 }
 
 void GCRuntime::beginSweepPhase(JS::gcreason::Reason reason,
                                 AutoGCSession& session) {
   /*
--- a/js/src/gc/GCParallelTask.h
+++ b/js/src/gc/GCParallelTask.h
@@ -26,17 +26,17 @@ class GCParallelTask {
  public:
   using TaskFunc = void (*)(GCParallelTask*);
 
  private:
   JSRuntime* const runtime_;
   TaskFunc func_;
 
   // The state of the parallel computation.
-  enum class State { NotStarted, Dispatched, Finished };
+  enum class State { NotStarted, Dispatched, Finishing, Finished };
   UnprotectedData<State> state_;
 
   // Amount of time this task took to execute.
   MainThreadOrGCTaskData<mozilla::TimeDuration> duration_;
 
   explicit GCParallelTask(const GCParallelTask&) = delete;
 
  protected:
@@ -74,24 +74,29 @@ class GCParallelTask {
 
   // If multiple tasks are to be started or joined at once, it is more
   // efficient to take the helper thread lock once and use these methods.
   MOZ_MUST_USE 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);
+  void joinAndRunFromMainThread(JSRuntime* rt);
+
+  // If the task is not already running, either start it or run it on the main
+  // thread if that fails.
+  void startOrRunIfIdle(AutoLockHelperThreadState& lock);
 
   // Dispatch a cancelation request.
   void cancelAndWait() {
     cancel_ = true;
     join();
   }
 
-  // Check if a task is actively running.
+  // Check if a task is running and has not called setFinishing().
   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
@@ -107,26 +112,36 @@ class GCParallelTask {
   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);
+    MOZ_ASSERT(state_ == State::Dispatched || state_ == State::Finishing);
     state_ = State::Finished;
   }
   void setNotStarted(const AutoLockHelperThreadState& lock) {
     MOZ_ASSERT(state_ == State::Finished);
     state_ = State::NotStarted;
   }
 
   void runTask() { func_(this); }
 
+ protected:
+  // Can be called to indicate that although the task is still
+  // running, it is about to finish.
+  void setFinishing(const AutoLockHelperThreadState& lock) {
+    MOZ_ASSERT(state_ == State::NotStarted || state_ == State::Dispatched);
+    if (state_ == State::Dispatched) {
+      state_ = State::Finishing;
+    }
+  }
+
   // This should be friended to HelperThread, but cannot be because it
   // would introduce several circular dependencies.
  public:
   void runFromHelperThread(AutoLockHelperThreadState& locked);
 };
 
 // CRTP template to handle cast to derived type when calling run().
 template <typename Derived>
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -101,29 +101,18 @@ class ChunkPool {
     Chunk* operator->() const { return get(); }
 
    private:
     Chunk* current_;
   };
 };
 
 class BackgroundSweepTask : public GCParallelTaskHelper<BackgroundSweepTask> {
-  using Base = GCParallelTaskHelper<BackgroundSweepTask>;
-
-  HelperThreadLockData<bool> done;
-
  public:
-  explicit BackgroundSweepTask(JSRuntime* rt);
-
-  bool isRunning() const;
-  bool isRunningWithLockHeld(const AutoLockHelperThreadState& lock) const;
-
-  void startIfIdle(AutoLockHelperThreadState& lock);
-  void runFromMainThread(JSRuntime* rt);
-
+  explicit BackgroundSweepTask(JSRuntime* rt) : GCParallelTaskHelper(rt) {}
   void run();
 };
 
 // Performs extra allocation off thread so that when memory is required on the
 // main thread it will already be available and waiting.
 class BackgroundAllocTask : public GCParallelTaskHelper<BackgroundAllocTask> {
   // Guarded by the GC lock.
   GCLockData<ChunkPool&> chunkPool_;
--- a/js/src/gc/Nursery.cpp
+++ b/js/src/gc/Nursery.cpp
@@ -1038,22 +1038,24 @@ bool js::Nursery::registerMallocedBuffer
   return mallocedBuffers.putNew(buffer);
 }
 
 void js::Nursery::freeMallocedBuffers() {
   if (mallocedBuffers.empty()) {
     return;
   }
 
-  bool started;
+  bool started = false;
   {
     AutoLockHelperThreadState lock;
     freeMallocedBuffersTask->joinWithLockHeld(lock);
     freeMallocedBuffersTask->transferBuffersToFree(mallocedBuffers, lock);
-    started = freeMallocedBuffersTask->startWithLockHeld(lock);
+    if (CanUseExtraThreads()) {
+      started = freeMallocedBuffersTask->startWithLockHeld(lock);
+    }
   }
 
   if (!started) {
     freeMallocedBuffersTask->runFromMainThread(runtime());
   }
 
   MOZ_ASSERT(mallocedBuffers.empty());
 }
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -1523,16 +1523,17 @@ 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.
   assertNotStarted();
 }
 
 bool js::GCParallelTask::startWithLockHeld(AutoLockHelperThreadState& lock) {
+  MOZ_ASSERT(CanUseExtraThreads());
   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;
   }
@@ -1547,16 +1548,31 @@ bool js::GCParallelTask::startWithLockHe
   return true;
 }
 
 bool js::GCParallelTask::start() {
   AutoLockHelperThreadState helperLock;
   return startWithLockHeld(helperLock);
 }
 
+void js::GCParallelTask::startOrRunIfIdle(AutoLockHelperThreadState& lock) {
+  if (isRunningWithLockHeld(lock)) {
+    return;
+  }
+
+  // Join the previous invocation of the task. This will return immediately
+  // if the thread has never been started.
+  joinWithLockHeld(lock);
+
+  if (!startWithLockHeld(lock)) {
+    AutoUnlockHelperThreadState unlock(lock);
+    runFromMainThread(runtime());
+  }
+}
+
 void js::GCParallelTask::joinWithLockHeld(AutoLockHelperThreadState& lock) {
   if (isNotStarted(lock)) {
     return;
   }
 
   while (!isFinished(lock)) {
     HelperThreadState().wait(lock, GlobalHelperThreadState::CONSUMER);
   }
@@ -1575,16 +1591,26 @@ static inline TimeDuration TimeSince(Tim
   // Sadly this happens sometimes.
   MOZ_ASSERT(now >= prev);
   if (now < prev) {
     now = prev;
   }
   return now - prev;
 }
 
+void GCParallelTask::joinAndRunFromMainThread(JSRuntime* rt) {
+  {
+    AutoLockHelperThreadState lock;
+    MOZ_ASSERT(!isRunningWithLockHeld(lock));
+    joinWithLockHeld(lock);
+  }
+
+  runFromMainThread(rt);
+}
+
 void js::GCParallelTask::runFromMainThread(JSRuntime* rt) {
   assertNotStarted();
   MOZ_ASSERT(js::CurrentThreadCanAccessRuntime(rt));
   TimeStamp timeStart = ReallyNow();
   runTask();
   duration_ = TimeSince(timeStart);
 }