Bug #784
threadsafety: Scott Meyers's advice about cached values
Description
The first 17 mins of the video EffectiveModernCpart6.mp4
by Scott Meyers are about correct threadsafe design of code which creates/accesses a cached value in a class.
His solution requires "advanced" features of C++ which are not present in C++98/C++03.
We need to follow his advice.
Related issues
History
#1 Updated by John Abbott over 8 years ago
We definitely want CoCoALib to be threadsafe. It mostly is when compiled with -DCoCoA_THREADSAFE_HACK
, but some sections of code are not (e.g. those where we use cached values such a G-bases).
I fear there is no portable solution using pure C++03/C++98, unless perhaps BOOST has some magic techniques -- perhaps investigate?
#2 Updated by John Abbott over 8 years ago
Here's an example of what Meyers suggests:
int MyClass::myGetValue() const { std::lock_guard<std::mutex> lock(myMutex); if (myValueAlreadyComputed) return myValue; myValue = ComputeValue(*this); myValueAlreadyComputed = true; return myValue; }
Note: MyClass
needs an extra field std::mutex myMutex;
Note: the whole function is under the lock_guard
Alternative implementation -- not sure which I prefer.
int MyClass::myGetValue() const { std::lock_guard<std::mutex> lock(myMutex); if (!myValueAlreadyComputed) { myValue = ComputeValue(*this); myValueAlreadyComputed = true; } return myValue; }
#3 Updated by Anna Maria Bigatti over 8 years ago
Victor Shoup said he made NTL threadsafe (using C++11)
#4 Updated by John Abbott over 8 years ago
- Status changed from New to In Progress
- % Done changed from 0 to 10
Bearing in mind the suggestion in issue #142 about using a compile-time switch to indicate whether we want threadsafe code, the example in comment 2 above should probably be written like this:
int MyClass::myGetValue() const { #ifdef CoCoA_THREADSAFE std::lock_guard<std::mutex> lock(myMutex); #endif if (!myValueAlreadyComputed) { myValue = ComputeValue(*this); myValueAlreadyComputed = true; } return myValue; }
The myMutex
data member could be with or without #ifdef...#endif
. It is probably a bit cleaner to use #ifdef
to suppress the data member when threadsafety is not needed (since the class would not have a useless data slot), but this may make the source code less readable. Perhaps if the class has several mutex members then they could all be lumped together in a single #ifdef...#endif
; this could be a reasonable compromise.
#5 Updated by Mario Albert over 8 years ago
I read a bit in Stroustrup C++ book. There I found a method std::call_once
(in <mutex>
). This method could be a nice solution to ensure threadsafetines. The only problem is that we have to change the code a bit more:
#ifdef CoCoA_THREADSAFE std::once_flag myValueFlag; #else bool myValueFlag; #end int myValue; void ComputeValue() const { // compute and set myValue myValue = 1; } int MyClass::myGetValue(const MyClass& cls) const { #ifdef CoCoA_THREADSAFE std::call_once(myValueFlag, ComputeValue, *this); #else if (!myValueFlag) { ComputeValue(*this); myValueFlag = true; } #endif return myValue; }
std::call_once
and std::once_flag myValueFlag
ensure that the method ComputeValue
is called only once. If another thread wants to call std::call_once(myValueFlag, ComputeValue, *this)
at the same time it waits until the first thread is finished and proceeds than as usual (but then everything should in a threadsafe state). If we call (std::call_cone(myValueFlag, ComputeValue, *this)
a second time ComputeValue
is not called and we directly jump to return myValue
. (http://www.cplusplus.com/reference/mutex/call_once/)
#6 Updated by John Abbott over 8 years ago
Mario and I are reading about some of the new features in C++ (v.11 and v.14). To me it seems that a "deferred" future
could be an interesting solution; and packaged_task
seems to be a neat way of wrapping up a future
. JAA is not sure if a packaged_task
can be "deferred".
#7 Updated by John Abbott over 8 years ago
Below is what I think would be the solution using async
and shared_future
. It requires a single data-member of type shared_future<value_t>
which is initialized using async
in the class ctor.
MyClass::MyClass(): mySharedFutureForValue(async(launch::deferred, ComputeValue, this)) { ... } int MyClass::myGetValue() const { return mySharedFutureForValue.get(); }
To be honest this code looks to be quite compact. One aspect I do not like too much is that the function which is called is named in the ctor, rather than in the accessor fn (which is where it will be called since I chose the "deferred policy").
NOTE (16:52) a simplistic example using this approach behaved as expected :-)
I had to include the std header <future>
for compilation to work.
#8 Updated by John Abbott over 8 years ago
Following on from the previous comment... here is how we can test whether the value has already been computed:
bool MyClass::IsKnownValue() const { const std::chrono::duration<int> ZERO = std::chrono::duration<int>::zero(); return (mySharedFutureForValue.wait_for(ZERO) == future_status::ready); }
Note that the status could change at any time if the value is actively being computed by another thread. JAA is not sure what implications this might have for us.
It might be possible to know whether the value is actively being computed (by another thread), by checking whether the status is future_status::timeout
. But how is this (practically) different from status_timeout::deferred
? Either way the value was not known in instant ago (when we asked for the status), but it could become available in the very near future (or it could take ages to compute).
JAA currently thinks that there is nothing useful to be gained from distinguishing timeout
from deferred
(in this context).
#9 Updated by John Abbott over 5 years ago
- Related to Design #1259: ThreadsafeCounter is now obsolete? added