Making GetValue threadsafe

[Miguel Angel Celigueta]

Hi all,

In a couple of occasions I lost quite a lot of time finding a bug that was throwing a segmentation fault due to a race condition.
It happened to me in OpenMP parallel processes, when accessing ProcessInfo[SOME_VARIABLE] or GetProperties()[SOME_OTHER_VARIABLE]. The memory was not allocated for those variables and different threads attempted to allocate it at the same time. Some methods like InitializeSolutionStep don't accept the ProcessInfo as const, so one must use some tricks -like casting to const- to avoid this problem. Then I thought that we could work on the DataValueContainer to make it threadsafe. Does it make sense to add a 'critical' in the GetValue method? Written next:

    template<class TDataType> TDataType& GetValue(const Variable<TDataType>& rThisVariable)
    {
        typename ContainerType::iterator i;

        if ((i = std::find_if(mData.begin(), mData.end(), IndexCheck(rThisVariable.Key())))  != mData.end())
            return *static_cast<TDataType*>(i->second);

        #pragma omp critical
        {
        mData.push_back(ValueType(&rThisVariable,new TDataType(rThisVariable.Zero())));
        }

        return *static_cast<TDataType*>(mData.back().second);
    }

[riccardo]

"critical" is generally a performance killer... it is true however that it should never arrive there

what if instead we define a
FastGetValue which is unchecked and DOES NOT ALLOCATE. If the data is not there it throws an error instead.

[riccardo]

Expanding on my own answer,

Putting a critical would not be a solution. The point is that the code within the critical section you propose changes the database which is used in the first part...so your solution would not be effective.

I am convinced that the only option is to throw an error...

Riccardo

[Miguel Angel Celigueta]

You are right, Riccardo.
As a curiosity, do you think flush would do the job? (Admitting that it woud introduce a significant overhead).
The idea was taken from http://www.michaelsuess.net/publications/suess_leopold_common_mistakes_06.pdf (section 3.7).

 template<class TDataType> TDataType& GetValue(const Variable<TDataType>& rThisVariable)
    {
        typename ContainerType::iterator i;

        #pragma omp flush (mData)
        if ((i = std::find_if(mData.begin(), mData.end(), IndexCheck(rThisVariable.Key())))  != mData.end())
            return *static_cast<TDataType*>(i->second);

        #pragma omp critical
        {
        mData.push_back(ValueType(&rThisVariable,new TDataType(rThisVariable.Zero())));
        }

        return *static_cast<TDataType*>(mData.back().second);
    }

[riccardo]

Hi MA,

I am not sure of the answer. I believe that flushing would force a serialisation of the code, so I don t see it as a good idea, however I my be wrong on this.

If you check
http://stackoverflow.com/questions/19687233/explict-flush-direcitve-with-openmp-when-is-it-necessary-and-when-is-it-helpful

The folks in there advise to never use flush.

One solution that would work is to do

Node.SetLock()
...All code as today...
Node.UnsetLock()

This would add some overhead but should still be highly parallel, a bit as a conditional critical around all code that is only active if there is a conflict.

Ciao for now
Riccardo