Multi-threaded Coin-toss Experiment

Question

Main point up front: What do I need to do change to display the correct values for heads/tails in my records?

Edit 1: The arrays of integers inside the Record appear to fill with random values once there is more than 1 thread.

I've been trying to debug this for a few days now. My code is complete and fully executes. (Just so everyone knows, I'm a student and not pretending to be a professional programmer.)

In my multi-threaded coin toss program, I'm trying to capture the amount of times heads or tails occurs in each thread. I'm flipping a coin a total of 100,000,000 times. (One-hundred million.)

The elements in the arrays inside my record class are not accumulating correctly. I know I don't need to use mutex locks, because each thread is accessing a separate memory location unique to the thread. But the threads are updating the values incorrectly, which shouldn't be happening.

Here are examples from my debug.

1. Single thread (notice the correct amount of heads/tails):

2. Two threads (now heads/tails count just goes crazy.):

Fully executable code sample is below, followed by sample output. Also, here is link to full code on Github as well:

#include <iostream>
#include <thread>
#include <vector>
#include <mutex>
#include <random>
#include <algorithm>
#include <time.h>
#include <strstream>
#include <random>


using namespace std;

default_random_engine dre;
uniform_int_distribution<int> Tosser(0,1);

struct Record {
    Record();
    ~Record();
    int numThreads;
    int *heads;
    int *tails;
    time_t startTime;
    time_t stopTime;
    time_t duration;
    int timesToToss;
};

Record::Record(): numThreads(0), heads(NULL), tails(NULL), startTime(NULL), stopTime(NULL), timesToToss(0){}
Record::~Record() {
    startTime = NULL;
    stopTime = NULL;
    duration = NULL;
    timesToToss = NULL;
    delete [] heads;
    heads = NULL;
    delete [] tails;
    tails = NULL;
    numThreads = NULL;
}

void concurrency(){
    vector<thread> threads;

    Record *records = new Record[4];
    Record *recPtr;
    int *numThrPtr;
    int *headsPtr;
    int *tailsPtr;
    time_t *startTimePtr;
    time_t *stopTimePtr;

    vector<time_t> durations;

    int timesToToss = 100000000; // Times to flip the coin.
    int index = 0; // Controls which record is being accessed.

    for(int i=1;i<3;i*=2){ //Performs 2 loops. 'i' is calculated to represent the number of threads for each test: 1, and 2 (full code contains up to 8 threads.)

        recPtr = &records[index]; //Get the address of the next record in the Record array.
        recPtr->timesToToss = timesToToss; //
        recPtr->numThreads = i; //Record the quantity of threads.
        recPtr->heads = new int[recPtr->numThreads]; //Create a new heads array, of 'x' elements, determined by number of threads.
        recPtr->tails = new int[recPtr->numThreads]; //Create a new tails array, of 'x' elements, determined by number of threads.
        recPtr->startTime = time(0); //Record the start time.

        for(int j = 0;j<recPtr->numThreads;j++){ //Start multi-threading.

            headsPtr = &recPtr->heads[j]; // Get the address of the index of the array, one element for each thread for heads.
            tailsPtr = &recPtr->tails[j]; // Get the address of the index of the array, one element for each thread for heads.

            threads.push_back(thread([&headsPtr, &tailsPtr, timesToToss](){for(int k=0;k<timesToToss;k++){ if (Tosser(dre)) ++(*headsPtr); else ++(*tailsPtr); } })); //Toss a coin!
        }

        for(auto& thread: threads) thread.join(); // Collect/join all the threads.

        while(!threads.empty()){ //Don't want to try and join 'live' threads with 'dead' ones!
            threads.pop_back();//Clear out the threads array to start with an empty array the next iteration.
        }

        recPtr->stopTime = time(0); //Record the end time.

        recPtr->duration = recPtr->stopTime - recPtr->startTime;

        timesToToss /= 2; //Recalculate timesToToss.
        ++index; //Increase the index.
    }

    for (int i=0;i<4;i++){ //Display the records.
        recPtr = &records[i];
        cout << "\nRecord #" << i+1 << ", " << recPtr->numThreads << " threads.";
        cout << "\nStart time: " << recPtr->startTime;
        cout << "\nStop time: " << recPtr->stopTime;
        cout << "\nTossed " << recPtr->timesToToss << " times (each thread).";
        cout << "\nHeads appeared << " << recPtr->heads << " times.";
        cout << "\nTails appeared << " << recPtr->tails << " times.";
        cout << "\nIt took " << recPtr->duration << " seconds.";
        durations.push_back(recPtr->duration);
        cout << "\n" << endl;
    }

    sort(durations.begin(),durations.end());

    cout << "Shortest duration: " << durations[0] << " seconds." << endl;

    delete [] records;
    records = NULL;
}

int main() {

    concurrency();

    return 0;
}

Output for recored #2 is[updated 5/5/2016 @ 2:16pm CST]:

Record #2, 2 threads.
Start time: 1462472702
Stop time: 1462472709
Tossed 50000000 times (each thread).
Heads appeared << 474443746 times.
Tails appeared << -1829315114 times.
It took 7 seconds.

Shortest duration: 3 seconds.

Process finished with exit code 0

Answer 1

int *heads; defines heads to be a pointer.

The default behaviour of << is to print the address of a pointer, not the data pointed at. There is an exception for pointers to char to print out c-style strings.

Since each thread gets its own int to count the number of heads generated by the thread, when the threads have completed the head counts must be summed and the sum printed.

In addition,

recPtr->heads = new int[recPtr->numThreads]; 

allocated storage for head counters, but nothing I can find in the code initializes them. This be Undefined behaviour. A simple hack fix for this is:

    for(int j = 0;j<recPtr->numThreads;j++){

        recPtr->heads[j] = 0;
        recPtr->tails[j] = 0;
        headsPtr = &recPtr->heads[j]; // Get the address of the index of the array, one element for each thread for heads.
        tailsPtr = &recPtr->tails[j]; // Get the address of the index of the array, one element for each thread for heads.

        threads.push_back(thread([&headsPtr, &tailsPtr, timesToToss]()
        {
            for(int k=0;k<timesToToss;k++)
            { 
                if (Tosser(dre)) ++(*headsPtr); 
                else ++(*tailsPtr); 
            } 
        })); //Toss a coin!
    }

Finally, (edit 3) the lambda definition thread([&headsPtr, &tailsPtr, timesToToss]() captures pointers to headsPtr and tailsPtr, so by the time the threads get a chance to start, all threads are pointing at the headsPtr and tailsPtr of the last threads.

Hack kludge is now:

    for (int j = 0; j < recPtr->numThreads; j++)
    {

        recPtr->heads[j] = 0;
        recPtr->tails[j] = 0;
        headsPtr = &recPtr->heads[j]; // Get the address of the index of the array, one element for each thread for heads.
        tailsPtr = &recPtr->tails[j]; // Get the address of the index of the array, one element for each thread for heads.

        threads.push_back(thread([headsPtr, tailsPtr, timesToToss]()
        {
            for(int k=0;k<timesToToss;k++)
            {   
                if (Tosser(dre)) ++(*headsPtr);
                else ++(*tailsPtr);
            }
        })); //Toss a coin!
    }

I've cleaned up the formatting of the lambda to make what went wrong easier to read.