Multi-threaded game loop

Question

This is a start on a multi-threaded game loop and I just wish to confirm my code is going in the correct direction, and if not, if there is any way it can be improved. I have good experience in game development and C++ in general, but limited experience when it comes to multi-threaded environments.

#include <iostream>
#include <string>
#include <thread>
#include <mutex>
#include <memory>
#include <chrono>
class Test
{
public:
 Test()
 {
 std::cout << "Test starting" << std::endl;
 start();
 }
 ~Test()
 {
 stop();
 std::cout << "Test finished" << std::endl;
 }
 inline void quit()
 {
 running = false;
 }
private:
 void start()
 {
 renderThread = std::unique_ptr<std::thread>(new std::thread(&Test::renderFunc, this));
 updateThread = std::unique_ptr<std::thread>(new std::thread(&Test::updateFunc, this));
 std::cout << "Threads Created" << std::endl;
 running = true;
 }
 void stop()
 {
 renderThread->join();
 updateThread->join();
 std::cout << "Threads joined" << std::endl;
 }
 void updateFunc()
 {
 while(running)
 {
 mutex.lock();
 std::cout << "update" << std::endl;
 mutex.unlock();
 }
 }
 void renderFunc()
 {
 while(running)
 {
 mutex.lock();
 std::cout << "render" << std::endl;
 mutex.unlock();
 }
 }
 volatile bool running;
 std::mutex mutex;
 std::unique_ptr<std::thread> renderThread;
 std::unique_ptr<std::thread> updateThread;
};
int main(int argc, char* argv[])
{
 std::unique_ptr<Test> test = std::unique_ptr<Test>(new Test());
 typedef std::chrono::high_resolution_clock Clock;
 typedef std::chrono::seconds Seconds;
 Clock::time_point t0, t1;
 Seconds seconds;
 t0 = Clock::now();
 do
 {
 t1 = Clock::now();
 seconds = std::chrono::duration_cast<Seconds>(t1 - t0);
 }while(seconds.count() <= 15);
 test->quit();
 return 0;
}

(The timer code is just there for testing the threaded code.)

• Jeff Mercado: Maybe I'm misunderstanding what's going on in your code but what's the point in having multiple threads if you're just going to have them wait on the same mutex? You've essentially made it single threaded since the threads can only execute sequentially. Right?
• const_ref: @Jeff Mercardo You may very well be right, like I said I have little to no experience with multithreaded programs, but most examples I have seen only used one mutex, but that may be down to the fact most were only reader/writer programs. Having two mutexs would probably be more beneficial but how would that ensure thread safety?
• Loki Astari: When I compile and run this I get an exception terminate called after throwing an instance of 'std::system_error' what(): Operation not permitted
• const_ref: Compiles and runs using Visual Studio 2012 for me. What compiler are you using?

Answer 1

Yes you have added enough explicit locks to make it safe (under normal situations).

But it is not exception safe and not written in C++ style. You are writing the code as if it was Java (maybe). Which is causing all sorts of problems.

Why do you need the start() and stop() functions? They provide no utility and they initialize members that should be initialized in the constructor. Also the threads are started before the object is fully initialized and thus you have a thread running in a function when the object is not initialized correctly (and thus will probably immediately exit).

 void start()
 {
 // Soon as you create a thread 
 // It starts running. So the next thing that is going to be
 // executed is the function `renderFunc()` which also depends
 // on member 'running' which has not been set.
 renderThread = std::unique_ptr<std::thread>(new std::thread(&Test::renderFunc, this));
 updateThread = std::unique_ptr<std::thread>(new std::thread(&Test::updateFunc, this));
 std::cout << "Threads Created" << std::endl;
 // You already have running threads that depend on this variable.
 // Yet this is the first time you even set it. Your code is already
 // broken.
 running = true;
 }

The next thing is the dynamic creation of the thread objects. Why. There is no need to dynamically create them. They should be automatic variables (ie non pointer members). The problem is that you need to be very careful with the initialization order.

class Test
{
 volatile bool running;
 std::mutex mutex;
 std::thread renderThread;
 std::thread updateThread;
 public:
 Test()
 : running(false) // Make sure this is first.
 , renderThread(&Test::renderFunc, this)
 , updateThread(&Test::updateFunc, this)
 {
 std::cout << "Test starting" << std::endl;
 }
 ~Test()
 {
 std::cout << "Test finished" << std::endl;
 updateThread.join();
 renderThread.join();
 }
 inline void quit()
 {
 running = false;
 }

Next thing is your locking and unlocking of the mutex.

 void updateFunc()
 {
 while(running)
 {
 mutex.lock();
 std::cout << "update" << std::endl;
 mutex.unlock();
 }
 }

This is not exception safe. Any situation were you do:

{
 lock(x);
 work();
 unlock(x);
}

Where lock/unlock is a combination that must be called as a pair. Then you should be using RAII. This is where you have an object where lock() is called in the constructor and unlock() is called in the destructor. This also makes the code exception safe because the destructor will always be called even if their are exceptions.

{
 LockObject locker(x); // calls lock(x)
 work();
} // destructor of locker will call unlock(x)

For this simple situation there is no need to create your own type. There is one defined in the standard scoped locked.

 void updateFunc()
 {
 while(running)
 {
 std::lock_guard<std::mutex> locker(mutex);
 std::cout << "update" << std::endl;
 }
 }

Stop using dynamic allocation when you do not need it:

 std::unique_ptr<Test> test = std::unique_ptr<Test>(new Test());
 // Just do this:
 Test test;

• Rafael Plugge: While Loki Astari said that you don't need dynamic memory allocation, one efficiency thing to note is that std::unique_ptr<Test> = std::unique_ptr<Test>(new Test()); is inefficient due to the code needing to make 2 allocations (new Test() creates a raw pointer that is passed to the unique_ptr's constructor) and should be changed to std::unique_ptr<Test> = std::make_unique<Test>();.
• Loki Astari: @RafaelPlugge: I agree that it should be changed if you have C++17 (for consistency with std::make_shared()) but implying its more efficient is just wrong. Go look at an implementation.