Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am new toboost::thread I am making a producer consumer with a Monitor. This is how I've coded it so far.

//{ Declarations in header
private:
  boost::condition_variable    _condition;
  boost::mutex                 _mutex;
  std::deque<RawMatrix*>       _queue;
  boost::detail::atomic_count  _count;
//}

void MatrixMonitor::deposit(RawMatrix* rawMatrix){
    boost::unique_lock<boost::mutex> lock(_mutex);
    _condition.wait(lock, boost::bind(std::less_equal<int>(), boost::ref(_count), max));
    _queue.push_back(rawMatrix);
    ++_count;
    _condition.notify_one();
}

RawMatrix* MatrixMonitor::withdraw(){
    boost::unique_lock<boost::mutex> lock(_mutex);
    _condition.wait(lock, boost::bind(std::greater_equal<int>(), boost::ref(_count), min));
    RawMatrix* elem = _queue.front();
    _queue.pop_front();
    --_count;
    _condition.notify_one();
    return elem;
}

So far I've written the Producer like this

void MatrixProducer::produce(){
    while(true){
        boost::mutex::scoped_lock lock(_mutex);
        RawMatrix* matrix = rawMatrix();
        _monitor->deposit(matrix);
        boost::this_thread::sleep(boost::posix_time::milliseconds(sleep_msecs));
    }
}
boost::thread& MatrixProducer::start(){
    _thread = boost::thread(boost::bind(&MatrixProducer::produce, this));
    return _thread;
}
RawMatrix* MatrixProducer::rawMatrix(){/*Generates and returns a matrix*/}

and The Consumer

boost::thread& MatrixConsumer::start(){
    _thread = boost::thread(boost::bind(&MatrixConsumer::consume, this));
    return _thread;
}

void MatrixConsumer::consume(){
    while(true){
        boost::mutex::scoped_lock lock(_mutex);
        RawMatrix* matrix = _monitor->withdraw();
        _matrix->deposit(Matrix);
        boost::this_thread::sleep(boost::posix_time::milliseconds(sleep_msecs));
    }
}

Is that Okay ? and also who will have the ownership of this Producer, Consumer and Monitor ?

share|improve this question
1  
We can review the first part of the code here. But this is probably not the best site to ask how to do something; first its off-topic but secondly there are fewer experts roaming around (and thus you may not get a good answer). So ask the how question on SO to get the best answers. –  Loki Astari Jul 28 '12 at 19:08

2 Answers 2

Why do you insist on putting the '' on members. It makes them look so ugly. Also most identifiers beginning with '' are reserved so you need to be careful. If you must use decade old conventions (that have been abandoned) to identify members use 'm_` as the prefix.

The reason I hate them is for this:

--_count;

// That is ugly and harder to read than:

--count; // but that's just an opionon.
         // The bit about being reserved is a problem though.
         // and if you use leading underscores you should understand
         // all the associated rules.

Curious where max/min come from?

_condition.wait(lock, boost::bind(std::less_equal<int>(), boost::ref(_count), max));
                                                                        //   ^^^^^

Passing RAW pointers around is not common in C++ (unlike C).
Normally you wrap a pointer inside a smart pointer object to indicate ownership semantics (and thus who is responsible for deleting the object).

// No indication of ownership is retained or passed the the moniter.
// So we have no idea who should delete rawMatrix?
//
// Have you passed ownership to the moniter or is the caller retaining ownership
// If the owner retains ownership when does he delete it?
void MatrixMonitor::deposit(RawMatrix* rawMatrix){


// I suppose you have to return a pointers as you were passed a pointer.
// but now the caller has no idea weather he should delete the pointer he
// is given or if the pointer is still owned by somebody else.
RawMatrix* MatrixMonitor::withdraw(){

Personally I would change the interface to make sure the ownership symantics are well defined. In this case I would pass ownership to the moniter and pass ownership out to the person withdrawing the item:

void MatrixMonitor::deposit(std::unique_ptr<RawMatrix> rawMatrix){

std::unique_ptr<RawMatrix> MatrixMonitor::withdraw(){

I think you want to separate producer and consumer from the Moniter. These are three different types of object. When you construct the producer/consumer you pass it a monitor object to use when it deposits/withdraws items.

int main()
{
      MatrixMonitor   monitor;
      Producer        farmer(monitor);  // Pass monitor by reference 
                                        // So the farmer is adding to this moiter.
      Consumer        shopper(moniter); // shooper uses the same monitor as the
                                        // farmer but because it is a consumer will
                                        // call withdraw() while the 
                                        // farmer calles deposit()

      farmer.run();
      shopper.run();

      // don't forget to wait for the threads to exit.
      farmer.join();
      shopper.join();
}

Edit: Based on new code for Producer/Consumer

One would assume that the producer needs to finish at some point.
You may want to change while(true) into while(!finished) or similar.

void MatrixProducer::produce(){
    while(true){

Don't see any need for this mutex.

        boost::mutex::scoped_lock lock(_mutex);

Ownership semantics again. Who owns the generated matrix?

        RawMatrix* matrix = rawMatrix();
share|improve this answer
    
Please Check my edit. I'll focus on the Ptr issue. but I've added my current Producer consumer code. –  Dipro Sen Jul 28 '12 at 19:48

When I wrote a similar class a few years ago, I found it was actually possible to:

  1. Make the collection type templated.
  2. Keep the queuing / threading logic in a non-templated base class so that it did not need to be exposed in the header file (unlikely the templated collection it was protecting which did).

Of course it doesn't have to "derive", but separate concerns of the collection type and the locking logic.

The other thing I have always implemented in this kind of collection is multi_push and multi_pop whereby a producer can add more than one item in a single locking action, and a consumer can similarly clear all the data to process rather than pull them off in ones.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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