4
\$\begingroup\$

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();
  }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ The standard already has a way of doing this: std::async(). Threads and thread pools are way too low level. You should not be using them unless you have tested and defined arguments that you can validate for using them. Modern code should be using the modern interface to concurrency. std::async()/std::promise/std::future \$\endgroup\$ – Martin York Jun 22 '17 at 21:42
  • 2
    \$\begingroup\$ As far as I'm aware the standard doesn't tell how std::async has to be implemented and that in almost no implementations it works with a thread pool but just launches threads instead. \$\endgroup\$ – Sebastian Gabl Jun 23 '17 at 15:03
  • \$\begingroup\$ the standard doesn't tell how std::async has to be implemented: Of course. The standard never specifies how something should be implemented. It specifies the affects thus allowing implementations to evolve and become better. None of this changes my original comment. though. \$\endgroup\$ – Martin York Jun 23 '17 at 15:43
  • 1
    \$\begingroup\$ And none of your comments changes my original intention of having tasks be worked on a thread pool instead of just launching threads for every task ;) \$\endgroup\$ – Sebastian Gabl Jun 26 '17 at 8:20
4
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Interesting notes, thank you. I shall take a look at packaged_task and condition variables._num_work is there to count how many tasks are unfinished, I used it because the task is removed from the queue as soon as a thread starts working on it but the wait() method should wait until all tasks are done not just until the queue is empty. If there's a more elegant way to do it I'm not aware of it. \$\endgroup\$ – Sebastian Gabl Jun 22 '17 at 15:17
  • \$\begingroup\$ @SebastianGabl to wait until all work is done you can simply keep track of the number of threads doing something and wait for the queue to empty. Also you should probably prohibit new work from being submitted while you are waiting. \$\endgroup\$ – Emily L. Jun 22 '17 at 20:11
  • \$\begingroup\$ How would I check if a thread is doing something without having a variable that tells me in the current implementation? Or can this be combined with the condition variable? \$\endgroup\$ – Sebastian Gabl Jun 23 '17 at 15:06
  • \$\begingroup\$ @SebastianGabl You would need to use a variable (unless you join the threads like I did in my code) but I recommend that instead of counting the total amount of work including those "in flight", simply count the number of threads doing work. To me this feels more direct and avoids the misread of the code that I originally did. \$\endgroup\$ – Emily L. Jun 24 '17 at 21:44
  • \$\begingroup\$ Oh I see, I was a bit confused because I thought you meant to remove the variable entirely :) Thanks again for your input. \$\endgroup\$ – Sebastian Gabl Jun 26 '17 at 8:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.