3
\$\begingroup\$

This is a nice workqueue class template to keep the gui fluid, while data is being written into a database or into some file/network...

#include <cassert>

#include <condition_variable>

#include <mutex>

#include <thread>

#include <functional>

template <template <typename> class Function = std::function>
class workqueue
{
  using f_t = Function<void()>;

  std::vector<f_t> queue_;

  std::mutex mutex_;

  std::condition_variable cv_;

  std::atomic_bool quit_flag_{false};

  std::thread thread_;

public:
  explicit workqueue() :
    thread_([this]()
      {
        bool qf{};

        while (!qf)
        {
          std::unique_lock<decltype(mutex_)> l(mutex_);

          while (!(qf = quit_flag_.load(std::memory_order_relaxed)) &&
            queue_.empty())
          {
            cv_.wait(l);
          }

          decltype(queue_) q(std::move(queue_));

          l.unlock();

          for (auto& f: q)
          {
            f();
          }
        }
      }
    )
  {
  }

  ~workqueue()
  {
    assert(!quit_flag_.load(std::memory_order_relaxed));
    quit_flag_.store(true, std::memory_order_relaxed);

    cv_.notify_one();

    assert(thread_.joinable());
    thread_.join();
  }

  template <typename F>
  void exec(F&& f)
  {
    {
      std::lock_guard<decltype(mutex_)> l(mutex_);

      queue_.emplace_back(std::forward<F>(f));
    }

    cv_.notify_one();
  }
};
\$\endgroup\$
3
\$\begingroup\$
  template <typename F>
  void exec(F&& f)
  {
    {
      std::lock_guard<decltype(mutex_)> l(mutex_);

      queue_.emplace_back(std::forward<F>(f));
    }

    cv_.notify_one();
  }

This is a bit weird. emplace_back allows in-place construction of an object. In this case, it will call the move constructor, copy constructor or a constructor that takes one argument.

If allowing in-place construction of f_ts is a necessary feature, it would be better to use a parameter pack and perfect forwarding (so a constructor with any number of arguments can be used).

However, I suspect that isn't the intention at all, and we just want to copy / move a function object. So we should be using push_back, not emplace_back.

There's also no reason for this to be a template function. We want an f_t. The user also has to pass in a compatible Function type, and call exec with a compatible type so they already have to be aware of exactly what f_t is to use the class. So it would be much cleaner to make f_t public, take a function object by value (as a sink argument), and move it into place:

void exec(f_t f)
{
  ...
    queue_.push_back(std::move(f));
  ...
}

This makes the intent instantly obvious, and avoids any complications and downsides involved with universal references.

If performance is a serious concern, then one overload taking an f_t const& and one taking an f_t&& would be a reasonable alternative.

\$\endgroup\$
  • 1
    \$\begingroup\$ usually the argument will be a lambda instance, that will be forwarded (&& or const&) into an f_t. \$\endgroup\$ – user1095108 Aug 26 '18 at 21:09
3
\$\begingroup\$
  1. You should use a templated concurrent queue that's not specific to your work offloading mechanism, instead of implementing one yourself (and not even encapsulating it in its own class). A popular such queue is Moody Camel's. If you visit that repository's page, you'll see a shorter producer-consumer example (even if it's not exactly what you're implementing).

  2. The interface for enqueueing work should support arguments following the function, i.e. a variadic-template method which wraps the arguments in a lambda.

\$\endgroup\$
  • 1
    \$\begingroup\$ if you want to store arguments to a function, you can do so in the work lambda itself, i.e. [a, b, c](){f(a, b, c);}. I don't really know what you mean. But, yeah, a lock-free queue might be better, but ... these usually come with restrictions and oddities. \$\endgroup\$ – user1095108 Aug 26 '18 at 15:24
  • \$\begingroup\$ 1. You should offer the users the convenience of not having to wrap up things in their own lambda. 2. The Moody Camel repository also has a blocking (= locking) queue if you'd rather use that. \$\endgroup\$ – einpoklum Aug 26 '18 at 16:51
  • \$\begingroup\$ lock-free is always good, but I'd rather write my own container, not depend on something external, so... it's just a simple workqueue for your everyday convenience. And allowing for a delayed apply of arguments, ... I don't really need that. \$\endgroup\$ – user1095108 Aug 26 '18 at 20:58
  • \$\begingroup\$ @user1095108: This is a wrong approach which you should forego. Unless you have an important reason to implement your own container, there cons far outweight the pros. It's good to depend on external code. You depend on things that are , well-tested, well-implemented, and inspire trust. That also makes it easier for you to modularize/componentize and better focus your development effort. \$\endgroup\$ – einpoklum Aug 26 '18 at 21:31
  • 1
    \$\begingroup\$ that's exactly my point, I don't want to debug other people's code, I'd rather debug my own. STL is well-tested, everything else ... not always and lockfree code is notorious for being difficult to implement correctly. \$\endgroup\$ – user1095108 Aug 26 '18 at 22:24

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.