Thread safe implementation of circular buffer

Question

Circular_buffer from boost library is not thread-safe. So I wrapped boost::circular_buffer object in a class as shown below. Mutual exclusion between the threads is achieved (I think) by using conditional variables, a mutex and a lock acquisition/release. Is this implementation thread safe?

#include <boost/thread/condition.hpp>
#include <boost/thread/mutex.hpp>
#include <boost/thread/thread.hpp>
#include <boost/circular_buffer.hpp>

// Thread safe circular buffer 
template <typename T>
class circ_buffer : private boost::noncopyable
{
public:
    typedef boost::mutex::scoped_lock lock;
    circ_buffer() {}
    circ_buffer(int n) {cb.set_capacity(n);}
    void send (T imdata) {
        lock lk(monitor);
        cb.push_back(imdata);
        buffer_not_empty.notify_one();
    }
    T receive() {
        lock lk(monitor);
        while (cb.empty())
            buffer_not_empty.wait(lk);
        T imdata = cb.front();
        cb.pop_front();
        return imdata;
    }
    void clear() {
        lock lk(monitor);
        cb.clear();
    }
    int size() {
        lock lk(monitor);
        return cb.size();
    }
    void set_capacity(int capacity) {
        lock lk(monitor);
        cb.set_capacity(capacity);
    }
private:
    boost::condition buffer_not_empty;
    boost::mutex monitor;
    boost::circular_buffer<T> cb;
};

Edit This is now a template class, which accepts an object of any type (not just cv::Mat object).

This looks like it would fit better on [CodeReview](http://codereview.stackexchange.com/). — Jerry Coffin, Mar 16 '12 at 19:56
Forgive my dumb question, but where does one need a thread safe circular buffer? At all the points where I have ever used a circular buffer, it would have been a bad mistake to access it from multiple threads like this. So just out of curiosity, what is your use case for this? — LiKao, Mar 16 '12 at 20:16
@LiKao I use it to grab frames from network cameras into MATLAB, see my previous post http://stackoverflow.com/questions/9472880/how-to-implement-a-circular-buffer-of-cvmat-objects-opencv. How would you approach this? — Alexey, Mar 16 '12 at 20:25
@LiKao : You'd use such a thing to implement a *producer-consumer queue* (http://en.wikipedia.org/wiki/Producer-consumer_problem). Such queues can be used between stages in a *multi-threaded pipeline*. — Emile Cormier, Mar 16 '12 at 23:47
@Alex if it is not a hassle would be easy to provide also a simple example how to use this class for grabbing and showing frames with opencv? — ttsesm, Dec 17 '15 at 16:15
You might want to use `synchronized` https://codereview.stackexchange.com/questions/15632/force-locking-for-thread-safety/15742#15742 — Gabriel, Jul 31 '20 at 11:27

score 18 · Accepted Answer · edited May 23 '17 at 11:53

18

Yes.
If you lock all the public methods with the same lock it will be threadsafe.

You could consider using read-write locks, which may have better performance if you have a lot of concurrent readers.

If you don't have a lot of readers, it will just add overhead, but may be worth checking the option and testing.

edited May 23 '17 at 11:53

Community

1
1

answered Mar 16 '12 at 19:57

Yochai Timmer

48,127
24
147
185

15

I don't think that read-write locks make sense in a circular buffer. Both producers and consumers modify the buffer, so all of them are actually *writers*. – David Rodríguez - dribeas Mar 16 '12 at 20:40
1

@DavidRodríguez-dribeas - You're right in this case. I didn't really go into the design, just the thread safety part. – Yochai Timmer Mar 16 '12 at 20:51

score 5 · Answer 2 · answered Mar 16 '12 at 20:16

5

i think it looks fine, except that there is some pointless copies of Mat made in send. You don't need the new, you can directly push the argument of send to your cb.

answered Mar 16 '12 at 20:16

Willem Hengeveld

2,758
23
19

+1 for avoiding pointless, expensive and contention-increasing copying. – Martin James Mar 16 '12 at 22:42
@MartinJames I cannot directly push the argument of send. "cv::Mat class implements reference counting and shallow copy such that when an image is assigned to another one, the image data is not copied, and both images will point to the same memory block." "A reference count is kept such that the memory will be released only when all of the references to the image will be destructed. If you wish to create an image that will contain a new copy of the original image, you will use the method copyTo()." - from "OpenCV 2 Computer Vision Application Programming Cookbook" (p. 28). – Alexey Mar 19 '12 at 13:54
1

you still don't need the new in that case, allocating the new image on the stack works just as well. But isn't this a feature that you want, the shared copy in your circular buffer? – Willem Hengeveld Mar 19 '12 at 18:34
+1 for making me realize that I don't need to allocate Mat on the heap. Mat objects can be stored on the stack instead with `Mat image2;` `image.copyTo(image2);` and then `cb.push_back(image2);`. I still need to use copyTo() because otherwise all objects in the buffer will refer to the last sent object. Alternatively, I can move image copying to outside of my circ_buffer class, in which case, yes, I can push the argument of send directly to the buffer as you suggested. – Alexey Mar 19 '12 at 21:44
1

'when an image is assigned to another one, the image data is not copied, and both images will point to the same memory block' - is this not exactly what you want? When you assign the reference to the queue, no copy is made and there are now 2 references. When a new image is created in the producer and the old image pointer is overwritten, there is then only one reference - on the queue. So, no copying needed. – Martin James Mar 22 '12 at 20:03

score 2 · Answer 3 · edited May 23 '17 at 12:16

2

Your implementation is similar to the one shown by this blogger. You should read that blog to see if you missed anything in your implementation.

If your Mat objects are expensive to create/copy, you should avoid continuously creating/copying/deleting them. Instead, you should have a pool (aka free list) of Mat objects that continually get recycled in some kind of pipeline architecture. I describe this type of architecture in this answer to a related question.

In that answer, I suggested using a blocking stack to implement the pool, but you could also use your blocking circular_buffer. The reason I suggested a stack was because I thought it may be more cache-friendly, but I never actually measured to see if it would make a difference.

edited May 23 '17 at 12:16

Community

1
1

answered Mar 16 '12 at 23:27

Emile Cormier

28,391
15
94
122

Mat objects (images) are not too big and are pretty much the same size during each call. I just realized that I can allocate Mat objects on stack instead. – Alexey Mar 19 '12 at 21:17
I have been using object pools, (based on blocking queues), for a long time. In fact, I use the poolObject+message-passing pattern almost exclusively for my multithreaded app designs. The no-copy, no-malloc, no-GC and built-in flow-control are not the only advantages. In GUI apps, creating object pools before any forms or work threads means that I can usually forget about explicit pool destruction or thread-termination hassle. Dumping the pool levels on a timer shows up leaks without miserable 3rd-party leak tools like V******* that slow the wole app to a crawl. – Martin James Mar 22 '12 at 20:15
By default, the copy constructor of cv::Mat creates a shallow copy. – Reunanen Sep 19 '16 at 11:52

score 1 · Answer 4 · answered Aug 03 '20 at 07:04

1

Very old question :) Here is a dis gin with lock-free implementation Link

Here is a BSD-2 Lib for this Link

answered Aug 03 '20 at 07:04

Yacov

1,060
14
27

score 0 · Answer 5 · answered Aug 23 '21 at 12:15

//This implementation above is broken. You also need condition variable
//boost::condition buffer_not_full; and wait in send on available space in the circular buffer.
enter code here

void send (T imdata) {
   lock lk(monitor);
   while (cb.full())
      buffer_not_full.wait(lk);
   cb.push_back(imdata);
   buffer_not_empty.notify_one();
}

T receive() {
     lock lk(monitor);
     while (cb.empty())
        buffer_not_empty.wait(lk);
     T imdata = cb.front();
     cb.pop_front();
     buffer_not_full.notify_one();
     return imdata;
}

score 0 · Answer 6 · answered Mar 16 '12 at 19:58

0

Looks good at first glance, except that you are not using the buffer_not_full condition at all. You probably want to add code similar to the buffer_not_empty code.

answered Mar 16 '12 at 19:58

Daniel Roethlisberger

6,958
2
41
59

1

If a data source produces more data than can fit in the buffer, boost::circular_buffer object write over the oldest data. This is ok. So `buffer_not_full` condition doesn't need be checked. – Alexey Mar 16 '12 at 20:11

Thread safe implementation of circular buffer

6 Answers6

Linked