-
-
Notifications
You must be signed in to change notification settings - Fork 686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: fixed race condition setting m_ANTSAssociate #4625
Conversation
@dzenanz @hjmjohnson @N-Dekker I don't know this code at all, but I noticed the existence of BTW I've created a nightly cdash submission than uses Thread Sanitizer: https://open.cdash.org/viewTest.php?onlyfailed&buildid=9578838 I will triage the other test failures... |
/* call base method */ | ||
/* Store the casted pointer to avoid dynamic casting in tight loops. */ | ||
this->m_ANTSAssociate = dynamic_cast<TNeighborhoodCorrelationMetric *>(this->m_Associate); | ||
if (this->m_ANTSAssociate == nullptr) | ||
{ | ||
itkExceptionMacro("Dynamic casting of associate pointer failed."); | ||
} | ||
std::call_once(this->m_ANTSAssociateOnceFlag, [this]() { | ||
/* Store the casted pointer to avoid dynamic casting in tight loops. */ | ||
this->m_ANTSAssociate = dynamic_cast<TNeighborhoodCorrelationMetric *>(this->m_Associate); | ||
if (this->m_ANTSAssociate == nullptr) | ||
{ | ||
itkExceptionMacro("Dynamic casting of associate pointer failed."); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you found this potential race condition. 👍 I'm sorry I don't know the code either, and I find it hard to understand. 🤷 At
Line 42 in 13ccff6
std::call_once(this->m_ANTSAssociateOnceFlag, [this, &associate]() { this->m_ANTSAssociate = associate; }); |
m_ANTSAssociateOnceFlag
apparently ensures that m_ANTSAssociate = associate
is only executed once during the lifetime of this member variable. Which means that it cannot be reset to a different "associate" anymore, during its lifetime. Is that OK?
It appears that m_Associate
is a protected member of DomainThreader
. It is assigned in DomainThreader::Execute
:
ITK/Modules/Core/Common/include/itkDomainThreader.hxx
Lines 54 to 57 in f408b77
void | |
DomainThreader<TDomainPartitioner, TAssociate>::Execute(TAssociate * enclosingClass, const DomainType & completeDomain) | |
{ | |
this->m_Associate = enclosingClass; |
So then I guess the question is: could this "threader" be executed multiple times for different "associates"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that a mutex (member variable)+ lock_guard variables (locally) would be more appropriate than call_once + once_flag, in this case. But again, I don't know the code either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then again, it seems a bit "overdone" to have a separate data member (m_ANTSAssociate), just to store the result of dynamic_cast<TNeighborhoodCorrelationMetric *>(Superclass::m_Associate)
, and then try to keep this data member up-to-date and thread-safe.
I mean: apparently the (original) code tries to avoid expensive dynamic_cast's. But extra member data and mutex locking isn't for free either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The this->m_ANTSAssociate
should be change to just a function level variable with the results of the dynamic cast? This seems like it may have been an "optimization" which is not worth the complexity, with some unexpected cost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanm Looks like m_ANTSAssociate
was actually introduced one month before songgang's commit already! Commit 9614409 authored by Michael Stauffer and committed by Hans (@hjmjohnson) has:
Lines 135 to 137 in 9614409
/** Internal pointer to the metric object in use by this threader. | |
* This will avoid costly dynamic casting in tight loops. */ | |
TNeighborhoodCorrelationMetric * m_ANTSAssociate; |
And its commit text says:
Remove calls to dynamic_cast in image metric threaders. Replace with cached pointer to metric object. Speed gain of several %.
Now I wonder if that speed gain will still be there when thread safety is taken into account 🤷 . Moreover, new compilers may yield different performance results anyway!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked a little more...
The string Store the casted pointer to avoid dynamic casting in tight loops
appears 7 times, so seems, at least at one time, people thought it was a useful optimization (or got copy-pasted :))
So then I guess the question is: could this "threader" be executed multiple times for different "associates"?
I think the answer is no. The string m_Associate
"only" appears 108 times, and almost always it's read, not written. I can't find a setter for the ivar in fact. Only set is in DomainThreader<TDomainPartitioner, TAssociate>::Execute
.
Anyone have further thoughts on this one? |
Found with thread sanitizer (TSan) running the `itkANTSNeighborhoodCorrelationImageToImageRegistrationTest` unit test. There was already a `m_ANTSAssociateOnceFlag` flag for use with `std::call_once`, so I just used that in this place too (it's already used elsewhere). Partial backtrace from TSan: ``` WARNING: ThreadSanitizer: data race (pid=79176) Write of size 8 at 0x000108f02240 by thread T2: #0 void itk::ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>, itk::ANTSNeighborhoodCorrelationImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreadedExecution_impl<itk::ThreadedIndexedContainerPartitioner>(itk::IdentityHelper<itk::ThreadedIndexedContainerPartitioner>, itk::Index<2u> const&, unsigned int) itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.hxx:118 (ITKMetricsv4TestDriver:arm64+0x1004bfe50) #1 itk::ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>, itk::ANTSNeighborhoodCorrelationImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreadedExecution(itk::Index<2u> const&, unsigned int) itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.h:235 (ITKMetricsv4TestDriver:arm64+0x1004bf9c0) #2 itk::DomainThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreaderCallback(void*) itkDomainThreader.hxx:123 (ITKMetricsv4TestDriver:arm64+0x100259b6c) #3 std::__1::future<std::__1::invoke_result<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>::type> itk::ThreadPool::AddWork<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&)::'lambda'()::operator()() const itkThreadPool.h:92 (ITKMetricsv4TestDriver:arm64+0x1007d3228) #4 decltype(std::declval<void* (*&)(void*)>()(std::declval<itk::PoolMultiThreader::ThreadPoolInfoStruct*>())) std::__1::__invoke[abi:v160006]<std::__1::future<std::__1::invoke_result<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>::type> itk::ThreadPool::AddWork<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&)::'lambda'()&>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&) invoke.h:394 (ITKMetricsv4TestDriver:arm64+0x1007d31a4) Previous write of size 8 at 0x000108f02240 by thread T14: #0 void itk::ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>, itk::ANTSNeighborhoodCorrelationImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreadedExecution_impl<itk::ThreadedIndexedContainerPartitioner>(itk::IdentityHelper<itk::ThreadedIndexedContainerPartitioner>, itk::Index<2u> const&, unsigned int) itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.hxx:118 (ITKMetricsv4TestDriver:arm64+0x1004bfe50) #1 itk::ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>, itk::ANTSNeighborhoodCorrelationImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreadedExecution(itk::Index<2u> const&, unsigned int) itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.h:235 (ITKMetricsv4TestDriver:arm64+0x1004bf9c0) #2 itk::DomainThreader<itk::ThreadedIndexedContainerPartitioner, itk::ImageToImageMetricv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double, itk::DefaultImageToImageMetricTraitsv4<itk::Image<double, 2u>, itk::Image<double, 2u>, itk::Image<double, 2u>, double>>>::ThreaderCallback(void*) itkDomainThreader.hxx:123 (ITKMetricsv4TestDriver:arm64+0x100259b6c) #3 std::__1::future<std::__1::invoke_result<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>::type> itk::ThreadPool::AddWork<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&)::'lambda'()::operator()() const itkThreadPool.h:92 (ITKMetricsv4TestDriver:arm64+0x1007d3228) #4 decltype(std::declval<void* (*&)(void*)>()(std::declval<itk::PoolMultiThreader::ThreadPoolInfoStruct*>())) std::__1::__invoke[abi:v160006]<std::__1::future<std::__1::invoke_result<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>::type> itk::ThreadPool::AddWork<void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&)::'lambda'()&>(void* (*&)(void*), itk::PoolMultiThreader::ThreadPoolInfoStruct*&&) invoke.h:394 (ITKMetricsv4TestDriver:arm64+0x1007d31a4) ```
Friendly ping. @dzenanz @N-Dekker @hjmjohnson @thewtex any thoughts on this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @seanm . LGTM
In the big picture, it would be a major improvement to migrate this code to modern C++ lambda's, use thread_local
, etc. in terms of readability, maybe performance, and immunity to some of these subtle issues like race conditions.
I don't actually use this class, so while I agree, realistically I won't have time to volunteer for that. :) What I have been trying to spend a bit of time on though is get all ITK tests passing with TSan. 10 to go: https://open.cdash.org/viewTest.php?onlyfailed&buildid=9719623 |
@thewtex so do you think this is mergeable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanm yes, thank you!
Found with thread sanitizer (TSan) running the
itkANTSNeighborhoodCorrelationImageToImageRegistrationTest
unit test.There was already a
m_ANTSAssociateOnceFlag
flag for use withstd::call_once
, so I just used that in this place too (it's already used elsewhere).Partial backtrace from TSan:
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.