std::thread help!!!

Hello All,

I'm attempting to write some multithreaded code and it ain't goin' swell.

Okay, this should be all the relevant code you need :

Code:

---

mutex ins, surf, out;


void counter(int & inside, int & surface, int & outside, vector<particle> parts, tetra *root) {


      int x = 0;


      for (auto it = parts.begin(); it < parts.end(); ++it) {


        x = inside_tetra(root, &(*it));


        if (x == 0) {


            ins.lock();
            ++inside;
            ins.unlock();
        }


        if (x == -1) {


            out.lock();
            ++outside;
            out.unlock();
        }


        if (x > 0) {


            surf.lock();
            ++surface;
            surf.unlock();
        }
      }


      return;
  }

/* ... now we're in the main-loop of the code.. */

  thread *threads = new thread[num_threads];
  vector<particle> *particle_sets = new vector<particle>[num_threads];

  for (unsigned i=0; i < num_threads; ++i) {


      threads[i] = thread(counter, inside, surface, outside, particle_sets[i], root_tetra);
  }

---

Now, a first reaction would be, MutantJohn, you sexy beast, why are you allocating memory for threads and particle_sets. The answer is because doing otherwise does not comply with the ISO standard and I'm all about that.

Basically, my code won't compile and I have no idea why. Oh, and root_tetra is a valid allocation in memory. The output I'm getting is really weird, it's basically :

Code:

---

g++ -g -O3 -Wall -Wextra -std=c++11 -lpthread -pedantic `pkg-config --cflags --libs gsl` -c main.cpp
In file included from /usr/include/c++/4.8.2/thread:39:0,
                from structures.h:7,
                from main.cpp:1:
/usr/include/c++/4.8.2/functional: In instantiation of ‘struct std::_Bind_simple<void (*(int, int, int, std::vector<particle>, tetra*))(int&, int&, int&, std::vector<particle>, tetra*)>’:
/usr/include/c++/4.8.2/thread:137:47:  required from ‘std::thread::thread(_Callable&&, _Args&& ...) [with _Callable = void (&)(int&, int&, int&, std::vector<particle>, tetra*); _Args = {int&, int&, int&, std::vector<particle, std::allocator<particle> >&, tetra*&}]’
main.cpp:120:90:  required from here
/usr/include/c++/4.8.2/functional:1697:61: error: no type named ‘type’ in ‘class std::result_of<void (*(int, int, int, std::vector<particle>, tetra*))(int&, int&, int&, std::vector<particle>, tetra*)>’
      typedef typename result_of<_Callable(_Args...)>::type result_type;
                                                            ^
/usr/include/c++/4.8.2/functional:1727:9: error: no type named ‘type’ in ‘class std::result_of<void (*(int, int, int, std::vector<particle>, tetra*))(int&, int&, int&, std::vector<particle>, tetra*)>’
        _M_invoke(_Index_tuple<_Indices...>)
        ^
Makefile:18: recipe for target 'main.o' failed
make: *** [main.o] Error 1

---

Okay, just a normal function call will work.

This :

Code:

---

counter(inside, surface, outside, particle_sets[0], root_tetra);

---

will actually compile just fine when I'm not trying to launch threads.

Okay, I'm a moron. Or I learned something about the STL today. You cannot pass references as arguments when using std::threads. Instead, you should use pointer syntax because that will compile. Oh. My. God.

Sorry, everybody. Nothin' to see here T_T

Quote:

---

Originally Posted by MutantJohn

Okay, I'm a moron. Or I learned something about the STL today. You cannot pass references as arguments when using std::threads. Instead, you should use pointer syntax because that will compile. Oh. My. God.

---

Well, actually you can. It just won't do what you expect:

Code:

---

#include <thread>

void foo2(int&) {}

void foo(int& n)
{
        std::cout << "foo: n is " << n << ".\n";
        n = 10;
        std::cout << "foo: n is " << n << ".\n";

        std::thread thread(&foo2, n);
        thread.join();
}

int main()
{
        int n = 0;
        std::cout << "main: n is " << n << ".\n";
        std::thread thread(&foo, n);
        thread.join();
        std::cout << "main: n is " << n << ".\n";
}

---

Output:
main: n is 0.
foo: n is 0.
foo: n is 10.
main: n is 0.

To fix, change
std::thread thread(&foo, n);
to
std::thread thread(&foo, std::ref(n));

However, a couple of points:
You can simplify
for (auto it = parts.begin(); it < parts.end(); ++it)
to
for (const auto & val : parts)
(assuming you don't need the iterator)

You should not lock manually, so don't write

ins.lock();
++inside;
ins.unlock();

Instead, use a lock, ie unique_lock:

std::mutex m;
std::unique_lock<std::mutex> l(m);

This takes the RAII principle into account and unlocks upon destruction of the lock.
But if you are just going to increment a variable, you may want to use atomics instead.

std::atomic<int> MyAtomicInt(0);
++MyAtomicInt;

Why on earth are you manually allocating memory with new? You should be using a vector, eg:

std::vector<std::thread> v;
v.emplace_back(&foo, std::ref(n));
v[0].join();

You can use a 2D-vector too, or just a plain struct to group thread and thread data, so

std::vector<std::vector<particle>> v(N, std::vector<particle>(N));

Or a struct

Code:

---

struct Thread
{
    // TODO: Create constructor
    std::thread Thread;
    particle Particle;
}

std::vector<Thread> MyThreads;
MyThreads.emplace_back(...);

---