Bug 1379814 - Clean up task selection logic, implement master task concept. r=luke
☠☠ backed out by c4eb424ae518 ☠ ☠
authorLars T Hansen <lhansen@mozilla.com>
Wed, 12 Jul 2017 14:33:17 -0700
changeset 374274 806a941c858051eceb8076b056fb278dcc259647
parent 374273 7fefe0f46c4648f92d75e02a5c5bf8b20a6f799d
child 374275 3a7bd8419732cb5a944b331d168729b9c9e6e8de
push id32318
push userkwierso@gmail.com
push dateFri, 11 Aug 2017 20:16:01 +0000
treeherdermozilla-central@80ff3f300e05 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs1379814
milestone57.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 1379814 - Clean up task selection logic, implement master task concept. r=luke
js/src/vm/HelperThreads.cpp
js/src/vm/HelperThreads.h
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -958,31 +958,52 @@ GlobalHelperThreadState::waitForAllThrea
 {
     CancelOffThreadIonCompile();
 
     AutoLockHelperThreadState lock;
     while (hasActiveThreads(lock))
         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.
+
 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;
     }
 
+    // At least the current thread is idle.
+    MOZ_ASSERT(idle > 0);
+
+    if (isMaster && idle <= 1)
+        return false;
+
     return true;
 }
 
 struct MOZ_RAII AutoSetContextRuntime
 {
     explicit AutoSetContextRuntime(JSRuntime* rt) {
         TlsContext.get()->setRuntime(rt);
     }
@@ -1208,17 +1229,22 @@ GlobalHelperThreadState::pendingIonCompi
 
     // Compilation will have to wait until one of the active compilations finishes.
     return false;
 }
 
 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
 GlobalHelperThreadState::canStartCompressionTask(const AutoLockHelperThreadState& lock)
 {
     return !compressionWorklist(lock).empty() &&
            checkTaskThreadLimit<SourceCompressionTask*>(maxCompressionThreads());
 }
@@ -2137,58 +2163,79 @@ HelperThread::threadLoop()
             oomUnsafe.crash("HelperThread cx.init()");
     }
     cx.setHelperThread(this);
     JS_SetNativeStackQuota(&cx, HELPER_STACK_QUOTA);
 
     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);
     }
 }
--- a/js/src/vm/HelperThreads.h
+++ b/js/src/vm/HelperThreads.h
@@ -141,20 +141,24 @@ class GlobalHelperThreadState
 
     void lock();
     void unlock();
 #ifdef DEBUG
     bool isLockedByCurrentThread();
 #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
         // able to resume making progress.
         PAUSE
     };
 
     void wait(AutoLockHelperThreadState& locked, CondVar which,
@@ -305,17 +309,17 @@ class GlobalHelperThreadState
     JSScript* finishScriptDecodeTask(JSContext* cx, void* token);
     bool finishMultiScriptsDecodeTask(JSContext* cx, void* token, MutableHandle<ScriptVector> scripts);
     JSObject* finishModuleParseTask(JSContext* cx, void* token);
 
     bool hasActiveThreads(const AutoLockHelperThreadState&);
     void waitForAllThreads();
 
     template <typename T>
-    bool checkTaskThreadLimit(size_t maxThreads) const;
+    bool checkTaskThreadLimit(size_t maxThreads, bool isMaster = false) const;
 
   private:
 
     /*
      * Lock protecting all mutable shared state accessed by helper threads, and
      * used by all condition variables.
      */
     js::Mutex helperLock;