6

I have read many of the questions already answered that relate to this but none of them gave me a clear understanding of which I should be using when I have multiple writers but a single reader. The code below is a contrived example of what I'm talking about.

struct StateInfo {
    long wd{};
    uint32_t perc{};
};

class Blah 
{
    const int numDevices = getDevices();
    std::shared_mutex sharedMutexSI_;
    vector<StateInfo> stateInfo;
public:

    Blah() : stateInfo(numDevices){};

    void writers(StateInfo &newSi, const int i)
    {
        std::shared_lock<std::shared_mutex> _MULTIPLE(sharedMutexSI_);
        stateInfo[i] = newSi;
    }

    StateInfo reader(const int i)
    {
        std::lock_guard<std::shared_mutex> _EXCLUSIVE(sharedMutexSI_);
        return stateInfo[i];
    }
};

The situation is that multiple writers may simultaneously update the stateInfo vector but never the same item in the vector as i is unique to each thread. A single reader thread may at any time try to read any of the vector items.

  1. Is the above code correct in avoiding race conditions?

  2. Is lock_guard the right one to use or should I use scoped_lock or unique_lock?

poby
  • 1,572
  • 15
  • 39
  • There is no difference between `lock_guard`, `scoped_lock` and `unique_lock` in this use case – Alan Birtles Jul 25 '20 at 08:07
  • Depending on the size of `long` on your platform you could change `StateInfo` to `std::atomic` and avoid locking all together – Alan Birtles Jul 25 '20 at 08:09
  • @AlanBirtles so `lock_guard` is the right one as it is the simplest and the others don't add anything useful in this case? – poby Jul 25 '20 at 08:09
  • Seems in 17 lock_guard is unfavoured or supersetted by scoped_lock? – Carlos Jul 25 '20 at 08:10
  • Both, `lock_guard` and `scoped_lock` are fine, `unique_lock` is an overkill. – Evg Jul 25 '20 at 08:12
  • My understanding is that `lock_guard` is the simplest and if I don't need the functionality of `scoped_lock`, then I should stick with the simplest? – poby Jul 25 '20 at 08:15
  • 1
    At least in libstdc++, for a single mutex `lock_guard` and `scoped_lock` are identical in their implementation. – Evg Jul 25 '20 at 08:22
  • If (number of readers) times (number of writers) to X is positive (i.e. `> 0`) all readers and writers of X must be synchronised. Then it depends on how many synchronisation primitives (e.g. mutex) is acceptable. With exactly one primitive, all readers and writers must use it (so all writers synchronise with each other and the reader). If `n` mutexes (`n` = number of elements) is acceptable, then each writer can use its own primitive (and not synchronise with other writers) but the reader must grab the corresponding primitive before reading each element (i.e. synchronise with all writers). – Peter Jul 25 '20 at 08:22
  • 1
    Regarding `lock_guard`, `scoped_lock` and `unique_lock`: see a dedicated question with answers [here](https://stackoverflow.com/q/43019598/509868). – anatolyg Jul 25 '20 at 08:25
  • You use `shared_lock` for writing and `scope_lock` for reading? That is opposite of what you want. Use `shared_lock` for reading and `scope_lock` for writing. – ALX23z Jul 25 '20 at 08:45
  • 1
    @ALX23z, OP has one reader and several writers, and there are no data races between writers. – Evg Jul 25 '20 at 08:52
  • 1
    @Evg you mean he has writers dedicated to each index? This is a presumption made outside of class which isn't good in general. In this case it might be preferable to have each state info have its own `std::mutex` or just make the `StateInfo` atomic. In any case the class suffers from false-sharing so I am not sure if shared lock helps much. – ALX23z Jul 25 '20 at 09:12
  • @ALX23z, yeah, that's what Alan Birtles and Peter suggested above. – Evg Jul 25 '20 at 09:15

1 Answers1

2

To summarize what was already written in comments:

Yes, the code is correct. However, it may be inefficient, because it disallows reading from any array element while writing to another array element. You might want to do more fine-grained synchronization by using a mutex for each array element.

class Blah 
{
    const int numDevices = getDevices();
    std::vector<std::mutex> mutexes;
    std::vector<StateInfo> stateInfo;
public:

    Blah() : mutexes(numDevices), stateInfo(numDevices){}

    void writers(StateInfo &newSi, const int i)
    {
        std::lock_guard<std::mutex> guard(mutexes[i]);
        stateInfo[i] = newSi;
    }

    StateInfo reader(const int i)
    {
        std::lock_guard<std::mutex> guard(mutexes[i]);
        return stateInfo[i];
    }
};
anatolyg
  • 26,506
  • 9
  • 60
  • 134