More Threading issue

blowekamp · November 20, 2017, 9:06pm

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…
@dzenanz

dzenanz · November 20, 2017, 10:11pm

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.

blowekamp · November 21, 2017, 3:03pm

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

github.com

InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/src/itkMultiThreader.cxx#L170


ThreadIdType MultiThreader::GetGlobalDefaultNumberOfThreads()
{
if( m_GlobalDefaultNumberOfThreads == 0 ) //need to initialize
  {
  m_GlobalDefaultNumberOfThreads = ThreadPool::GetGlobalDefaultNumberOfThreads();
  }
return m_GlobalDefaultNumberOfThreads;
}


MultiThreader::MultiThreader() :
m_ThreadPool( ThreadPool::GetInstance() ),
m_UseThreadPool( MultiThreader::GetGlobalDefaultUseThreadPool() )
{
for( ThreadIdType i = 0; i < ITK_MAX_THREADS; ++i )
  {
  m_ThreadInfoArray[i].ThreadID           = i;
  m_ThreadInfoArray[i].ActiveFlag         = ITK_NULLPTR;
  m_ThreadInfoArray[i].ActiveFlagLock     = ITK_NULLPTR;


  m_MultipleMethod[i]                     = ITK_NULLPTR;
  m_MultipleData[i]                       = ITK_NULLPTR;

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

github.com

InsightSoftwareConsortium/ITK/blob/master/Modules/Core/Common/src/itkThreadPool.cxx#L67


m_ExceptionOccurred(false)
{
MutexLockHolder<SimpleFastMutexLock> mutexHolder(m_Mutex);
m_ThreadPoolInstance = this; //threads need this
m_ThreadPoolInstance->UnRegister(); // Remove extra reference
PlatformCreate(m_ThreadsSemaphore);
}


ThreadIdType
ThreadPool
::GetGlobalDefaultNumberOfThreads()
{
ThreadIdType threadCount = 0;
/* The ITK_NUMBER_OF_THREADS_ENV_LIST contains is an
 * environmental variable that holds a ':' separated
 * list of environmental variables that whould be
 * queried in order for setting the m_GlobalMaximumNumberOfThreads.
 *
 * This is intended to be a mechanism suitable to easy
 * runtime modification to ease using the proper number
 * of threads for load balancing batch processing

This seems potentially problematic to me…

dzenanz · November 21, 2017, 3:19pm

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

blowekamp · November 21, 2017, 3:38pm

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.

blowekamp · November 21, 2017, 5:22pm

Here is a patch which addresses the specific issue I encountered:
http://review.source.kitware.com/#/c/22813/

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.

dzenanz · November 21, 2017, 5:44pm

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.

blowekamp · November 21, 2017, 6:31pm

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