Is there a better way to make this code thread-safe? Thread_local static seems a blunt tool

Viewed 259

The following code mimics a larger programme that creates an instance of a simulation and then parallelises it using firstprivate to make my instances private. However, the instance itself creates two more instances within its methods.

The structure seems contrived but my hands are tied somewhat: the classes and their dependence are dictated to me by the tools I would like to use and I think this scenario is common in the scientific computing community.

It compiles fine and upon manual testing appears to be thread-safe and working.

But I am not sure I am using C++ technology in an optimal way as I suspect I could declare instances further up in the memory hierarchy and save myself from having to use a static variable, possibly via passing an instance created in a parallel region by reference to my other instance or something like that. I suspect this is best because everything within the braces of #pragma omp parallel {} is local to the thread.

Hence, my goal is to create two (or more) thread-local, independent instances of each class, and in particular GenNo as it models a random number generator that is to be seeded once per thread and then simply called with the same seed although here I am changing what I call the 'seed' in a predictable way just to understand the programme's behaviour and reveal violations of thread safety / race conditions.

The code that is commented out did not work but produced a 'segmentation fault' with the programme exiting with SIGSEGV 11. I believe that the 'unique' pointer is not so unique when deployed in parallel. Overall, this solution seems more elegant though and I would like to make it work but am happy to listen to your comments.

In order to get the std::unique_ptr functionality, the line beginning with thread_local static has to be commented out and the other comments have to be removed.

#include <iostream>
#include <omp.h>
//#include <memory>

class GenNo
{
public:

    int num;

    explicit GenNo(int num_)
    {
        num = num_;
    };

    void create(int incr)
    {
        num += incr;
    }
};

class HelpCrunch{
public:
    HelpCrunch() {

    }

    void helper(int number)
    {
        std::cout << "Seed is " << number << " for thread number: " << omp_get_thread_num() << std::endl;
    }
};

class calculate : public HelpCrunch
{
public:

    int specific_seed;
    bool first_run;

    void CrunchManyNos()
    {
        HelpCrunch solver;

        thread_local static GenNo RanNo(specific_seed);
        //std::unique_ptr<GenNo> GenNo_ptr(nullptr);
        /*
        if(first_run == true)
        {
            GenNo_ptr.reset(new GenNo(specific_seed));
            first_run = false;
        }
         solver.helper(GenNo_ptr->num);
*/
        RanNo.create(1);
        solver.helper(RanNo.num);



        //do actual things that I hope are useful.
    };
};




int main()
{

    calculate MyLargeProb;
    MyLargeProb.first_run = true;

#pragma omp parallel firstprivate(MyLargeProb)
    {
        int thread_specific_seed = omp_get_thread_num();
        MyLargeProb.specific_seed = thread_specific_seed;

        #pragma omp for
        for(int i = 0; i < 10; i++)
        {
            MyLargeProb.CrunchManyNos();
            std::cout << "Current iteration is " << i << std::endl;
        }

    }
    return 0;
}

Now the output with the thread_local static keyword was:

Seed is 2 for thread number: 1
Current iteration is 5
Seed is 3 for thread number: 1
Current iteration is 6
Seed is 4 for thread number: 1
Current iteration is 7
Seed is 5 for thread number: 1
Current iteration is 8
Seed is 6 for thread number: 1
Current iteration is 9


Seed is 1 for thread number: 0
Current iteration is 0
Seed is 2 for thread number: 0
Current iteration is 1
Seed is 3 for thread number: 0
Current iteration is 2
Seed is 4 for thread number: 0
Current iteration is 3
Seed is 5 for thread number: 0
Current iteration is 4

While without using thread_local but retaining static I get:

Seed is 2 for thread number: 1
Current iteration is 5
Seed is 3 for thread number: 1
Current iteration is 6
Seed is 4 for thread number: 1
Current iteration is 7
Seed is 6 for thread number: 1
Current iteration is 8
Seed is 7 for thread number: 1
Current iteration is 9


Seed is 5 for thread number: 0
Current iteration is 0
Seed is 8 for thread number: 0
Current iteration is 1
Seed is 9 for thread number: 0
Current iteration is 2
Seed is 10 for thread number: 0
Current iteration is 3
Seed is 11 for thread number: 0
Current iteration is 4

If I leave out the static keyword altogether, the instance just keeps getting re-assigned and while I strongly suspect it would be kept private to the thread, it is of little use as the counter would be stuck at 1 or 2 for thread 0 and 1 on a dual core machine. (The real-world application would have to be able to 'count up' and be undisturbed by the parallel thread.)

What I need help with

Now, I modeled the example in way that a violation of thread safety would become apparent via the counters interfering with each other, as we can see is the case when thread_local is left out but static is left in (having neither is silly). The class HelpCrunch is in reality much more complex and most likely thread-safe and fine to be reinitialised with each loop repetition. (This is actually better because it picks up a bunch of variables from its child that is a private instance.) But do you think I would be better off adding thread_local to the creation of solver as well, without the static keyword? Or should I declare the instance elsewhere, in which case I would need help with the passing by pointer/reference etc.

1 Answers

First, your example uses the global object std::cout in a thread-unsafe way, accessing it concurrently from multiple threads. I had to add #pragma omp critical at some places to get a readable output.

Second, the commented code crashes because GenNo_ptr has automatic duration so that it is destroyed every time CrunchManyNos() completes execution. Therefore, when first_run is false you are derefencing a nullptr pointer.

When it comes to your specific question, there is a huge difference between making RanNo static or static thread_local:

  • If it is static there will be a single instance of RanNo initialized the first time CrunchManyNos() gets executed. It is to some extent a global variable that would be used unsafely in the concurrent context of your example.

  • If it is static thread_local (by the way using openmp you should prefer threadprivate) it will be created the first time a new thread calls CrunchManyNos() and will last for the duration of the thread.

Related