Backed out changeset fc437d64c831 (bug 1486556) for breaking coordination between record/replay system and helper threads.
authorBrian Hackett <bhackett1024@gmail.com>
Thu, 30 Aug 2018 04:24:58 -1000
changeset 434082 54c5a8af9831a0a36ed39c6f4581101acdcae79b
parent 434081 4ca69733bd89adf297f823f272739b4c742dd7e0
child 434083 c317d6b31d9c951c9357fb9a49d2686a3efcfe2f
child 434099 345269b39c6c2d08edebb8225e55c690dc019537
push id34537
push usernbeleuzu@mozilla.com
push dateThu, 30 Aug 2018 16:59:14 +0000
treeherdermozilla-central@c317d6b31d9c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1486556
milestone63.0a1
backs outfc437d64c83157d3539c8b337d4048e00cd373ea
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
Backed out changeset fc437d64c831 (bug 1486556) for breaking coordination between record/replay system and helper threads.
js/src/vm/HelperThreads.cpp
mfbt/RecordReplay.h
toolkit/recordreplay/Thread.cpp
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -2415,41 +2415,41 @@ HelperThread::threadLoop()
     {
         AutoEnterOOMUnsafeRegion oomUnsafe;
         if (!cx.init(ContextKind::HelperThread))
             oomUnsafe.crash("HelperThread cx.init()");
     }
     cx.setHelperThread(this);
     JS_SetNativeStackQuota(&cx, HELPER_STACK_QUOTA);
 
-    if (mozilla::recordreplay::IsRecordingOrReplaying())
-        mozilla::recordreplay::NotifyUnrecordedWait(WakeupAll);
-
     while (!terminate) {
         MOZ_ASSERT(idle());
 
-        if (mozilla::recordreplay::IsRecordingOrReplaying()) {
-            // Unlock the helper thread state lock before potentially
-            // blocking while the main thread waits for all threads to
-            // become idle. Otherwise we would need to see if we need to
-            // block at every point where a helper thread acquires the
-            // helper thread state lock.
-            AutoUnlockHelperThreadState unlock(lock);
-            mozilla::recordreplay::MaybeWaitForCheckpointSave();
-        }
-
         // 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.
 
         const TaskSpec* task = findHighestPriorityTask(lock);
         if (!task) {
             AUTO_PROFILER_LABEL("HelperThread::threadLoop::wait", IDLE);
+            if (mozilla::recordreplay::IsRecordingOrReplaying()) {
+                // Unlock the helper thread state lock before potentially
+                // blocking while the main thread waits for all threads to
+                // become idle. Otherwise we would need to see if we need to
+                // block at every point where a helper thread acquires the
+                // helper thread state lock.
+                {
+                    AutoUnlockHelperThreadState unlock(lock);
+                    mozilla::recordreplay::MaybeWaitForCheckpointSave();
+                }
+                mozilla::recordreplay::NotifyUnrecordedWait(WakeupAll);
+            }
+
             HelperThreadState().wait(lock, GlobalHelperThreadState::PRODUCER);
             continue;
         }
 
         js::oom::SetThreadType(task->type);
         (this->*(task->handleWorkload))(lock);
         js::oom::SetThreadType(js::THREAD_TYPE_NONE);
     }
--- a/mfbt/RecordReplay.h
+++ b/mfbt/RecordReplay.h
@@ -263,17 +263,18 @@ static inline bool HasDivergedFromRecord
 // --- they wait on a recorded lock (one which was created when events were not
 // passed through) or an associated cvar --- this is handled automatically.
 //
 // Threads which block indefinitely on unrecorded resources must call
 // NotifyUnrecordedWait first.
 //
 // The callback passed to NotifyUnrecordedWait will be invoked at most once
 // by the main thread whenever the main thread is waiting for other threads to
-// become idle.
+// become idle, and at most once after the call to NotifyUnrecordedWait if the
+// main thread is already waiting for other threads to become idle.
 //
 // The callback should poke the thread so that it is no longer blocked on the
 // resource. The thread must call MaybeWaitForCheckpointSave before blocking
 // again.
 MFBT_API void NotifyUnrecordedWait(const std::function<void()>& aCallback);
 MFBT_API void MaybeWaitForCheckpointSave();
 
 // API for debugging inconsistent behavior between recording and replay.
--- a/toolkit/recordreplay/Thread.cpp
+++ b/toolkit/recordreplay/Thread.cpp
@@ -473,16 +473,25 @@ Thread::ResumeIdleThreads()
     Notify(i);
   }
 }
 
 void
 Thread::NotifyUnrecordedWait(const std::function<void()>& aCallback)
 {
   MonitorAutoLock lock(*gMonitor);
+  if (mUnrecordedWaitCallback) {
+    // Per the documentation for NotifyUnrecordedWait, we need to call the
+    // routine after a notify, even if the routine has been called already
+    // since the main thread started to wait for idle replay threads.
+    mUnrecordedWaitNotified = false;
+  } else {
+    MOZ_RELEASE_ASSERT(!mUnrecordedWaitNotified);
+  }
+
   mUnrecordedWaitCallback = aCallback;
 
   // The main thread might be able to make progress now by calling the routine
   // if it is waiting for idle replay threads.
   if (gThreadsShouldIdle) {
     Notify(MainThreadId);
   }
 }