Thread pool implementation

Question

As a kind of test of some newer features of C++ I implemented a thread pool. I'm aware that there are probably libraries out there that already have thread pools implemented but this was just as a small test for myself. I'm looking for opinions on my implementation as well as suggestions on how to improve it if possible.

The current intention of it is just to have a simple way to add functions that should be executed to the pool without having to worry about the overhead of starting a new thread each time.

Here's the class:

typedef std::function<void(void)> work_function;

class ThreadPool {
  private:
    bool _running;
    std::queue<work_function> _work_queue;
    std::atomic<size_t> _num_work;
    std::mutex _mutex;
    std::vector<std::thread> _threads;
  public:
    ThreadPool(size_t num_threads = std::thread::hardware_concurrency());
    virtual ~ThreadPool();
    void addWork(work_function work);
    void clearWork();
    void wait();
};

An here's the implementation:

ThreadPool::ThreadPool(size_t num_threads) :
    _running(true), _num_work(0) {
  auto thread_loop = [&](size_t id) {
    while (_running) {
      _mutex.lock();
      if (!_work_queue.empty()) {
        auto work = _work_queue.front();
        _work_queue.pop();
        _mutex.unlock();
        work();
        _num_work--;
      } else {
        _mutex.unlock();
        std::this_thread::yield();
      }
    }
  };
  _threads.reserve(num_threads);
  for (size_t i = 0; i < num_threads; i++) {
    _threads.push_back(std::thread(thread_loop, i));
  }
}

ThreadPool::~ThreadPool() {
  _running = false;
  for (std::thread& t : _threads) {
    t.join();
  }
}

void ThreadPool::addWork(work_function work) {
  _mutex.lock();
  _work_queue.push(work);
  _num_work++;
  _mutex.unlock();
}

void ThreadPool::clearWork() {
  std::queue<work_function> empty;
  _mutex.lock();
  _num_work -= _work_queue.size();
  std::swap(_work_queue, empty);
  _mutex.unlock();
}

void ThreadPool::wait() {
  while (_num_work.load() > 0) {
    std::this_thread::yield();
  }
}

Answer 1

Please excuse my brevity, I'm typing on a phone.

Naming

The rules regarding use of leading underscore in identifiers are a bit complex (https://stackoverflow.com/a/228797/2498188). Although you do not have undefined behavior, I do generally recommend that one avoids leading underscore on identifiers as not everyone knows the rules that well.

Consider using std::packaged_task

While your code can be used with any freestanding function without arguments and no return type, by using packaged task you can use any function with bind and you can get a std::future for the return value or synchronisation of the result. It would make your code more generally usable.

Synchronisation

The value _num_work is never used outside of a mutex look and does thus not have to be atomic, in fact you do not need it at all as you can just use the size() method on the queue.

Edit: I see now that it counts the number of tasks including the ones "in flight", I don't really see how this is useful if you don't provide a method for separately querying the number in flight and the number queued.

Effectivity

Note that you do call yield in your thread loop, this doesn't make the thread wait, it only informs the OS that some other thread can run in its place. If there is no other thread to run, it will run again at the top of your loop essentially pegging all of your CPUs at 100% even without any tasks in the queue. You need to use a std::condition_variable to block your worker threads when there is no work to do, and wake them when work arrives.

Desired methods

You're missing don't methods that are typically desirable, like for example the ability to terminate the thread pool with and without waiting for all jobs to complete.

Finally as a reference I offer up my pool in a previous question: Thread pool worker implementation