Multithreading C++ loop

Question

I've made a small skeleton for a larger project that will include cross-platform multithreading (by using Boost) and thread-safe random numbers (by using GNU scientific libraries and mutexes). My goal is to run a simulation (probably over 100,000 loops) in which each thread will utilize a random number and read from a library that the threads will not change the value of (thus, I don't bother putting the library in a mutex)!

Is this code alright?

#include <stdio.h> // Standard in out for c
#include <vector> // Vectors
#include <boost/thread.hpp> // Boost threads
#include <boost/signals2/mutex.hpp> // Boost mutexes
#include <gsl/gsl_rng.h> // Gnu Scientific Library, thread safe rand

const gsl_rng_type * T = gsl_rng_default; // Select random generator
gsl_rng * r = gsl_rng_alloc (T); // Allocate the range
boost::mutex mtx; // Create a mutex
std::vector<unsigned long> data; // Create a vector

int count=23; // Have a total number of threadFunctions you want to loop through
int CORES = static_cast<int>(boost::thread::hardware_concurrency());

void threadFunction(int id);

int main()
{
    printf("Number of cores = %d\n-------------------\n\n",CORES);
    gsl_rng_env_setup(); // Setup the random variable environment
    gsl_rng_set (r,time(NULL)); // Seed the r generator
    std::vector<boost::thread *> t; // Create vector of boost::thread pointers
    while(count>0){ // While you are looping
        for(int i=0;i<((CORES>count?count:CORES));i++) // Create 4 threads if more than 4 loops are left, else create the appropriate number of threads
            t.push_back(new boost::thread(threadFunction,i)); // Push threads objects back
        for(int i=0;i<t.size();i++){ // Loop through all threads
            t[i]->join(); // Wait to join.  Note, these are pointers so use -> instead of .
            count--; // Reduce count by 1
        }
        t.resize(0); // Empty the array
    }

    printf("\nEnd of threads...");
    FILE *out = fopen("RandomNumbers.txt","w+"); // Create a file to hold values
    for(int i=0;i<data.size();i++) // Print out values
        fprintf(out,"%lu\n",data[i]);
    fclose(out); // Close file
    printf("\nPress Enter to exit...");
    getchar();
    return 0;
}

void threadFunction(int id)
{
    { // By scoping in, the lock_guard can destroy itself upon exit
        boost::lock_guard<boost::mutex> ownlock(mtx); // Create lock_guard
        data.push_back(gsl_rng_get(r)); // Put a random number in data
    }
    printf("Working on %d...\n",id);
}

Answer 1

1. You're overdoing it with comments. A lot. And trivial ones at that. Every programmer knows that std::vector<boost::thread *> t; creates a vector of pointers to boost::thread, or that you have to use -> with pointers. Such comments are really just clutter and actually make the code harder to read. Save them for the non-obvious bits.

2. It's unclear what mtx protects. Does it protect data? Or r? Or both? This is one of the places where you should comment, but you didn't (//Create a mutex is absolutely pointless). If possible, rename the mutex to match its purpose. It would also be a good idea to wrap the mutex and the data it protects in a class, to ensure that unsafe access is impossible.

3. ((CORES>count?count:CORES)) requires the reader to parse it (and what's with the double parentheses?) The meaning of std::min(CORES, count), on the other hand, is immediately obvious. And how does it create 4 threads? You could just as well rewrite the entire loop to make it more concise (and perhaps more efficient too):

   while (count > 0) {
     std::vector<boost::thread *> threads(std::min(CORES, count));
     for (size_t i = 0; i < threads.size(); ++i) { // Start appropriate number of threads
       threads[i] = new boost::thread(threadFunction, i);
     }
     for (size_t i = 0; i < threads.size(); ++i) { // Wait for all threads to finish
       threads[i]->join();
       delete threads[i];
       --count;
     }
   }
   

   Also note that I added a call to delete into the waiting loop. Your previous code was leaking the threads.

4. You're writing C++. Don't use (type unsafe!) C I/O. Do this instead:

   {
     std::ofstream out("RandomNumbers.txt");
     for (size_t idx = 0; idx < data.size(); ++idx) {
       out << data[idx] << '\n';
     }
   }
   

   The same holds for all other uses of C I/O (printf(), getchar()), of course.

5. count and CORES would be better declared with type size_t, as they represent the number of some objects.

6. If you have access to C++11, all the above could be simplified even further using range-based for loops, lambdas in standard algorithms, or at the very least iterator-based loops. I didn't bother with iterators here (even though they could be used), because their type has to be spelled out explicitly.

   In such case, it would also make more sense not to allocate the threads dynamically, but create them directly in the vector. No need for manual deallocation, no dynamic allocation overhead.

Answer 2

boost::thread_group is the way to go. The part you might be missing to control the number of threads is boost::io_service.

boost::thread_group m_thread_group;
boost::asio::io_service m_io_service;
std::auto_ptr<boost::asio::io_service::work> m_work;

Initialize boost::asio::io_service::work with boost::asio::io_service to keep the thread alive when its finish its task.

create your threads:

for(int i =0; i < 4; i++) {
    m_thread_group.create_thread([&]() {
        m_main_service.run();
    });
}

Finally you can now use asynchronous calls to the threads with:

m_main_service.post(boost::bind(&A::my_function, this, ...));

Its a bit tricky to clean up and end the thread. You simple have to reset the work and have any long running or infinite function check for reset work.

NOTE: post will maintain a list of functions to execute. Each task posted will execute once a thread is free to do so.

NOTE 2: This approach is an asynchronous approach and is not easy to implement, but the flip side is that its one of the better way to multi-thread because it requires not explicit locking.

NOTE 3: To get results from a thread you will have to post back. Either using another boost::io_service::post() with boost or PostMessage() with windows. Different framework like Qt also have different methods to post results back.