Hook and fix up EmulationWorker -> threading works, pick'n'pile is happy.

.vscode/settings.json

@@ -52,6 +52,7 @@
         "new": "cpp",
         "typeinfo": "cpp",
         "__mutex_base": "cpp",
-        "mutex": "cpp"
+        "mutex": "cpp",
+        "condition_variable": "cpp"
     }
 }

src/common/AudioQueue.cxx

@@ -101,7 +101,7 @@ Int16* AudioQueue::enqueue(Int16* fragment)
   if (mySize < capacity) mySize++;
   else {
     myNextFragment = (myNextFragment + 1) % capacity;
-    (cerr << "audio buffer overflow\n").flush();
+    //(cerr << "audio buffer overflow\n").flush();
   }
 
   return newFragment;

src/emucore/EmulationWorker.cxx

@@ -24,7 +24,7 @@
 using namespace std::chrono;
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-EmulationWorker::EmulationWorker(TIA& tia) : myTia(tia)
+EmulationWorker::EmulationWorker() : myPendingSignal(Signal::none), myState(State::initializing)
 {
   std::mutex mutex;
   std::unique_lock<std::mutex> lock(mutex);
@@ -34,7 +34,7 @@ EmulationWorker::EmulationWorker(TIA& tia) : myTia(tia)
     &EmulationWorker::threadMain, this, &threadInitialized, &mutex
   );
 
-  // Wait until the thread has acquired myMutex and moved on
+  // Wait until the thread has acquired myWakeupMutex and moved on
   while (myState == State::initializing) threadInitialized.wait(lock);
 }
 
@@ -43,11 +43,11 @@ EmulationWorker::~EmulationWorker()
 {
   // This has to run in a block in order to release the mutex before joining
   {
-    std::unique_lock<std::mutex> lock(myMutex);
+    std::unique_lock<std::mutex> lock(myWakeupMutex);
 
     if (myState != State::exception) {
-      myPendingSignal = Signal::quit;
+      signalQuit();
-      mySignalCondition.notify_one();
+      myWakeupCondition.notify_one();
     }
   }
 
@@ -69,23 +69,22 @@ void EmulationWorker::handlePossibleException()
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-void EmulationWorker::start(uInt32 cyclesPerSecond, uInt32 maxCycles, uInt32 minCycles, DispatchResult* dispatchResult)
+void EmulationWorker::start(uInt32 cyclesPerSecond, uInt32 maxCycles, uInt32 minCycles, DispatchResult* dispatchResult, TIA* tia)
 {
-  // Optimization: run this in a block to unlock the mutex before notifying the thread;
+  waitForSignalClear();
-  // otherwise, the thread would immediatelly block again until we have released the mutex,
+
-  // sacrificing a timeslice
-  {
   // Aquire the mutex -> wait until the thread is suspended
-    std::unique_lock<std::mutex> lock(myMutex);
+  std::unique_lock<std::mutex> lock(myWakeupMutex);
 
   // Pass on possible exceptions
   handlePossibleException();
 
   // NB: The thread does not suspend execution in State::initialized
   if (myState != State::waitingForResume)
-      throw runtime_error("start called on running or dead worker");
+    fatal("start called on running or dead worker");
 
   // Store the parameters for emulation
+  myTia = tia;
   myCyclesPerSecond = cyclesPerSecond;
   myMaxCycles = maxCycles;
   myMinCycles = minCycles;
@@ -93,51 +92,50 @@ void EmulationWorker::start(uInt32 cyclesPerSecond, uInt32 maxCycles, uInt32 min
 
   // Set the signal...
   myPendingSignal = Signal::resume;
-  }
 
   // ... and wakeup the thread
-  mySignalCondition.notify_one();
+  myWakeupCondition.notify_one();
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-void EmulationWorker::stop()
+uInt64 EmulationWorker::stop()
 {
-  // See EmulationWorker::start for the gory details
+  waitForSignalClear();
-  {
+
-    std::unique_lock<std::mutex> lock(myMutex);
+  std::unique_lock<std::mutex> lock(myWakeupMutex);
   handlePossibleException();
 
   // If the worker has stopped on its own, we return
-    if (myState == State::waitingForResume) return;
+  if (myState == State::waitingForResume) return 0;
 
   // NB: The thread does not suspend execution in State::initialized or State::running
   if (myState != State::waitingForStop)
-      throw runtime_error("stop called on a dead worker");
+    fatal("stop called on a dead worker");
 
   myPendingSignal = Signal::stop;
-  }
 
-  mySignalCondition.notify_one();
+  myWakeupCondition.notify_one();
+
+  return myTotalCycles;
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 void EmulationWorker::threadMain(std::condition_variable* initializedCondition, std::mutex* initializationMutex)
 {
-  std::unique_lock<std::mutex> lock(myMutex);
+  std::unique_lock<std::mutex> lock(myWakeupMutex);
 
   try {
-    // Optimization: release the lock before notifying our parent -> saves a timeslice
     {
       // Wait until our parent releases the lock and sleeps
       std::lock_guard<std::mutex> guard(*initializationMutex);
 
       // Update the state...
       myState = State::initialized;
-    }
 
       // ... and wake up our parent to notifiy that we have initialized. From this point, the
       // parent can safely assume that we are running while the mutex is locked.
       initializedCondition->notify_one();
+    }
 
     while (myPendingSignal != Signal::quit) handleWakeup(lock);
   }
@@ -153,7 +151,7 @@ void EmulationWorker::handleWakeup(std::unique_lock<std::mutex>& lock)
   switch (myState) {
     case State::initialized:
       myState = State::waitingForResume;
-      mySignalCondition.wait(lock);
+      myWakeupCondition.wait(lock);
       break;
 
     case State::waitingForResume:
@@ -165,7 +163,7 @@ void EmulationWorker::handleWakeup(std::unique_lock<std::mutex>& lock)
       break;
 
     default:
-      throw runtime_error("wakeup in invalid worker state");
+      fatal("wakeup in invalid worker state");
   }
 }
 
@@ -174,20 +172,21 @@ void EmulationWorker::handleWakeupFromWaitingForResume(std::unique_lock<std::mut
 {
   switch (myPendingSignal) {
     case Signal::resume:
-      myPendingSignal = Signal::none;
+      clearSignal();
       myVirtualTime = high_resolution_clock::now();
+      myTotalCycles = 0;
       dispatchEmulation(lock);
       break;
 
     case Signal::none:
-      mySignalCondition.wait(lock);
+      myWakeupCondition.wait(lock);
       break;
 
     case Signal::quit:
       break;
 
     default:
-      throw runtime_error("invalid signal while waiting for resume");
+      fatal("invalid signal while waiting for resume");
   }
 }
 
@@ -197,16 +196,16 @@ void EmulationWorker::handleWakeupFromWaitingForStop(std::unique_lock<std::mutex
   switch (myPendingSignal) {
     case Signal::stop:
       myState = State::waitingForResume;
-      myPendingSignal = Signal::none;
+      clearSignal();
 
-      mySignalCondition.wait(lock);
+      myWakeupCondition.wait(lock);
       break;
 
     case Signal::none:
       if (myVirtualTime <= high_resolution_clock::now())
         dispatchEmulation(lock);
       else
-        mySignalCondition.wait_until(lock, myVirtualTime);
+        myWakeupCondition.wait_until(lock, myVirtualTime);
 
       break;
 
@@ -214,7 +213,7 @@ void EmulationWorker::handleWakeupFromWaitingForStop(std::unique_lock<std::mutex
       break;
 
     default:
-      throw runtime_error("invalid signal while waiting for stop");
+      fatal("invalid signal while waiting for stop");
   }
 }
 
@@ -226,29 +225,62 @@ void EmulationWorker::dispatchEmulation(std::unique_lock<std::mutex>& lock)
   uInt64 totalCycles = 0;
 
   do {
-    myTia.update(*myDispatchResult, totalCycles > 0 ? myMinCycles - totalCycles : myMaxCycles);
+    myTia->update(*myDispatchResult, totalCycles > 0 ? myMinCycles - totalCycles : myMaxCycles);
+    totalCycles += myDispatchResult->getCycles();
   } while (totalCycles < myMinCycles && myDispatchResult->getStatus() == DispatchResult::Status::ok);
 
+  myTotalCycles += totalCycles;
+
+  bool continueEmulating = false;
+
   if (myDispatchResult->getStatus() == DispatchResult::Status::ok) {
     // If emulation finished successfully, we can go for another round
     duration<double> timesliceSeconds(static_cast<double>(totalCycles) / static_cast<double>(myCyclesPerSecond));
     myVirtualTime += duration_cast<high_resolution_clock::duration>(timesliceSeconds);
 
     myState = State::waitingForStop;
-
+    continueEmulating = myVirtualTime > high_resolution_clock::now();
-    if (myVirtualTime > high_resolution_clock::now())
-      // If we can keep up with the emulation, we sleep
-      mySignalCondition.wait_until(lock, myVirtualTime);
-    else {
-      // If we are already lagging behind, we briefly relinquish control over the mutex
-      // and yield to scheduler, to make sure that the main thread has a chance to stop us
-      lock.release();
-      std::this_thread::yield();
-      lock.lock();
   }
+
+  if (continueEmulating) {
+    myState = State::waitingForStop;
+    myWakeupCondition.wait_until(lock, myVirtualTime);
   } else {
-    // If execution trapped, we stop immediatelly.
     myState = State::waitingForResume;
-    mySignalCondition.wait(lock);
+    myWakeupCondition.wait(lock);
   }
 }
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+void EmulationWorker::clearSignal()
+{
+  std::unique_lock<std::mutex> lock(mySignalChangeMutex);
+  myPendingSignal = Signal::none;
+
+  mySignalChangeCondition.notify_one();
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+void EmulationWorker::signalQuit()
+{
+  std::unique_lock<std::mutex> lock(mySignalChangeMutex);
+  myPendingSignal = Signal::quit;
+
+  mySignalChangeCondition.notify_one();
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+void EmulationWorker::waitForSignalClear()
+{
+  std::unique_lock<std::mutex> lock(mySignalChangeMutex);
+
+  while (myPendingSignal != Signal::none && myPendingSignal != Signal::quit)
+    mySignalChangeCondition.wait(lock);
+}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+void EmulationWorker::fatal(string message)
+{
+  (cerr << "FATAL in emulation worker: " << message << std::endl).flush();
+  throw runtime_error(message);
+}

src/emucore/EmulationWorker.hxx

@@ -43,13 +43,13 @@ class EmulationWorker
 
   public:
 
-    EmulationWorker(TIA& tia);
+    EmulationWorker();
 
     ~EmulationWorker();
 
-    void start(uInt32 cyclesPerSecond, uInt32 maxCycles, uInt32 minCycles, DispatchResult* dispatchResult);
+    void start(uInt32 cyclesPerSecond, uInt32 maxCycles, uInt32 minCycles, DispatchResult* dispatchResult, TIA* tia);
 
-    void stop();
+    uInt64 stop();
 
   private:
 
@@ -58,6 +58,12 @@ class EmulationWorker
     // Passing references into a thread is awkward and requires std::ref -> use pointers here
     void threadMain(std::condition_variable* initializedCondition, std::mutex* initializationMutex);
 
+    void clearSignal();
+
+    void signalQuit();
+
+    void waitForSignalClear();
+
     void handleWakeup(std::unique_lock<std::mutex>& lock);
 
     void handleWakeupFromWaitingForResume(std::unique_lock<std::mutex>& lock);
@@ -66,14 +72,20 @@ class EmulationWorker
 
     void dispatchEmulation(std::unique_lock<std::mutex>& lock);
 
+    void fatal(string message);
+
   private:
 
-    TIA& myTia;
+    TIA* myTia;
 
     std::thread myThread;
 
-    std::condition_variable mySignalCondition;
+    std::condition_variable myWakeupCondition;
-    std::mutex myMutex;
+    std::mutex myWakeupMutex;
+
+    std::condition_variable mySignalChangeCondition;
+    std::mutex mySignalChangeMutex;
+
     std::exception_ptr myPendingException;
     Signal myPendingSignal;
     // The initial access to myState is not synchronized -> make this atomic
@@ -84,7 +96,18 @@ class EmulationWorker
     uInt32 myMinCycles;
     DispatchResult* myDispatchResult;
 
+    uInt64 myTotalCycles;
     std::chrono::time_point<std::chrono::high_resolution_clock> myVirtualTime;
+
+  private:
+
+    EmulationWorker(const EmulationWorker&) = delete;
+
+    EmulationWorker(EmulationWorker&&) = delete;
+
+    EmulationWorker& operator=(const EmulationWorker&) = delete;
+
+    EmulationWorker& operator=(EmulationWorker&&) = delete;
 };
 
 #endif // EMULATION_WORKER_HXX

src/emucore/OSystem.cxx

@@ -54,6 +54,7 @@
 #include "Version.hxx"
 #include "TIA.hxx"
 #include "DispatchResult.hxx"
+#include "EmulationWorker.hxx"
 
 #include "OSystem.hxx"
 
@@ -637,27 +638,35 @@ float OSystem::frameRate() const
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-double OSystem::dispatchEmulation(uInt32 cyclesPerSecond)
+double OSystem::dispatchEmulation(EmulationWorker& emulationWorker)
 {
   if (!myConsole) return 0.;
 
-  Int64 totalCycles = 0;
+  TIA& tia(myConsole->tia());
-  const Int64 minCycles = myConsole->emulationTiming().minCyclesPerTimeslice();
+  EmulationTiming& timing(myConsole->emulationTiming());
-  const Int64 maxCycles = myConsole->emulationTiming().maxCyclesPerTimeslice();
   DispatchResult dispatchResult;
 
-  do {
+  bool framePending = tia.newFramePending();
-    myConsole->tia().update(dispatchResult, totalCycles > 0 ? minCycles - totalCycles : maxCycles);
+  if (framePending) tia.renderToFrameBuffer();
 
-    totalCycles += dispatchResult.getCycles();
+  emulationWorker.start(
-  } while (totalCycles < minCycles && dispatchResult.getStatus() == DispatchResult::Status::ok);
+    timing.cyclesPerSecond(),
+    timing.maxCyclesPerTimeslice(),
+    timing.minCyclesPerTimeslice(),
+    &dispatchResult,
+    &tia
+  );
+
+  if (framePending) myFrameBuffer->updateInEmulationMode();
+
+  uInt64 totalCycles = emulationWorker.stop();
 
   if (dispatchResult.getStatus() == DispatchResult::Status::debugger) myDebugger->start();
 
   if (dispatchResult.getStatus() == DispatchResult::Status::ok && myEventHandler->frying())
     myConsole->fry();
 
-  return static_cast<double>(totalCycles) / static_cast<double>(cyclesPerSecond);
+  return static_cast<double>(totalCycles) / static_cast<double>(timing.cyclesPerSecond());
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@@ -666,6 +675,7 @@ void OSystem::mainLoop()
   // Sleep-based wait: good for CPU, bad for graphical sync
   bool busyWait = mySettings->getString("timing") != "sleep";
   time_point<high_resolution_clock> virtualTime = high_resolution_clock::now();
+  EmulationWorker emulationWorker;
 
   for(;;)
   {
@@ -674,14 +684,9 @@ void OSystem::mainLoop()
 
     double timesliceSeconds;
 
-    if (myEventHandler->state() == EventHandlerState::EMULATION) {
+    if (myEventHandler->state() == EventHandlerState::EMULATION)
-      timesliceSeconds = dispatchEmulation(myConsole ? myConsole->emulationTiming().cyclesPerSecond() : 1);
+      timesliceSeconds = dispatchEmulation(emulationWorker);
-
+    else {
-      if (myConsole && myConsole->tia().newFramePending()) {
-        myConsole->tia().renderToFrameBuffer();
-        myFrameBuffer->updateInEmulationMode();
-      }
-    } else {
       timesliceSeconds = 1. / 30.;
       myFrameBuffer->update();
     }

src/emucore/OSystem.hxx

@@ -38,6 +38,7 @@ class Settings;
 class Sound;
 class StateManager;
 class VideoDialog;
+class EmulationWorker;
 
 #include "FSNode.hxx"
 #include "FrameBufferConstants.hxx"
@@ -563,7 +564,7 @@ class OSystem
     void validatePath(string& path, const string& setting,
                       const string& defaultpath);
 
-    double dispatchEmulation(uInt32 cyclesPerSecond);
+    double dispatchEmulation(EmulationWorker& emulationWorker);
 
     // Following constructors and assignment operators not supported
     OSystem(const OSystem&) = delete;