8
\$\begingroup\$

I have a producer consumer that simulates the interaction of threads controlling a hardware device.

I have 1 thread controlling the device, 1 thread reading, and 1 thread outputting. The read is synchronous on the thread while the controlling is asynchronous.

My producer is the thread controlling the device and my consumer is a thread that processes the data. Often the consumer takes about the same amount of time as the producer (ideal), but sometimes it takes more time.

The hope is that the producer should not produce more than the consumer can process. This is partly due to hardware overflow, the consumer will free a buffer on the device, and partly due to visual latency.

I think the producer should be responsible for regulating the rate of production. I implemented this regulation using a sleep timeout.

I am wondering if this is the best way to do it.

My heuristic for success is the time output of the redraw command, which should be roughly ConsumerTime*8 (maybe 400).

#include <cstdlib>
#include <thread>
#include <condition_variable>
#include <iostream>
#include <atomic>
#include <chrono>
//UTILITY
//
using namespace std;
#if !defined(WIN32)
#include <unistd.h>
void
_sleep (int ms)
{
    usleep (ms * 1000); //convert to microseconds
}
#endif
long long
timestamp ()
{
    auto now = std::chrono::system_clock::now ();
    auto duration = now.time_since_epoch ();
    auto millis = std::chrono::duration_cast<std::chrono::milliseconds>(duration).count ();
    return millis;
}
//REDRAW
//
bool killRedraw;
bool drawIt;
long long last;
void
redraw ()
{
    cout << "Starting drawing thread" << endl;
    last = timestamp ();
    while (!killRedraw)
    {
        if (drawIt)
        {
            long long now = timestamp ();
            cout << "I drew it:" << now - last << endl;
            last = now;
            drawIt = false;
        }
        _sleep (15);
    }
}
//PRODUCER
//
atomic<int> balance;
bool killProducer;
void
produce ()
{
    cout << "Made one:" << ++balance << endl;
}
void
producer ()
{
    balance = 0;
    cout << "Starting producer thread" << endl;
    while (!killProducer)
    {
        _sleep (40);
        produce ();
        while (balance > 1)
        {
            _sleep (10);
        }
    }
}
//CONSUMER
//
bool killConsumer;
void
consume ()
{
    cout << "Ate one:" << --balance << endl;
}
void
consumer ()
{
    cout << "Starting consumer thread" << endl;
    while (!killConsumer)
    {
        int basetime = 35;
        int noise = rand () % 20;
        _sleep (basetime + noise);
        consume ();
        static int cycle = -1;
        if ((cycle = ++cycle % 8) == 7)
        {
            drawIt = true;
        }
    }
}
//Main
//
int
main (int argc, char** argv)
{
    //
    // Important metric is the "draw it" time, which should be about 8*50
    //
    killProducer = false;
    std::thread tP (producer);
    killConsumer = false;
    std::thread tC (consumer);
    killRedraw = false;
    drawIt = false;
    std::thread tD (redraw);
    _sleep (10000);
    killProducer = true;
    killConsumer = true;
    killRedraw = true;
    tD.join ();
    tP.join ();
    tC.join ();
    //
    return 0;
}
\$\endgroup\$
4
\$\begingroup\$

I have some minor things to point out:

  • There is not a single empty line in this code, and this essentially looks like globals, functions, and comments stacked on top of each other. You should have each type separated so that the code is easier to read (especially with the function names and return types on separate lines).

  • Since you're using C++11, you should be using <random> in place of std::rand. It has been determined that rand() is harmful and should no longer be used in C++11.

  • Unless I'm misunderstanding the use of multithreading here, I don't think you should be using any global variables, as they're prone to bugs. If that's the case, then you should just be passing them to functions as needed. If not, then at least put them all towards the top (they're already global, so it doesn't make a difference to have them in different places outside the functions).

  • You don't need to be using std::endl everywhere. It doesn't appear that you're needing to flush the buffer each time, which can also affect performance. Instead, output a "\n" to produce a newline without also flushing the buffer.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback, although these things weren't really the focus of my question, rather they are in there to ensure the thing compiles. Also the booleans should probably be atomic... \$\endgroup\$ – Mikhail Jul 8 '14 at 19:10
  • \$\begingroup\$ @Mikhail: I understand, but we're allowed to comment on any aspect of the code. This was also an older question, so it probably would've stayed unseen had I not said anything. \$\endgroup\$ – Jamal Jul 8 '14 at 19:35

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.