Generating random numbers in multiple threads

Question

I'm trying to learn how to use threads to generate lots of random numbers. In particular, I'd like know:

• Is the following code thread safe? (I think the histogram class is the only one that requires special attention)

• Are there implication for the 'randomness' of my numbers when they are generated from different threads?

• Is my timing giving me meaningful results?

Currently, I can't use a std::vector as my container type, because my histogram class relies on the container being initialised with zeroed elements. Is it possible to allow both fixed length and variable length containers?

#include <random>
#include <array>
#include <iostream>
#include <iomanip>
#include <thread>
#include <algorithm>
#include <mutex>
#include <chrono>

//class to produce random numbers from a poisson distribution
class poisson_generator
{
    public:
        poisson_generator( int seed, double mean)
            :engine{ seed }, poisson{ mean } {}

        int operator()()
        { return sample(); }

        int sample()
        { return poisson(engine) ; }

    private:
        std::default_random_engine engine;
        std::poisson_distribution<int> poisson;

};


//class to store random numbers
template < class Container>
class histogram
{
    public:
        histogram()
            :fData{} {}

        void fill( int val )
        {
            std::lock_guard<std::mutex> guard(fMutex);
            ++fData[val];
        }

        typename Container::iterator begin()
        { return fData.begin(); }

        typename Container::iterator end()
        { return fData.end(); }

        typename Container::value_type at(typename Container::size_type pos)
        {
            std::lock_guard<std::mutex> guard(fMutex);
            return fData.at(pos);
        }


   private:
        Container fData;
        std::mutex fMutex;
};

//Function to gnenerate events
    template <class Generator, class Storage>
void generate_events( Generator& g , Storage& store, int nEvents )

{
    std::cout << "Generating " + std::to_string(nEvents) + " events.\n";
    while ( nEvents-- )
    {
        store.fill( g() );
    }
}


//Printer for the histogram
    template <class H>
void printHistogram( H& h, unsigned int min, unsigned int max, int peak = 30 )
{

    auto total = std::accumulate( begin(h), end(h), 0 );
    std::cout << "There were " << total << " events" << '\n' ;

    auto dmax = *std::max_element( begin(h), end(h) );
    auto denom = (dmax > peak  ? dmax : peak ) / peak;

    auto length = end(h) - begin(h);

    for ( unsigned int i = min ; i != max ; ++ i)
    {
        std::cout << std::setw(4) << i << ' '
            << (i > length ? 0 : std::string( h.at(i) / denom, '*' ) ) << '\n';
    }
}

int main( int argc, char * argv[] )
{
    //Set up
    histogram<std::array<int, 100> > h;
    int sample_size = 1000000;
    std::random_device rd;

    //Create threads
    std::vector<std::thread> threads;
    auto nThreads = 8; //default

    //Read number of threads from command line
    if (argc == 2 )
    {nThreads = std::stoi(argv[1]) ; }

      auto start = std::chrono::monotonic_clock::now();

for( int t = 0 ; t != nThreads ; ++t )
{
    //Divide sample size between nThreads
    int sample_t = sample_size / nThreads;

    if ( t == nThreads - 1 )
    {
        sample_t = sample_size - ( nThreads - 1 ) * sample_t ;
    }

    //Generate random events
    threads.push_back(std::thread( [&h, t, sample_t, &rd] ()
                {
                poisson_generator pg ( rd(), 10 );
                generate_events( pg, h, sample_t);
                }
                ) );
}

for (auto& thread : threads )
{ thread.join() ; }

 //Timing print out
    auto finish = std::chrono::monotonic_clock::now();
    auto time_period = finish - start;
    std::cout << "Processing took " <<
        std::chrono::duration<double, std::milli>(time_period).count()  << " ms .\n" ;

    //Histogram print out
    printHistogram( h, 0, 25);

    return 0;
}

Answer 1

There are a couple of things to worry about wrt to shared memory.

First, histogram<T>::begin and histogram<T>::end provide unguarded access to the container. You're using them safely here, but you should generally not provide such unguarded access. One possible solution is a friend histogram_range, which has a std::lock_guard against fMutex as a member and provides begin and end functions that return Container::const_iterator.

Second, you provide shared access to rd. The standard doesn't require (as far as I can tell) that rd::operator() be thread-safe. Better would be to add the following line to your for loop: const auto seed = rd(); and capture seed instead of &rd in the lambda expression.

Otherwise, your thread safety seems OK.

---

I can't imagine that generating random numbers from different threads would have a deleterious effect on the randomness of the numbers. In fact, the excellent <random> primer released by the C++ standards committee explicitly mentions (approvingly) that applications may create thread-local engines (see footnote 14 on pg 5).

---

You appear to be measuring time correctly (except see below), and in particular you appear to be printing the number of milliseconds that your program takes to generate the random data you care about. Beyond that, I'm not sure what you mean by meaningful.

std::monotonic_clock was present in some drafts, but was removed from the final standard. Use std::steady_clock instead.

---

Some general notes:

In histogram: I suggest that at be declared const, and fMutex be declared mutable. Read-only access to a const histogram<T>& should be possible.

Your code has undefined behavior whenever your generator generates a number larger than 100 (because you use a std::array<int, 100> as your backing store). Consider using at instead of operator[] to access the data, as at does bounds checking. Probably a better solution is to use a std::vector as your storage instead of templatizing on the Container type, and doing explicit bounds checking yourself in fill. You can then expand the vector (e.g. with

if (val > fData.size()) fData.insert(fData.end(), val - fData.size(), 0);

Your spacing and indentation are inconsistent. That makes your code more difficult to read.

Answer 2

I just have a few minor points:

• C++11 now supports right-angle brackets for templates, so instead of this:

  histogram<std::array<int, 100> > h;
  

  you can now have this (with them together):

  histogram<std::array<int, 100>> h;
  

• This is not very maintainable:

  if (argc == 2 )
  {nThreads = std::stoi(argv[1]) ; }
  

  If you're going to use curly braces for possible additional lines, it may look misleading to arrange them in such a way. Just do it the same way you're already doing with existing multi-line statements.

• Your indentation within main() is inconsistent. Specifically, the two for loops are left-aligned, making it look like they're separate functions. Make sure all of this stays consistent.