patterncppMinor
Concurrent FIFO in C++11
Viewed 0 times
concurrentfifostackoverflow
Problem
I have implemented a simple FIFO that can optionally be used by either a single thread or way to pass data between threads. The class is templated with arguments for the types the queue will contain and also the number of elements in the queue (queue data is stored in an
and for use in a multithreaded context:
It has these methods:
Specifically for the multi-threaded version:
After making it I found this article and this article, which impleme
std::array). For single thread use, it is instantiated by:ST_FIFOand for use in a multithreaded context:
MT_FIFOIt has these methods:
push(with and without move semantics): put one item one the buffer. Always succeeds--if buffer is full, oldest element is popped.
try_push: put one item into the buffer. If buffer is full, push fails.
multi_push: push a number of items onto buffer. Mostly useful for multi-threaded, where all items are pushed under same lock. Always succeeds.
try_multi_push: same as multi_push, but fails if buffer is full.
pop: pops one item off buffer. In single threaded version, returns immediately if buffer is empty. In multi-threaded version, this method blocks (using a condition variable) until data becomes available.
multi_pop: pops a given number of items out of the buffer. If buffer contains fewer items than number requested, returns all items in buffer. In multi-threaded version, blocks until one item becomes available. Returns number of items popped from buffer.
peek: get an item from buffer without removing it. Can be any item in the buffer, but must be less than the number of items in the buffer.
size: returns number of items in buffer.
is_empty: returns true if buffer is empty.
Specifically for the multi-threaded version:
try_pop: pops an item from buffer. If buffer is empty returns false immediately.
wait_off: if a thread is waiting for an item to be popped, this method can be called from another thread to terminate the waiting (useful to unblock the thread when exiting.
wait_on: turns waiting back on.
After making it I found this article and this article, which impleme
Solution
I'm in a bit of a hurry so I'll only touch on the big high level stuff that I noticed.
Abuse of SFINAE
I think that the way you are designing two different use cases into one template class and switching between them is an abuse of SFINAE.
The most obvious indication of this is the fact that you define two aliases
I understand that this is a way to avoid code duplication but I do not approve of it. The added clutter and the extra overhead of a mutex and atomic variables etc on the single threaded queue is also less than ideal.
What I would do instead is to make a shared, hidden base class which holds the shared functionality. Something like this perhaps:
In this way you can share the common implementation of methods between the two variants without abusing SFINAE and without the single threaded version having the size overhead of the mutex etc. One might be worried that this is a violation of the Liskov Susbstitution Principle but as the base class is hidden in a "detail" namespace it's a clear hint not to use it and it cannot be directly instantiated. Yes, the user may accept a pointer or reference to a
Abuse of SFINAE
I think that the way you are designing two different use cases into one template class and switching between them is an abuse of SFINAE.
The most obvious indication of this is the fact that you define two aliases
ST_FIFO and MT_FIFO to act as if they were two separate templates. I understand that this is a way to avoid code duplication but I do not approve of it. The added clutter and the extra overhead of a mutex and atomic variables etc on the single threaded queue is also less than ideal.
What I would do instead is to make a shared, hidden base class which holds the shared functionality. Something like this perhaps:
namespace detail{
class queue_base{
public:
push(); // etc
protected:
queue_base(); // Make CTOR protected to prevent direct instantiation
};
}
class queue : public detail::queue_base{};
class concurrent_queue : public detail::queue_base{};In this way you can share the common implementation of methods between the two variants without abusing SFINAE and without the single threaded version having the size overhead of the mutex etc. One might be worried that this is a violation of the Liskov Susbstitution Principle but as the base class is hidden in a "detail" namespace it's a clear hint not to use it and it cannot be directly instantiated. Yes, the user may accept a pointer or reference to a
queue_base object but they are going through hoops to do it and one might argue that they both provide the same base contract so they are Liskov substitutable. I find it acceptable, although not ideal. The other option is to let queue and concurrent_queue have a member of type detail::queue_base and forward the method calls but I don't find this forwarding very appealing either.Code Snippets
namespace detail{
class queue_base{
public:
push(); // etc
protected:
queue_base(); // Make CTOR protected to prevent direct instantiation
};
}
class queue : public detail::queue_base{};
class concurrent_queue : public detail::queue_base{};Context
StackExchange Code Review Q#162658, answer score: 9
Revisions (0)
No revisions yet.