Mutex cleanup

Engineering
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.

1 Like
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?

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.

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.

1 Like
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.

6

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

7

These should go in the ITK 5 migration guide.

8

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

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.

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.

1 Like
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.

2 Likes
12

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

1 Like
13

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

1 Like
14

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

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

2 Likes
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?

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)
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.

2 Likes
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?

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.

1 Like