Bug 1379814 - Clean up task selection logic, implement master task concept (take 2). r=luke

--HG--
extra : rebase_source : 2e865028e8921e5043dabab0acce2bb915b1256c
extra : histedit_source : f093356241f3fd5e2681e517e1c66f660f715503

js/src/vm/HelperThreads.cpp

@@ -963,21 +963,56 @@ GlobalHelperThreadState::waitForAllThreads()
         wait(lock, CONSUMER);
 }
 
+// A task can be a "master" task, ie, it will block waiting for other worker
+// threads that perform work on its behalf.  If so it must not take the last
+// available thread; there must always be at least one worker thread able to do
+// the actual work.  (Or the system may deadlock.)
+//
+// If a task is a master task it *must* pass isMaster=true here, or perform a
+// similar calculation to avoid deadlock from starvation.
+//
+// isMaster should only be true if the thread calling checkTaskThreadLimit() is
+// a helper thread.
+//
+// NOTE: Calling checkTaskThreadLimit() from a helper thread in the dynamic
+// region after currentTask.emplace() and before currentTask.reset() may cause
+// it to return a different result than if it is called outside that dynamic
+// region, as the predicate inspects the values of the threads' currentTask
+// members.
+
 template <typename T>
 bool
-GlobalHelperThreadState::checkTaskThreadLimit(size_t maxThreads) const
+GlobalHelperThreadState::checkTaskThreadLimit(size_t maxThreads, bool isMaster) const
 {
-    if (maxThreads >= threadCount)
+    MOZ_ASSERT(maxThreads > 0);
+
+    if (!isMaster && maxThreads >= threadCount)
         return true;
 
     size_t count = 0;
+    size_t idle = 0;
     for (auto& thread : *threads) {
-        if (thread.currentTask.isSome() && thread.currentTask->is<T>())
-            count++;
+        if (thread.currentTask.isSome()) {
+            if (thread.currentTask->is<T>())
+                count++;
+        } else {
+            idle++;
+        }
         if (count >= maxThreads)
             return false;
     }
 
+    // It is possible for the number of idle threads to be zero here, because
+    // checkTaskThreadLimit() can be called from non-helper threads.  Notably,
+    // the compression task scheduler invokes it, and runs off a helper thread.
+    if (idle == 0)
+        return false;
+
+    // A master thread that's the last available thread must not be allowed to
+    // run.
+    if (isMaster && idle == 1)
+        return false;
+
     return true;
 }
 
@@ -1213,7 +1248,12 @@ GlobalHelperThreadState::pendingIonCompileHasSufficientPriority(
 bool
 GlobalHelperThreadState::canStartParseTask(const AutoLockHelperThreadState& lock)
 {
-    return !parseWorklist(lock).empty() && checkTaskThreadLimit<ParseTask*>(maxParseThreads());
+    // Parse tasks that end up compiling asm.js in turn may use Wasm compilation
+    // threads to generate machine code.  We have no way (at present) to know
+    // ahead of time whether a parse task is going to parse asm.js content or
+    // not, so we just assume that all parse tasks are master tasks.
+    return !parseWorklist(lock).empty() &&
+           checkTaskThreadLimit<ParseTask*>(maxParseThreads(), /*isMaster=*/true);
 }
 
 bool
@@ -2142,53 +2182,74 @@ HelperThread::threadLoop()
     while (true) {
         MOZ_ASSERT(idle());
 
-        // Block until a task is available. Save the value of whether we are
-        // going to do an Ion compile, in case the value returned by the method
-        // changes.
-        bool ionCompile = false;
+        js::oom::ThreadType task;
         while (true) {
             if (terminate)
                 return;
-            if ((ionCompile = HelperThreadState().pendingIonCompileHasSufficientPriority(lock)) ||
-                HelperThreadState().canStartWasmCompile(lock) ||
-                HelperThreadState().canStartPromiseTask(lock) ||
-                HelperThreadState().canStartParseTask(lock) ||
-                HelperThreadState().canStartCompressionTask(lock) ||
-                HelperThreadState().canStartGCHelperTask(lock) ||
-                HelperThreadState().canStartGCParallelTask(lock) ||
-                HelperThreadState().canStartIonFreeTask(lock))
-            {
+
+            // Select the task type to run.  Task priority is determined
+            // exclusively here.
+            //
+            // The selectors may depend on the HelperThreadState not changing
+            // between task selection and task execution, in particular, on new
+            // tasks not being added (because of the lifo structure of the work
+            // lists).  Unlocking the HelperThreadState between task selection
+            // and execution is not well-defined.
+
+            if (HelperThreadState().canStartGCParallelTask(lock))
+                task = js::oom::THREAD_TYPE_GCPARALLEL;
+            else if (HelperThreadState().canStartGCHelperTask(lock))
+                task = js::oom::THREAD_TYPE_GCHELPER;
+            else if (HelperThreadState().pendingIonCompileHasSufficientPriority(lock))
+                task = js::oom::THREAD_TYPE_ION;
+            else if (HelperThreadState().canStartWasmCompile(lock))
+                task = js::oom::THREAD_TYPE_WASM;
+            else if (HelperThreadState().canStartPromiseTask(lock))
+                task = js::oom::THREAD_TYPE_PROMISE_TASK;
+            else if (HelperThreadState().canStartParseTask(lock))
+                task = js::oom::THREAD_TYPE_PARSE;
+            else if (HelperThreadState().canStartCompressionTask(lock))
+                task = js::oom::THREAD_TYPE_COMPRESS;
+            else if (HelperThreadState().canStartIonFreeTask(lock))
+                task = js::oom::THREAD_TYPE_ION_FREE;
+            else
+                task = js::oom::THREAD_TYPE_NONE;
+
+            if (task != js::oom::THREAD_TYPE_NONE)
                 break;
-            }
+
             HelperThreadState().wait(lock, GlobalHelperThreadState::PRODUCER);
         }
 
-        if (HelperThreadState().canStartGCParallelTask(lock)) {
-            js::oom::SetThreadType(js::oom::THREAD_TYPE_GCPARALLEL);
+        js::oom::SetThreadType(task);
+        switch (task) {
+          case js::oom::THREAD_TYPE_GCPARALLEL:
             handleGCParallelWorkload(lock);
-        } else if (HelperThreadState().canStartGCHelperTask(lock)) {
-            js::oom::SetThreadType(js::oom::THREAD_TYPE_GCHELPER);
+            break;
+          case js::oom::THREAD_TYPE_GCHELPER:
             handleGCHelperWorkload(lock);
-        } else if (ionCompile) {
-            js::oom::SetThreadType(js::oom::THREAD_TYPE_ION);
+            break;
+          case js::oom::THREAD_TYPE_ION:
             handleIonWorkload(lock);
-        } else if (HelperThreadState().canStartWasmCompile(lock)) {
-            js::oom::SetThreadType(js::oom::THREAD_TYPE_WASM);
+            break;
+          case js::oom::THREAD_TYPE_WASM:
             handleWasmWorkload(lock);
-        } else if (HelperThreadState().canStartPromiseTask(lock)) {
-            js::oom::SetThreadType(js::oom::THREAD_TYPE_PROMISE_TASK);
+            break;
+          case js::oom::THREAD_TYPE_PROMISE_TASK:
             handlePromiseTaskWorkload(lock);
-        } else if (HelperThreadState().canStartParseTask(lock)) {
-            js::oom::SetThreadType(js::oom::THREAD_TYPE_PARSE);
+            break;
+          case js::oom::THREAD_TYPE_PARSE:
             handleParseWorkload(lock);
-        } else if (HelperThreadState().canStartCompressionTask(lock)) {
-            js::oom::SetThreadType(js::oom::THREAD_TYPE_COMPRESS);
+            break;
+          case js::oom::THREAD_TYPE_COMPRESS:
             handleCompressionWorkload(lock);
-        } else if (HelperThreadState().canStartIonFreeTask(lock)) {
-            js::oom::SetThreadType(js::oom::THREAD_TYPE_ION_FREE);
+            break;
+          case js::oom::THREAD_TYPE_ION_FREE:
             handleIonFreeWorkload(lock);
-        } else {
+            break;
+          default:
             MOZ_CRASH("No task to perform");
         }
+        js::oom::SetThreadType(js::oom::THREAD_TYPE_NONE);
     }
 }

js/src/vm/HelperThreads.h

@@ -146,10 +146,14 @@ class GlobalHelperThreadState
 #endif
 
     enum CondVar {
-        // For notifying threads waiting for work that they may be able to make progress.
+        // For notifying threads waiting for work that they may be able to make
+        // progress, ie, a work item has been completed by a helper thread and
+        // the thread that created the work item can now consume it.
         CONSUMER,
 
-        // For notifying threads doing work that they may be able to make progress.
+        // For notifying helper threads doing the work that they may be able to
+        // make progress, ie, a work item has been enqueued and an idle helper
+        // thread may pick up up the work item and perform it.
         PRODUCER,
 
         // For notifying threads doing work which are paused that they may be
@@ -235,10 +239,14 @@ class GlobalHelperThreadState
 
     // Used by a major GC to signal processing enqueued compression tasks.
     void startHandlingCompressionTasks(const AutoLockHelperThreadState&);
+
+  private:
     void scheduleCompressionTasks(const AutoLockHelperThreadState&);
 
-    // Unlike the methods above, the value returned by this method can change
-    // over time, even if the helper thread state lock is held throughout.
+  public:
+    // Unlike the public methods above, the value returned by this method can
+    // change over time, even if the helper thread state lock is held
+    // throughout.
     bool pendingIonCompileHasSufficientPriority(const AutoLockHelperThreadState& lock);
 
     jit::IonBuilder* highestPriorityPendingIonCompile(const AutoLockHelperThreadState& lock,
@@ -310,7 +318,7 @@ class GlobalHelperThreadState
     void waitForAllThreads();
 
     template <typename T>
-    bool checkTaskThreadLimit(size_t maxThreads) const;
+    bool checkTaskThreadLimit(size_t maxThreads, bool isMaster = false) const;
 
   private: