Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BUG: fixed race condition setting m_ANTSAssociate #4625

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

BUG: fixed race condition setting m_ANTSAssociate #4625

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions ...clude/itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,16 @@ ANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader<
const DomainType & domain,
const ThreadIdType threadId)
{
/* 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.");
}
});
Comment on lines -116 to +123
Copy link
Contributor

@N-Dekker N-Dekker Apr 30, 2024 •
edited
Loading

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

ITK/Modules/Registration/Metricsv4/include/itkANTSNeighborhoodCorrelationImageToImageMetricv4GetValueAndDerivativeThreader.hxx

Line 42 in 13ccff6

std::call_once(this->m_ANTSAssociateOnceFlag, [this, &associate]() { this->m_ANTSAssociate = associate; });
The 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"?

Copy link
Contributor

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.

Copy link
Contributor

@N-Dekker N-Dekker Apr 30, 2024 •
edited
Loading

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.

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was done by @songgang back in 2012 in 651dcd0. Are you still around @songgang ?

Copy link
Contributor

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:

ITK/Modules/Registration/Metricsv4/include/itkANTSNeighborhoodCorrelationImageToImageMetricv4DenseGetValueAndDerivativeThreader.h

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!

Copy link
Contributor Author

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.


/* call base method */
Superclass::ThreadedExecution(domain, threadId);
}

Expand Down