Bug 1853400 - Part 2: Don't call the helper thread task dispatch callback with the helper thread lock held r=jandem

This works by keeping a count of tasks to dispatch on AutoLockHelperThreadState
which are handled when it is unlocked.  One issue is that we must take care not
to wait for a helper task to complete before we've had a chance to displach the
task we're going to wait on.

Differential Revision: https://phabricator.services.mozilla.com/D208906

js/src/gc/GCParallelTask.cpp

@@ -100,6 +100,12 @@ void js::GCParallelTask::joinWithLockHeld(AutoLockHelperThreadState& lock,
     return;
   }
 
+  if (lock.hasQueuedTasks()) {
+    // Unlock to allow task dispatch without lock held, otherwise we could wait
+    // forever.
+    AutoUnlockHelperThreadState unlock(lock);
+  }
+
   if (isNotYetRunning(lock) && deadline.isNothing()) {
     // If the task was dispatched but has not yet started then cancel the task
     // and run it from the main thread. This stops us from blocking here when

js/src/gc/GCParallelTask.h

@@ -161,8 +161,6 @@ class GCParallelTask : private mozilla::LinkedListElement<GCParallelTask>,
   void joinWithLockHeld(
       AutoLockHelperThreadState& lock,
       mozilla::Maybe<mozilla::TimeStamp> deadline = mozilla::Nothing());
-  void joinNonIdleTask(mozilla::Maybe<mozilla::TimeStamp> deadline,
-                       AutoLockHelperThreadState& lock);
 
   // Instead of dispatching to a helper, run the task on the current thread.
   void runFromMainThread();
@@ -247,6 +245,9 @@ class GCParallelTask : private mozilla::LinkedListElement<GCParallelTask>,
   }
   friend class gc::GCRuntime;
 
+  void joinNonIdleTask(mozilla::Maybe<mozilla::TimeStamp> deadline,
+                       AutoLockHelperThreadState& lock);
+
   void runTask(JS::GCContext* gcx, AutoLockHelperThreadState& lock);
 
   // Implement the HelperThreadTask interface.

js/src/vm/HelperThreadState.h

@@ -81,8 +81,6 @@ typedef Vector<Tier2GeneratorTask*, 0, SystemAllocPolicy>
 
 // Per-process state for off thread work items.
 class GlobalHelperThreadState {
-  friend class AutoLockHelperThreadState;
-
  public:
   // A single tier-2 ModuleGenerator job spawns many compilation jobs, and we
   // do not want to allow more than one such ModuleGenerator to run at a time.
@@ -181,6 +179,7 @@ class GlobalHelperThreadState {
   // JS::SetHelperThreadTaskCallback. If this is not set the internal thread
   // pool is used.
   JS::HelperThreadTaskCallback dispatchTaskCallback = nullptr;
+  friend class AutoHelperTaskQueue;
 
   // The number of tasks dispatched to the thread pool that have not started
   // running yet.
@@ -227,7 +226,7 @@ class GlobalHelperThreadState {
   void assertIsLockedByCurrentThread() const;
 #endif
 
-  void wait(AutoLockHelperThreadState& locked,
+  void wait(AutoLockHelperThreadState& lock,
             mozilla::TimeDuration timeout = mozilla::TimeDuration::Forever());
   void notifyAll(const AutoLockHelperThreadState&);
 

js/src/vm/HelperThreads.cpp

@@ -889,25 +889,23 @@ void GlobalHelperThreadState::assertIsLockedByCurrentThread() const {
 }
 #endif  // DEBUG
 
-void GlobalHelperThreadState::dispatch(
-    DispatchReason reason, const AutoLockHelperThreadState& locked) {
-  if (canStartTasks(locked) && tasksPending_ < threadCount) {
+void GlobalHelperThreadState::dispatch(DispatchReason reason,
+                                       const AutoLockHelperThreadState& lock) {
+  if (canStartTasks(lock) && tasksPending_ < threadCount) {
     // This doesn't guarantee that we don't dispatch more tasks to the external
     // pool than necessary if tasks are taking a long time to start, but it does
     // limit the number.
     tasksPending_++;
 
-    // The hazard analysis can't tell that the callback doesn't GC.
-    JS::AutoSuppressGCAnalysis nogc;
-
-    dispatchTaskCallback(reason);
+    lock.queueTaskToDispatch(reason);
   }
 }
 
 void GlobalHelperThreadState::wait(
-    AutoLockHelperThreadState& locked,
+    AutoLockHelperThreadState& lock,
     TimeDuration timeout /* = TimeDuration::Forever() */) {
-  consumerWakeup.wait_for(locked, timeout);
+  MOZ_ASSERT(!lock.hasQueuedTasks());
+  consumerWakeup.wait_for(lock, timeout);
 }
 
 void GlobalHelperThreadState::notifyAll(const AutoLockHelperThreadState&) {
@@ -1534,6 +1532,10 @@ void js::RunPendingSourceCompressions(JSRuntime* runtime) {
 
   HelperThreadState().startHandlingCompressionTasks(
       GlobalHelperThreadState::ScheduleCompressionTask::API, nullptr, lock);
+  {
+    // Dispatch tasks.
+    AutoUnlockHelperThreadState unlock(lock);
+  }
 
   // Wait until all tasks have started compression.
   while (!HelperThreadState().compressionWorklist(lock).empty()) {
@@ -1735,3 +1737,28 @@ void GlobalHelperThreadState::runTaskLocked(HelperThreadTask* task,
 
   js::oom::SetThreadType(js::THREAD_TYPE_NONE);
 }
+
+void AutoHelperTaskQueue::queueTaskToDispatch(JS::DispatchReason reason) const {
+  // This is marked const because it doesn't release the mutex.
+
+  if (reason == JS::DispatchReason::FinishedTask) {
+    finishedTasksToDispatch++;
+    return;
+  }
+
+  newTasksToDispatch++;
+}
+
+void AutoHelperTaskQueue::dispatchQueuedTasks() {
+  // The hazard analysis can't tell that the callback doesn't GC.
+  JS::AutoSuppressGCAnalysis nogc;
+
+  for (size_t i = 0; i < newTasksToDispatch; i++) {
+    HelperThreadState().dispatchTaskCallback(JS::DispatchReason::NewTask);
+  }
+  for (size_t i = 0; i < finishedTasksToDispatch; i++) {
+    HelperThreadState().dispatchTaskCallback(JS::DispatchReason::FinishedTask);
+  }
+  newTasksToDispatch = 0;
+  finishedTasksToDispatch = 0;
+}

js/src/vm/HelperThreads.h

@@ -14,6 +14,7 @@
 #include "mozilla/Variant.h"
 
 #include "js/AllocPolicy.h"
+#include "js/HelperThreadAPI.h"
 #include "js/shadow/Zone.h"
 #include "js/UniquePtr.h"
 #include "js/Vector.h"
@@ -68,12 +69,40 @@ using UniqueTier2GeneratorTask = UniquePtr<Tier2GeneratorTask>;
  */
 extern Mutex gHelperThreadLock MOZ_UNANNOTATED;
 
-class MOZ_RAII AutoLockHelperThreadState : public LockGuard<Mutex> {
-  using Base = LockGuard<Mutex>;
-  friend class UnlockGuard<AutoLockHelperThreadState>;
-
+// Set of tasks to dispatch when the helper thread state lock is released.
+class AutoHelperTaskQueue {
  public:
-  explicit AutoLockHelperThreadState() : Base(gHelperThreadLock) {}
+  ~AutoHelperTaskQueue() { dispatchQueuedTasks(); }
+  bool hasQueuedTasks() const {
+    return newTasksToDispatch || finishedTasksToDispatch;
+  }
+  void queueTaskToDispatch(JS::DispatchReason reason) const;
+  void dispatchQueuedTasks();
+
+ private:
+  mutable uint32_t newTasksToDispatch = 0;
+  mutable uint32_t finishedTasksToDispatch = 0;
+};
+
+// A lock guard for data protected by the helper thread lock.
+//
+// This can also queue helper thread tasks to be triggered when the lock is
+// released.
+class MOZ_RAII AutoLockHelperThreadState
+    : public AutoHelperTaskQueue,  // Must come before LockGuard.
+      public LockGuard<Mutex> {
+ public:
+  AutoLockHelperThreadState() : LockGuard<Mutex>(gHelperThreadLock) {}
+  AutoLockHelperThreadState(const AutoLockHelperThreadState&) = delete;
+
+ private:
+  friend class UnlockGuard<AutoLockHelperThreadState>;
+  void unlock() {
+    LockGuard<Mutex>::unlock();
+    dispatchQueuedTasks();
+  }
+
+  friend class GlobalHelperThreadState;
 };
 
 using AutoUnlockHelperThreadState = UnlockGuard<AutoLockHelperThreadState>;

js/src/vm/InternalThreadPool.cpp

@@ -187,7 +187,10 @@ void InternalThreadPool::DispatchTask(JS::DispatchReason reason) {
 }
 
 void InternalThreadPool::dispatchTask(JS::DispatchReason reason) {
-  gHelperThreadLock.assertOwnedByCurrentThread();
+  // This could now use a separate mutex like TaskController, but continues to
+  // use the helper thread state lock for convenience.
+  AutoLockHelperThreadState lock;
+
   queuedTasks++;
   if (reason == JS::DispatchReason::NewTask) {
     wakeup.notify_one();
@@ -279,7 +282,9 @@ void HelperThread::threadLoop(InternalThreadPool* pool) {
   while (!pool->terminating) {
     if (pool->queuedTasks != 0) {
       pool->queuedTasks--;
-      HelperThreadState().runOneTask(lock);
+
+      AutoUnlockHelperThreadState unlock(lock);
+      JS::RunHelperThreadTask();
       continue;
     }