Data races resolution C++

Question

I wanted to ask for some help in solving the data races in my program. I started with a simulation of how things should work if I were using multithreading and then modified the code so that I can check if I really obtain those results but I don't know how to resolve the data races in the code. Can someone help me please? My code is the following:

void pi(size_t& id, size_t nThread, size_t N, double& pigreco)
{
    size_t begin = id* N / nThread;
    size_t end = (id+ 1) * N / nThread;

    random_device rd;       // Object to create random seed
    mt19937 generator(rd());    // Mersenne Twister seeded with rd()
    uniform_real_distribution<double> distribution(-1.0, 1.0);

    for (size_t i = begin; i < end; i++) {
        double x = distribution(generator);
        double y = distribution(generator);
        if (sqrt(x*x + y*y) < 1.0)
            pigreco += 4.0 / double(N);
    }
}

int main(int argc, char* argv[])
{
    if (argc != 2) {
        cerr << "Usage: ./pi <number of threads>" << endl;
        exit(EXIT_FAILURE);
    }

    size_t nThread = (size_t)atoi(argv[1]);

    size_t N = 100000000;
    cout << "Generating " << N << " random values using " << nThread << " thread(s)." << endl;

    atomic<double> pigreco = 0.0;

    // create threads
    vector<thread> threads(nThread);
    for (size_t i = 0; i < nThread; i++)
        threads[i] = thread(pi, ref(i), nThread, N, ref(pigreco));

    for_each(threads.begin(), threads.end(), mem_fn(&thread::join));

    cout << "Estimated value for pi: " << pigreco << endl;

    exit(EXIT_SUCCESS);
}

I tried using valgrind in order to find the possible data races and I found out that there is one at the first for loop of the main, I think probably because I use 'i' as argument of my function but that I don't know how to resolve it.

Drop the `ref(i)` so you pass a copy to the `std::thread` constructor? — Botje, Nov 25 '22 at 15:54
`thread(pimontecarlo, ref(i), ...)` is unwise. You give all threads a reference to the same variable. Moreover, that variable is local to the loop - once the loop exists, the variable is destroyed and threads are left holding a dangling reference. Why are you passing `i` by reference, and not by value? It doesn't make sense. — Igor Tandetnik, Nov 25 '22 at 15:55
Also note that having a few threads hammering on a single atomic variable is pants for performance. Have each thread produce its result in a local variable and only update `pi` at the end of each thread. — Botje, Nov 25 '22 at 15:55
You have a data race on `pi`. All threads are writing to the same variable, concurrently with no synchronization. It's not entirely clear what you are trying to achieve here, so it's difficult to suggest how to fix it. — Igor Tandetnik, Nov 25 '22 at 15:58
You don't even use `threadID` for anything useful. You use it in computing `begin` and `end` - but the absolute values of those variables don't matter, only the difference `end - begin` does, as that's how many times the loop runs. And that difference is always `N / numThreads`, independent of `threadID` — Igor Tandetnik, Nov 25 '22 at 16:00
Have each thread compute an integer counter of how many times a random point falls into the circle. Don't have them all write to the same counter variable, give each thread a separate variable. Then, after the threads complete, add up those counters in `main`, and compute `pi = 4.0 * counter / N` — Igor Tandetnik, Nov 25 '22 at 16:05
`sqrt(x*x + y*y) < 1.0` is equivalent to `x*x + y*y < 1.0`, the latter avoids calling an expensive function — Igor Tandetnik, Nov 25 '22 at 16:06

score 1 · Accepted Answer · answered Nov 25 '22 at 17:23

I don't know how to resolve the data races in the code.

Then stop using std::thread, and especially stop passing references to functions on those threads.

Here is one way you could avoid passing any references between threads:

#include <cstdlib>
#include <iostream>
#include <random>
#include <future>
#include <algorithm>

int pimontecarlo(size_t iterations)
{
    int count = 0;

    std::random_device rd;
    std::mt19937 generator(rd());
    std::uniform_real_distribution<double> distribution(-1.0, 1.0);

    for (size_t i = 0; i < iterations; i++) {
        double x = distribution(generator);
        double y = distribution(generator);
        if (sqrt(x*x + y*y) < 1.0) ++count;
    }

    return count;
}

int main(int argc, char* argv[])
{
    if (argc != 2) {
        std::cerr << "Usage: ./pimontecarlo <number of threads>" << std::endl;
        exit(EXIT_FAILURE);
    }

    size_t numThreads { atoi(argv[1]) };

    size_t N = 100000000;
    std::cout << "Generating " << N << " random values using " << numThreads << " thread(s)." << std::endl;

    // create futures
    std::vector<std::future<int>> results(numThreads);
    for (auto & result : results)
        result = std::async(pimontecarlo, N / numThreads);

    int count = 0;
    for (auto & result : results)
        count += result.get();

    double pi = (4.0 * count) / N;

    std::cout.precision(12);
    std::cout << "Estimated value for pi: " << pi << std::endl;

    exit(EXIT_SUCCESS);
}

See it on coliru

Data races resolution C++

1 Answers1