Introduce a MutexProtected helper class for multithreading

[daljit46]

In C++, it's very easy to forget to lock a mutex before accessing the resource it protects. An example of this situation in our codebase is illustrated in #2755.
This "forgot to lock the mutex" problem can dealt with rather effectively by introducing a wrapper MutexProtected class that protects a resource that needs synchronisation across threads. Suppose you have code that looks like this:

Resource resource;
std::mutex mutex;
// to use the resource we need to always remember to lock the mutex
std::lock_guard<std::mutex> lock(mutex);
do_something(resource);

Then we can transform it into:

MutexProtected<Resource> protectedResource;
// to use the resource we must lock the mutex to access the resource
auto resourceGuard = protectedResource.lock();
do_something(*resourceGuard); // use dereference operator to access the resource

The implementation of MutexProtected can be something like this:

template<typename T>
class MutexProtected {
    std::mutex m_mutex;
    T m_object;
public:
    // Constructor forwards arguments to the constructor of the object
    template<typename...Args>
    MutexProtected(Args&&...args):
        m_object(std::forward<Args>(args)...)
    {
    }

    // A helper class that locks the mutex and provides access to the object
    class Guard {
        MutexProtected& m_protectedObject;
        std::lock_guard<std::mutex> m_lock;

    public:
        Guard(MutexProtected& protectedObject):
            m_protectedObject(protectedObject),
            m_lock(protectedObject.m_mutex)
        {
        }
        T& operator*() { return m_protectedObject.m_object; }
        T* operator->() { return m_protectedObject.m_object; }
    };

    Guard lock() { return Guard(*this); }
};

Using this class to protect synchronised values, we can ensure that a developer never forgets to lock a mutex and reducing the chances of a race condition. This pattern is somewhat similar to how mutexes are used in Rust and is available in number of C++ libraries like Facebook's Folly or Boost. I would like to add to this our codebase to help prevent race conditions.

[Lestropie]

Conceptually it makes sense and I like it. But I suspect there are cases where greater modification of existing code would be necessary to best make use of it.

Many multi-threading applications will currently do something like:

class Master {
  protected:
    vector<float> read_by_threads;
    float written_from_threads;
    std::mutex mutex;
}
class Slave {
  public:
    Slave (Master& master) :
        master (master),
        local_variable (0.0) { }
    Slave (const Slave& that) :
        master (that.master),
        local_variable (0.0) { }
    ~Slave() {
       std::lock_guard<std::mutex> lock (master.mutex);
       written_from_threads += local_variable;
    }
    void operator() () {
      // Does something with master.read_by_threads
      local_variable += X;
    }
  protected:
    Master& master;
    float local_variable;
}

Slave needs fast read-only access to Master.read_by_threads, and only a single mutex-locked write to master.written_from_threads.

I'd rather have something like:

class Slave::WriteBack {
  public:
    WriteBack (Master& master) :
        written_from_threads (0.0) { }
    WriteBack (const Shared&) = delete;
    ~WriteBack() {
      master.written_from_threads = written_from_threads;
    }
    void operator() (const Slave& that) {
      std::lock_guard<std::mutex> lock (mutex);
      written_from_threads += that.written_from_threads;
    }
  protected:
    Master& master;
    float written_from_threads;
    std::mutex mutex;
}
class Slave {
  public:
    Slave (Master& master) :
        master (master),
        write_back (new WriteBack (master)) { }
    Slave (const Slave& that) :
        master (that.master),
        write_back (that.write_back),
        local_variable (0.0) { }
    ~Slave() {
       *write_back += local_variable;
    }
    void operator() () {
      // Does something with master.read_by_threads
      local_variable += X;
    }
  protected:
    const Master& master;
    std::shared_ptr<WriteBack> write_back;
    float local_variable;
}

Crucially, your MutexProtected wrapper to gain access is wholly applicable to the Slave::WriteBack class, where only very rare write access is necessary. Whereas wrapping MutexProtected around Master would slow down very frequent read-only operations.

In addition:

• Slave now only has a const reference to Master.
• Explicit identification of which Master members are to be updated by that specific job.
• mutex doesn't have to live in master, it can instead be in Slave::WriteBack, which better ties it to that specific multi-threading job.

, all of which are I think beneficial.

It might be a case of grepping for mutexes and assessing the proportion of those for which such a wrapper is trivially applicable. Others might require structural changes to facilitate it.

[jdtournier]

👍 All for this type of abstraction, especially if it helps avoid issues down the track.

Quick thought though: it's very rare for developers to resort to manually handling a mutex - they'll typically rely on the ThreadedLoop or Thread::Queue constructs. As long as these are solid, that should avoid most issues. So while I like the idea, I don't think it'll be used all that widely throughout the codebase (though I'm sure there will be a few exceptions, of course). Definitely still worth doing though, even if we end up using it only within our own constructs - having these types of guarantees is definitely a Good Thing™.

On a slightly different note: Rob, where do you see the type of construct you mentioned? We have quite a few cases of a single Shared class being referenced by a bunch of worker classe, which update some value from the shared class in their destructor, but there is normally no need to mutex-protect that final update, as that's guaranteed to happen in the main thread anyway (at least it is when using our constructs). The order of execution when running a ThreadedLoop is:

main thread	worker threads
instantiate shared class
construct worker from shared class
copy-constuct additional workers from original worker
	run each worker's execute() method (which invokes the Functor's operator() method per voxel)
join all worker threads
invoke destructor for all workers, and update information in shared class at that point

Since all destructors are invoked in the main threads, once all worker threads have joined, there is no scope for race conditions, and no need to protect the operation with a mutex.

You may of course have other bits of code that operate slightly differently, but if so, it might be worth double-checking whether they really need that mutex in the final update, assuming each worker thread genuinely does not touch any shared variable during the execution of its own execute() or operator() method.

[daljit46]

So while I like the idea, I don't think it'll be used all that widely throughout the codebase

Yes, I agree. I would add that low level constructs like a mutex are not only not needed, but also fundamentally undesirable in application level code. Using higher level abstractions like queues is a much saner way of doing multithreading.
The scope for this class should largely be limited to our core library and the occasional place where manual synchronisation across threads is necessary.

[Lestropie]

Rob, where do you see the type of construct you mentioned?

Mostly I was thinking of SIFT2, since that's where I've invested time most recently. Statistical inference code does something similar also, and it's a simpler example to use here. Question is whether this lock is required. ThreadedQueue is not used, but run_queue() is. If it turns out that a lock isn't required there, then my first comment is largely a detraction from the Issue.

[daljit46]

Closed by e8232b2