Simple example program that uses tbb::queueing mutex inside a tbb::parallel_for does not compile

Question

Here is a toy example I'm playing with to learn how to use TBB. Parallel::operator() should run in parallel, but it has a critical region that should only be accessed by a single processor at a time, so the message it prints does not get scrambled. My problem is that it fails to compile and the compiler message isn't really helping me much. What am I doing wrong?

Also, is this considered the proper way to implement a mutex inside a parallel_for?

#include <iostream>
#include <vector>
#include <cmath>
#include <tbb/tbb.h>

typedef tbb::queuing_mutex Mutex;

struct Parallel
{
    Mutex mutex;
    std::vector<int> * values;

    Parallel(std::vector<int> * values_) : values(values_) {}

    void operator()( tbb::blocked_range< unsigned int > & range ) const {
        for(unsigned int i = range.begin(); i < range.end(); ++i) {
            {
                Mutex::scoped_lock lock(mutex);
                if ( (*values)[i] > 40)
                {
                    std::cout << "NO SCRAMBLING ALLOWED!\n";
                    std::cout.flush();
                }
                lock.release();
            }
        }
    }
};

int main() {
    const int someValue = 20000;

    std::vector<int> data(someValue);
    for(int i = 0; i < someValue; ++i) {
        data[i] = std::rand();
    }

    tbb::parallel_for( tbb::blocked_range<unsigned int>(0, data.size()),
                       Parallel(&data) );
}

Bellow is the error message:

/path-to-src/main.cpp: In member function 'void Parallel::operator()(tbb::blocked_range<unsigned int>&) const':
/path-to-src/main.cpp:20:46: error: no matching function for call to 'tbb::queuing_mutex::scoped_lock::scoped_lock(const Mutex&)'
/path-to-src/main.cpp:20:46: note: candidates are:
In file included from /usr/include/tbb/tbb.h:65:0,
                 from /path-to-src/main.cpp:4:
/usr/include/tbb/queuing_mutex.h:80:9: note: tbb::queuing_mutex::scoped_lock::scoped_lock(tbb::queuing_mutex&)
/usr/include/tbb/queuing_mutex.h:80:9: note:   no known conversion for argument 1 from 'const Mutex {aka const tbb::queuing_mutex}' to 'tbb::queuing_mutex&'
/usr/include/tbb/queuing_mutex.h:77:9: note: tbb::queuing_mutex::scoped_lock::scoped_lock()
/usr/include/tbb/queuing_mutex.h:77:9: note:   candidate expects 0 arguments, 1 provided
/usr/include/tbb/queuing_mutex.h:66:11: note: tbb::queuing_mutex::scoped_lock::scoped_lock(const tbb::queuing_mutex::scoped_lock&)
/usr/include/tbb/queuing_mutex.h:66:11: note:   no known conversion for argument 1 from 'const Mutex {aka const tbb::queuing_mutex}' to 'const tbb::queuing_mutex::scoped_lock&'

Answer 1

tbb::parallel_for was designed to not compile the example as written. It's protecting against a bug in the code. The range form of tbb::parallel_for copies the functor by value into multiple task objects. Hence each task will have a separate copy of mutex, and thus the mutex will not provide the intended synchronization.

The way to fix the code is declare the mutex outside of struct Parallel, and pass it via a pointer, similar to the pointer for "values".

Answer 2

Everything is correct, except that the mutex must be mutable in order for it to be used in a lock. Moreover, releasing the lock is not necessary.

typedef tbb::queuing_mutex Mutex;

struct Parallel
{
    mutable Mutex mutex;              // mutable so that we can use it in const member
    std::vector<int> * values;

    Parallel(std::vector<int> * values_) : values(values_) {}

    // note: this is a const member
    void operator()( tbb::blocked_range< unsigned int > & range ) const
    {
        for(unsigned int i = range.begin(); i < range.end(); ++i)
        {
            Mutex::scoped_lock lock(mutex);       // requires non-const argument
            if ( (*values)[i] > 40)
            {
                std::cout << "NO SCRAMBLING ALLOWED!\n";
                std::cout.flush();
            }
            // no need to release the lock: the destructor will do that for you
        }
    }
};

Alternatively, you can, of course, make the member function non-const.