Usage of TBB parallel_for_each() with reduction to Atomic counters

Question

Here is my atomic counter class used for a TBB code.

#include <atomic>
template <typename T = int>
struct AtomicCounter {

private:
std::atomic<T> value;
std::atomic<T> num_inter;

public:
void put_inter(T niter) {
T x = num_inter.fetch_add(niter);
}

T get_inter() { return num_inter.load(); }

void increment() {  ++value; }
void put(T data) {  value.fetch_add(data) ; // ignore the old value returned }
void decrement() {  --value; }
T    get()       { return value.load(); }

};

I am using a parallel foreach loop in which every thread has its own local counter value and it updates the global counter object using put function of the AtomicCounter class. The function object for parallel foreach takes this global counter as a reference.

template<typename counter_t>
struct my_functor {
 public:
  my_functor(){}
  my_functor(counter_t &c):m_c(c){ } 

  template<typename label_t>
  void operator()(label_t label) const {

  // do some work --> local counter

   m_c.put(local_value); // put the local counter value in the global counter 
  } 

  private:
    counter_t& m_c;
    mutable int local_value; //local counter value  

  }

   // for TBB 
 //declare an object of this class as a global counter.

 AtmoicCounter<int> c;
 my_functor<atomicCounter<int>>  func(c) //functor object
 parallel_for_each( mywork.begin(), mywork.end(), func)    //TBB parallel for each

SO basically, each thread updates the global counter. Is there anything wrong with the atomicCounter class where i declare the std::atomic member ? I am using GCC 4.7.2 with C++11 features and intel TBB 4.2

Thanks!

Answer 1

I don't see any problem in declarations. If you are not sure in GCC atomics for some reason, use TBB atomics in which I'm sure.

But where I see the problem is usage of mutable keyword in order to access fields from the const functor. mutable is often mark of a bad design. And in parallel programming its usage is especially error-prone. If a method which is called in parallel marked as const, it often means that it will be called in parallel on the same instance of the functor class. Thus you have a data-race on the mutable field.

Instead of breaking the const restrictions, you can move local_value into the scope of operator(), or try thread-local containers tbb::combinable or tbb::enumerable_thread_specific to cache the thread-specific partial values, or apply the parallel reduction algorithm tbb::parallel_reduce where you can safely store partial results in functor's body.