Is my double lock check implemented correctly?

Question

An example in Meyers' book Effective Modern C ++, paragraph 16.

in caching a class with expensive int evaluation, you can try using the std :: atomic avriables pair instead of the mutex:

class Widget { public: int magicValue() const { if (cachedValid) { return cachedValue; } else { auto val1 = expensiveComputation1(); auto val2 = expensiveComputation2(); cachedValue = va1 + val2; cacheValid = true; return cachedValue; } } private: mutable std::atomic<bool> cacheValid { false }; mutable std::atomic<int> cachedValue; }; 

This will work, but sometimes it will work a lot harder than it should. Consumer: the stream calls Widget :: magicValue, sees cacheValid as false, performs two expensive calculations and assigns their sum tocachedValud. At this point, the second thread Widget :: magicValue also sees cacheValid as false and thus carries over from the same expensive calculations as the first thread finished.

Then he gives a solution with a mutex:

 class Widget { public: int magicValue() const { std::lock_guard<std::mutex> guard(m); if (cacheValid) { return cachedValue; } else { auto val1 = expensiveComputation1(); auto val2 = expensiveComputation2(); cachedValue = va1 + val2; cacheValid = true; return cachedValue; } } private: mutable std::mutex m; mutable bool cacheValid { false }; mutable int cachedValue; }; 

But I think that the solution is not so effective, I believe that to combine a mutex and an atom, it is necessary to create a Double-Checked Locking Pattern , as shown below.

 class Widget { public: int magicValue() const { if (!cacheValid) { std::lock_guard<std::mutex> guard(m); if (!cacheValid) { auto val1 = expensiveComputation1(); auto val2 = expensiveComputation2(); cachedValue = va1 + val2; cacheValid = true; } } return cachedValue; } private: mutable std::mutex m; mutable std::atomic<bool> cacheValid { false }; mutable std::atomic<int> cachedValue; }; 

Because I'm new to multi-threaded programming, so I want to know:

• Is my code correct?
• Is the performance different?

EDIT:

---

The code is fixed. if (! cachedValue) → if (! cacheValid)

Answer 1

Is my code correct?

Yes. The Double-Checked Locking Pattern app is correctly applied. But see below for some improvements.

Is the performance different?

Compared to the fully blocked option (the second in your post), it basically has better performance until magicValue() is called only once (but even in this case, the performance loss is negligible).

Compared to the unoccupied option (1st in your post), your code shows better performance until the cost calculation is faster than waiting for the mutex.

For example, a sum of 10 values ​​is (usually) faster than waiting on a mutex. In this case, the 1st option is preferred. On the other hand, 10 reads from a file are slower than waiting for a mutex, so your option is better than the first.

---

In fact, there are simple improvisations of your code that make it faster (at least on some machines) and improve your understanding of the code:

• cachedValue variable does not require atomic semantics at all. It is protected by the cacheValid flag, which atomicity does all the work. Moreover, one atomic flag can protect several non-atomic values.

• Also, as noted by fooobar.com/questions/986555 / ... in this answer, when accessing the cacheValid flag you don’t need the sequence order (which is the default when you just read or write an atomic variable), the release order is sufficient.

---

 class Widget { public: int magicValue() const { //'Acquire' semantic when read flag. if (!cacheValid.load(std::memory_order_acquire)) { std::lock_guard<std::mutex> guard(m); // Reading flag under mutex locked doesn't require any memory order. if (!cacheValid.load(std::memory_order_relaxed)) { auto val1 = expensiveComputation1(); auto val2 = expensiveComputation2(); cachedValue = va1 + val2; // 'Release' semantic when write flag cacheValid.store(true, std::memory_order_release); } } return cachedValue; } private: mutable std::mutex m; mutable std::atomic<bool> cacheValid { false }; mutable int cachedValue; // Atomic isn't needed here. }; 

Answer 2

As noted in HappyCactus, the second if (!cachedValue) should be if (!cachedValid) . Aside from this typo, I think your Double-Checked Locking Pattern demo is correct. However, I think there is no need to use std::atomic in cachedValue . The only place cachedValue is cachedValue = va1 + val2; is cachedValue = va1 + val2; . Before it is completed, no thread will ever reach the return cachedValue; which is the only cachedValue place. Therefore, it is impossible for writing and reading to be parallel. And there are no problems with parallel readings.

Answer 3

You can make your decision a little more efficient by reducing memory requirements. This does not require the memory order of a sequential sequence of sequences for atomic operations.

The difference in performance may be negligible on x86, but noticeable on ARM, because the order of the sequential memory sequence is expensive on ARM. For details, see the "Strong" and "weak" models of hardware memory from Herb Sutter .

Proposed Changes:

 class Widget { public: int magicValue() const { if (cachedValid.load(std::memory_order_acquire)) { // Acquire semantics. return cachedValue; } else { auto val1 = expensiveComputation1(); auto val2 = expensiveComputation2(); cachedValue = va1 + val2; // Non-atomic write. // Release semantics. // Prevents compiler and CPU store reordering. // Makes this and preceding stores by this thread visible to other threads. cachedValid.store(true, std::memory_order_release); return cachedValue; } } private: mutable std::atomic<bool> cacheValid { false }; mutable int cachedValue; // Non-atomic. }; 

Answer 4

Wrong:

 int magicValue() const { if (!cachedValid) { // this part is unprotected, what if a second thread evaluates // the previous test when this first is here? it behaves // exactly like in the first example. std::lock_guard<std::mutex> guard(m); if (!cachedValue) { auto val1 = expensiveComputation1(); auto val2 = expensiveComputation2(); cachedValue = va1 + val2; cachedValid = true; } } return cachedValue;