Mutex cleanup


(Dženan Zukić) #1

Should we remove MutexLock and friends (MutexLockHolder, ConditionVariable), and replace them by C++11 equivalents (std::mutex, std::scoped_lock, std::condition_variable)?

A bump in major version number coupled with a switch to C++11 is an excellent opportunity to do so.


(Bradley Lowekamp) #2

So as a philosophy, ITK has usually had a wrapper around system level things to enable more portable code and isolation ITK from changes and system differences. Therefore, perhaps we should just write a different back end to these ITK classes which is C++11 based.

TBB also has mutex’s and conditional variables, is it better to stick with one treading API then mix them?


(Bradley Lowekamp) #3

I was looking at itk::AtomicInt vs std::atomic it. We are not very consistent in which one we are using. We should stabbing consistency convention for using a C++11 or system level or itk level wrapper for these types of things.

Here is a comparable change done to itk::AtomicInt:

But we have the fooling occurrences of std::atomic:

Documentation/ITK5MigrationGuide.md:you might use instead std::atomic.
Documentation/ITK5MigrationGuide.md:After, using std::atomic:
Documentation/ITK5MigrationGuide.md:std::atomic m_NumVoxelsInsideMask;
Modules/Core/Common/include/itkMultiThreaderBase.h: std::atomic progress;
Modules/Core/Common/include/itkMultiThreaderBase.h: std::atomic pixelProgress;
Modules/Core/Common/src/itkTBBMultiThreader.cxx: std::atomic< SizeValueType > progress( 0 );
Modules/Core/Common/src/itkTBBMultiThreader.cxx: std::atomic pixelProgress = { 0 };

Particular of concert is the Migration Guide. Which do we recommend using? While there is the flexibility in remote modules, we should be consistent and clear especially in the Core group.


(Dženan Zukić) #4

itk::AtomicInt and itk::MutexLock had a lot of sense before C++11. Now they are just a duplication of standard library’s functionality, and therefore make no sense. I intend to rewrite itk::Mutex and friends to be wrappers for std::mutex etc. But we should also deprecate them, along with itk::AtomicInt and any other system abstractions which were needed in ITK before C++11.


(Matt McCormick) #5

At one time, there was a greater need for wrapping standard library functionality since it was not implemented or buggy across compilers. I do not think we need wrappers if the standard library implementations are available across platforms and toolchains.


(Bradley Lowekamp) #6

Do we have a list of things to be replaced by C++11 some place?


(Matt McCormick) #7

These should go in the ITK 5 migration guide.


(Bradley Lowekamp) #8

I was referring more to a prioritized and approved TODO rather than a complete list.


(Matt McCormick) #9

There is no such list that I am aware of. If there items to be replaced by C++11, they can be discussed here, on Discourse.


(Matt McCormick) #10

We could mark itk::AtomicInt as deprecated and use std::atomic everwhere. At this point std::atomic has successfully proven itself as a cross-platform implementation for itk::AtomicInt.


(Bradley Lowekamp) #11

There are a couple meta-programming items such as EnableIf, IsSame that have C++11 equivalents:
https://en.cppreference.com/w/cpp/types/enable_if
https://en.cppreference.com/w/cpp/types/is_same

Then the who constraints mostly could be replaced with static_assert and type traits. The old implementation is full of work arounds with old compilers and the compilation errors are not as clear.


(Matt McCormick) #12

:+1: std::enable_if and std::is_same are good candidates, too.


(Simon Alexander) #13

Not chiming in with anything useful, but think migration to at c++11 and cleaning up consistency is a good goal!


(Dženan Zukić) #14

Here is the patch. I got stuck at a crash in itkConditionVariable. Can anyone see what I am doing wrong there?


(Sean McBride) #15

If you are refactoring that code, I’d say it’s a must to try it all with Thread Sanitizer:

https://clang.llvm.org/docs/ThreadSanitizer.html

Sean


(Dženan Zukić) #16

I finally got around to this again. The latest code is here.

It looks like ITK’s Mutex and friends have an incompatible API with respect to std::mutex and friends. itk::MutexLock is recursive, whereas std::mutex is not. There is std::recursive_mutex, but std::condition_variable works only with plain std::mutex. So I don’t think we can re-implement ITK mutex infrastructure to be simple wrappers for std stuff. The problem is this public function. Whatever it returns needs to be usable by itk::ConditionVariable.

Now I think it is best if we move all the classes which duplicate functionality in C++11 standard library into a compatibility module. Should we create V4_Compatibility module or something similarly named, or put it in Deprecated?


(Dženan Zukić) #17

Thanks for this suggestion. Here is some useful output for the latest code:

WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=20236)
  Cycle in lock order graph: M931 (0x7ffc42ce0510) => M932 (0x7ffc42ce04e0) => M931

  Mutex M932 acquired here while holding mutex M931 in thread T1:
    #0 pthread_mutex_lock ??:? (ITKCommon1TestDriver+0x488727)
    #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/x86_64-linux-gnu/c++/5.4.0/bits/gthr-default.h:748 (ITKCommon1TestDriver+0x91755c)
    #2 std::mutex::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/mutex:135 (ITKCommon1TestDriver+0x91755c)
    #3 itk::SimpleMutexLock::Lock() /home/dzenan/ITK-git/Modules/Core/Common/include/itkMutexLock.h:66 (ITKCommon1TestDriver+0x91755c)
    #4 ConditionVariableTestIncCount(void*) /home/dzenan/ITK-git/Modules/Core/Common/test/itkConditionVariableTest.cxx:50 (ITKCommon1TestDriver+0x91755c)
    #5 ConditionVariableTestCallback(void*) /home/dzenan/ITK-git/Modules/Core/Common/test/itkConditionVariableTest.cxx:100 (ITKCommon1TestDriver+0x9177b2)
    #6 itk::MultiThreaderBase::SingleMethodProxy(void*) /home/dzenan/ITK-git/Modules/Core/Common/src/itkMultiThreaderBase.cxx:440 (libITKCommon-5.0.so.1+0xc2d8f)

    Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message

  Mutex M931 acquired here while holding mutex M932 in thread T1:
    #0 pthread_mutex_lock ??:? (ITKCommon1TestDriver+0x488727)
    #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/x86_64-linux-gnu/c++/5.4.0/bits/gthr-default.h:748 (ITKCommon1TestDriver+0x620de2)
    #2 std::mutex::lock() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/mutex:135 (ITKCommon1TestDriver+0x620de2)
    #3 itk::SimpleMutexLock::Unlock() /home/dzenan/ITK-git/Modules/Core/Common/include/itkMutexLock.h:91 (ITKCommon1TestDriver+0x620de2)
    #4 ConditionVariableTestIncCount(void*) /home/dzenan/ITK-git/Modules/Core/Common/test/itkConditionVariableTest.cxx:61 (ITKCommon1TestDriver+0x9175de)
    #5 ConditionVariableTestCallback(void*) /home/dzenan/ITK-git/Modules/Core/Common/test/itkConditionVariableTest.cxx:100 (ITKCommon1TestDriver+0x9177b2)
    #6 itk::MultiThreaderBase::SingleMethodProxy(void*) /home/dzenan/ITK-git/Modules/Core/Common/src/itkMultiThreaderBase.cxx:440 (libITKCommon-5.0.so.1+0xc2d8f)

(Matt McCormick) #18

Thanks for working on this @dzenanz.

I think these classes should be moved to ITKDeprecated. This will avoid confusion on their valid use and status. The transition is not difficult and is well-documented in the migration guide.


(Gordian) #19

Hi,

a follow up question and to wrap up the initial question:

Should I use std::mutex instead of itk::SimpleFastMutexLock in my ITK-4.13 code to make a (future) migration to ITKv5 easier?


(Bradley Lowekamp) #20

It depends on the compatibility requirements of your project.

If your ITK 4.13 code/project already depends or required C++11 then I would embrace those features in your project.

If however you are writing an ITK remote module or want your code/project to be fully compatible with all ITKv4 compilers then you would need to still use ITKv4 C++03 style and headers.