Optimizing producer consumer using monitors

Question

I am trying to review concurrency concepts, i.e monitor, semaphores, etc. I've learned that Semaphores are natively not available in C++11 (presumably because it is very error prone) - which is why I am trying to review monitors.

The producer/consumer below already works correctly (to my knowledge), but I am wondering if it is possible/correct to make an optimization and explicitly release the unique_lock, instead of waiting for it to destroy itself.

std::mutex mutex;
std::condition_variable space_in_buffer;
std::condition_variable buffer_empty;
std::queue<int> buffer;
#define MAX_BUFFER_SIZE 10

void produce() {
    while (1) {
        std::unique_lock<std::mutex> lock(mutex);
        if (buffer.size() == MAX_BUFFER_SIZE) {
            space_in_buffer.wait(lock);
        } else {
            // produce
            int new_product = rand() % 30;
            buffer.push(new_product);
            printf("Produced [%d] | Item Count: %lu\n", new_product,
                    buffer.size());

            /*
             * Possible optimization to call lock.unlock() here, instead
             * of waiting for the unique lock to go out of scope ?
             */

            buffer_empty.notify_one();

            // potential for extra processing here
        }
    }
}

void consume() {
    while (1) {
        std::unique_lock<std::mutex> lock(mutex);
        if (buffer.size() == 0) {
            buffer_empty.wait(lock);
        } else {
            // consume
            int to_be_consumed = buffer.front();
            buffer.pop();

            printf("Consumed [%d] | Item Count: %lu\n", to_be_consumed,
                    buffer.size());

            /*
             * Possible optimization to call lock.unlock() here, instead
             * of waiting for the unique lock to go out of scope ?
             */

            space_in_buffer.notify_one();

            // potential for extra processing here
        }
    }
}

Answer 1

1. for(;;) is the more idiomatic infinite loop, read as "forever".

2. Avoid long conditional blocks, and unneccessary indentation. What you actually have is a loop followed by the else-block. Which would be best written as:

   while (buffer.size() == MAX_BUFFER_SIZE)
       space_in_buffer.wait(lock);
   rest-of-loop;
   

   Unless you prefer using a lambda for the condition.

   space_in_buffer.wait(lock, [&]{ return buffer.size() == MAX_BUFFER_SIZE; });
   rest-of-loop;
   

3. You are using the std::queue as a ring-buffer. That's inefficient, especially as the queue still allocates and frees its nodes on .push() and .pop(). Use a dedicated ringbuffer like boost::circular_buffer, orsomething without any dynamic allocation at all.

4. The format for printing a std::size_t with std::printf is "%zu". Don't drop the length-marker.

5. Mutex, buffer, and condition-variables should be encapsulated in a class-template with the buffer-size as a parameter for reuse.

6. Yes, you should minimize the time you hold the lock to maximize concurrency. See whether you can do that with scoping or need to manually release the lock.