More Threading issue

The server I am working on has 2 socket with 44 physical cores configured to run with hyper-threading. My general experience with hyper-threading is: It does not hurt the through put, and some times the feature does improve the performance. So I am running with what ITK should default to as 88 threads.

On the master branch, I noticed that the test SliceImageFilterTests.PhysicalPoint1 was running particularly slow.

The test takes 270 seconds when ctest -V -R SliceImageFilterTests.PhysicalPoint1, but taking just 6.4 seconds when ITK_GLOBAL_DEFAULT_NUMBER_OF_THREADS=88 ctest -V -R SliceImageFilterTests.PhysicalPoint1 is run.

Similarly, when running the whole test suit it took 305 seconds with running just ctest -j 80, while just 142 seconds with ITK_GLOBAL_DEFAULT_NUMBER_OF_THREADS=88 ctest -j 80. It’s my expectation that these should both be running filters with a 88 threads by default.

Something funny is happening…

I don’t know why 88 wouldn’t be the default number of threads. On my computer, ctest -C RelWithDebInfo -V -R SliceImageFilterTests.PhysicalPoint1 runs in about 2.5 seconds. And it is being run with 4 threads, as expected.

Can you trace ThreadPool::GetGlobalDefaultNumberOfThreads() in \Modules\Core\Common\src\itkThreadPool.cxx? The threadCount variable should be 88. If it is not, please explain what is causing that behavior.

So looking at the code closer it looks like the TheadPool is always created:

and the thread pool always create all the thread in the constructor:

This seems potentially problematic to me…

Should I make it create threads the first time AddWork is invoked? Or create “up to N” threads as needed in AddWork (N=GlobalDefaultNumberOfThreads)?

For the default case when the thread pool is not being used it should not be created.

Consider wrapping the MultiTheader’s usage of the m_ThreadPool with a getter method. Then the thread pool does not have to be constructed in the MultiThread’s constructor, it can be constructor in the getter if and when it’s needed.

I hacked not constructing the ThreadPool, and it did not solve my reported problem.

Here is a patch which addresses the specific issue I encountered:

The motivations for the modification to the GlobalDefaultNumberOfThreads is not clear to me. Specifically, I don’t know when the one method was moved to the TheadPool from the multi-threader.

I moved it during the big thread pool rewrite. The move was done to avoid circular dependency. Without moving that method to thread pool, ThreadPool would also depend on MultiThreader.

Yes. The separation between the two classes is not very clean right now.