ukoethe / vigra

a generic C++ library for image analysis
http://ukoethe.github.io/vigra/
Other
412 stars 192 forks source link

fix thread safety in multi_fft.hxx and fftw3.hxx #95

Open ukoethe opened 12 years ago

ukoethe commented 12 years ago

a mutex lock is required around calls to the planner functions (see http://www.fftw.org/fftw3_doc/Thread-safety.html)

hmeine commented 12 years ago

Is thread safety an important goal for VIGRA functions, or should this be documented and left to the user? I assume you want this because the API of some functions hides the fact that the FFTW is involved, and it appears as if the functions worked on (thread-owned) image arrays only?!

ukoethe commented 12 years ago

Maybe it's easier to just document the behavior and tell people what to do for multi-threading. In any case, fourierTransform(), convolveFFT() etc. are currently not thread-safe because FFTW plan creation is not.

jschleic commented 12 years ago

I don't think, mutex locks are the direction to go. They block execution and thus reduce the overall speedup that can be achieved by parallelism. Documentation is good, but you can achieve thread-safety without mutex locks by re-using plans. I've implemented that as FFTFilter class: see https://github.com/jschleic/simple-STORM/blob/master/storm/fftfilter.hxx The constructor creates the plans needed and applyFourierFilter() only calls fftw_execute() that is thread safe.

Another possible API could be adding an optional PlanObject, so that a user can create a plan from the master thread via a function

plan = createForwardPlan(srcImageRange(exampleImage));

and pass this plan to the actual execute-function as optional object from different threads

void fourierTransform(srcUpperLeft, srcLowerRight, srcAcc,
                      FFTWComplexImage::traverser destUpperLeft, FFTWComplexImage::Accessor dest,
                      FFTWPlan plan = FFTWPlan());
ukoethe commented 12 years ago

But even if plans are reused, their creation must be protected by a mutex. BTW, the plan class you propose already exists in multi_fft.hxx.

jschleic commented 12 years ago

Oh, multi_fft.hxx in fact has all the functionality I wanted. But it's not thread-safe (even when locking the init functions), since the execute-functions modify class object variables realArray, realKernel etc. Thus execute() cannot be called simultaneously from different threads, if I understand the code correctly. Qt has a good section about reentrant vs. thread-safe: http://doc.qt.nokia.com/latest/threads-reentrancy.html.

Using multi_fft-functionality one would need a different FFTWPlan object for every thread instead of one shared object as in my proposal. Then you indeed need to lock the init functions.

But how would you implement a mutex lock? Is there a general concept for portable mutexes? As far as I know this depends on the threading library used. For example OpenMP would use "#pragma omp critical", Qt has a "QMutexLocker" class and pthreads have a function for mutexes. What functions are used for native Win32 threads? ;-)