Avoid creating new set of threads every frame

[kartikmohta]

Creating and destroying threads at each frame seems like a lot of overhead, maybe can keep the threads running and signal it when new data arrives.

[MaximilienNaveau]

I suggest we use boost for this:

#include <boost/thread/thread.hpp>
#include <boost/asio/io_service.hpp>

This allows you to spawn a pool of thread and then send them a pointer to a function to execute using boost::bind. It's very efficient.

[versatran01]

Does this approach require joining the thread at each frame?

I had a quick glance at the codebase and I found some questionable design choices that I don't quite understand. Specifically, why does the subject class need a mutex and why is it stored as a shared_ptr in a map. Why disable copy for Subject and KalmanFilter? They should have value semantics. And even the driver base class has an unnecessary mutex.

[versatran01]

I think the reason why everything is a shared_ptr is because there is a mutex in the Subject class. Again I fail to understand why this is needed. The only way a mutex is necessary is when functions like

    string subject_name =
      client->GetSubjectName(i).SubjectName;

will return duplicate subject_name, but that shouldn't be the case.

[MaximilienNaveau]

Does this approach require joining the thread at each frame?

Not that I know of, this is why I suggested it. I believe boot creates a pool of thread always active and you send them by a safe way a pointer to a method to execute. You can still wait until the method is done but it's not joining the thread, the thread goes idle until another method is sent to it.

[versatran01]

I'm not an expert in asio, but my understanding is one can just call io_service::post to give some work to the ios_service's internal thread pool? Is that what you are proposing here?

Also I'd love to get rid of all these random mutexes if possible.

My personal preference would be TBB of course, but using asio is also ok since we already depend on boost.

[MaximilienNaveau]

I'm not an expert in asio, but my understanding is one can just call io_service::post to give some work to the ios_service's internal thread pool? Is that what you are proposing here?

Yes, I suggest to do this:

// thread pool :
boost::asio::io_service io_service;
boost::asio::io_service::work io_work(io_service);
boost::thread_group threadpool ;
// add one thread (or more) to the thread pool
threadpool_.create_thread(boost::bind(&boost::asio::io_service::run, &io_service_));

// Make the thread work using post
io_service_.post(boost::bind(&my_method, a_class_object, some_arg1, some_arg2));

// end of program (in the destructor of a class for example)
io_service.stop();
threadpool.join_all();

I personally prefer boost as it's:

• a very standard dependency,
• cross platform
• usually already installed when using ROS1/2
• easy to use using CMake

[versatran01]

Cool, this looks good. If you can do another PR, I'd be happy to review and merge it. I think for this particular one just change the vector of threads to asio and thread_pool. It would also be nice to get a rough performance measure before and after. We will worry about mutexes later.

[MaximilienNaveau]

I won't have time to do this before end of June but I will try later on.

[MaximilienNaveau]

If I do this I could also update the mutex problem.

[versatran01]

There's no hurry, I'm not sure how many people in our lab are using this atm.

My quick scan through the code shows that there's one mutex per Subject, which makes it non-copyable. If we remove it we can just use Subject instead of SubjectPtr. There's also one in each driver class, which is also not doing anything useful. I'd say it's safe to remove both with very limited refactoring.