From a6fd01d483e826e6793383a6f9fe3480fd31ee81 Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Thu, 21 Jan 2021 18:13:19 +0100 Subject: [PATCH 1/3] ENH: Allow sharing xoutManager between threads --- Core/Kernel/elxElastixMain.cxx | 44 ++++++++++++++++++++++++++++++++++ Core/Kernel/elxElastixMain.h | 3 +++ Core/Main/elxElastixFilter.hxx | 2 +- 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/Core/Kernel/elxElastixMain.cxx b/Core/Kernel/elxElastixMain.cxx index b9c68a37c..aa8007ecd 100644 --- a/Core/Kernel/elxElastixMain.cxx +++ b/Core/Kernel/elxElastixMain.cxx @@ -35,6 +35,9 @@ # include "itkOpenCLSetup.h" #endif +#include + + namespace { /** @@ -139,6 +142,47 @@ xoutManager::xoutManager(const std::string & logFileName, const bool setupLoggin } } + +std::shared_ptr +xoutManager::GetSharedManager(const std::string & logFileName, const bool setupLogging, const bool setupCout) +{ + const auto makeManagerPtrPair = [](const std::string & logFileName, const bool setupLogging, const bool setupCout) { + std::shared_ptr sharedPtr(new xoutManager(logFileName, setupLogging, setupCout)); + std::weak_ptr weakPtr(sharedPtr); + return std::make_pair(sharedPtr, weakPtr); + }; + + // Note that the initialization of this static variable is thread-safe, + // as supported by C++11 "magic statics". + static auto managerPtrPair = makeManagerPtrPair(logFileName, setupLogging, setupCout); + + const struct ResetGuard + { + std::shared_ptr & sharedPtr; + ~ResetGuard() { sharedPtr.reset(); } + } resetGuard{ managerPtrPair.first }; + + const auto lockedSharedPtr = managerPtrPair.second.lock(); + + if (lockedSharedPtr == nullptr) + { + // Apply the "double-checked locking" design pattern. + static std::mutex managerMutex; + const std::lock_guard lock(managerMutex); + + const auto doubleCheckedLockedSharedPtr = managerPtrPair.second.lock(); + + if (doubleCheckedLockedSharedPtr == nullptr) + { + managerPtrPair = makeManagerPtrPair(logFileName, setupLogging, setupCout); + return managerPtrPair.second.lock(); + } + return doubleCheckedLockedSharedPtr; + } + return lockedSharedPtr; +} + + xoutManager::Guard::~Guard() { g_data = {}; diff --git a/Core/Kernel/elxElastixMain.h b/Core/Kernel/elxElastixMain.h index ffb09a2af..17790dc0e 100644 --- a/Core/Kernel/elxElastixMain.h +++ b/Core/Kernel/elxElastixMain.h @@ -64,6 +64,9 @@ class xoutManager /** This explicit constructor does set up the "xout" output streams. */ explicit xoutManager(const std::string & logfilename, const bool setupLogging, const bool setupCout); + static std::shared_ptr + GetSharedManager(const std::string & logFileName, const bool setupLogging, const bool setupCout); + /** The default-constructor only just constructs a manager object */ xoutManager() = default; diff --git a/Core/Main/elxElastixFilter.hxx b/Core/Main/elxElastixFilter.hxx index 92aabfa0c..36d768e51 100644 --- a/Core/Main/elxElastixFilter.hxx +++ b/Core/Main/elxElastixFilter.hxx @@ -197,7 +197,7 @@ ElastixFilter::GenerateData(void) } // Setup xout - const elx::xoutManager manager(logFileName, this->GetLogToFile(), this->GetLogToConsole()); + const auto sharedManager = xoutManager::GetSharedManager(logFileName, this->GetLogToFile(), this->GetLogToConsole()); // Run the (possibly multiple) registration(s) for (unsigned int i = 0; i < parameterMapVector.size(); ++i) From 70a923f65b983c041191e44a87a0335387bdd7fe Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Wed, 17 Feb 2021 21:46:47 +0100 Subject: [PATCH 2/3] ENH: Improve thread-safety by adding mutex locks to xout classes --- Common/xout/xoutbase.cxx | 30 +++++++++++++++++++++++++++--- Common/xout/xoutbase.h | 8 ++++++++ Common/xout/xoutcell.cxx | 2 ++ Common/xout/xoutrow.cxx | 20 ++++++++++++++++++++ 4 files changed, 57 insertions(+), 3 deletions(-) diff --git a/Common/xout/xoutbase.cxx b/Common/xout/xoutbase.cxx index a4fa3b5be..f4ee00873 100644 --- a/Common/xout/xoutbase.cxx +++ b/Common/xout/xoutbase.cxx @@ -56,6 +56,8 @@ xoutbase & xoutbase::operator[](const char * cellname) void xoutbase::WriteBufferedData(void) { + const LockGuardType mutexLock(GetRecursiveMutex()); + /** Update the target c-streams. */ for (const auto & cell : m_CTargetCells) { @@ -78,6 +80,8 @@ xoutbase::WriteBufferedData(void) int xoutbase::AddTargetCell(const char * name, std::ostream * cell) { + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 1; if (this->m_XTargetCells.count(name)) @@ -103,6 +107,8 @@ xoutbase::AddTargetCell(const char * name, std::ostream * cell) int xoutbase::AddTargetCell(const char * name, Self * cell) { + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 1; if (this->m_CTargetCells.count(name)) @@ -128,6 +134,8 @@ xoutbase::AddTargetCell(const char * name, Self * cell) int xoutbase::RemoveTargetCell(const char * name) { + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 1; if (this->m_XTargetCells.erase(name) > 0) @@ -152,6 +160,8 @@ xoutbase::RemoveTargetCell(const char * name) void xoutbase::SetTargetCells(const CStreamMapType & cellmap) { + const LockGuardType mutexLock(GetRecursiveMutex()); + this->m_CTargetCells = cellmap; } // end SetTargetCells @@ -164,6 +174,7 @@ xoutbase::SetTargetCells(const CStreamMapType & cellmap) void xoutbase::SetTargetCells(const XStreamMapType & cellmap) { + const LockGuardType mutexLock(GetRecursiveMutex()); this->m_XTargetCells = cellmap; } // end SetTargetCells @@ -176,7 +187,8 @@ xoutbase::SetTargetCells(const XStreamMapType & cellmap) int xoutbase::AddOutput(const char * name, std::ostream * output) { - int returndummy = 1; + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 1; if (this->m_XOutputs.count(name)) { @@ -200,7 +212,8 @@ xoutbase::AddOutput(const char * name, std::ostream * output) int xoutbase::AddOutput(const char * name, Self * output) { - int returndummy = 1; + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 1; if (this->m_COutputs.count(name)) { @@ -224,7 +237,8 @@ xoutbase::AddOutput(const char * name, Self * output) int xoutbase::RemoveOutput(const char * name) { - int returndummy = 1; + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 1; if (this->m_XOutputs.count(name)) { @@ -250,6 +264,7 @@ xoutbase::RemoveOutput(const char * name) void xoutbase::SetOutputs(const CStreamMapType & outputmap) { + const LockGuardType mutexLock(GetRecursiveMutex()); this->m_COutputs = outputmap; } // end SetOutputs @@ -262,6 +277,7 @@ xoutbase::SetOutputs(const CStreamMapType & outputmap) void xoutbase::SetOutputs(const XStreamMapType & outputmap) { + const LockGuardType mutexLock(GetRecursiveMutex()); this->m_XOutputs = outputmap; } // end SetOutputs @@ -289,4 +305,12 @@ xoutbase::GetCOutputs(void) } // end GetOutputs + +std::recursive_mutex & +xoutbase::GetRecursiveMutex() +{ + static std::recursive_mutex mutex; + return mutex; +} + } // end namespace xoutlibrary diff --git a/Common/xout/xoutbase.h b/Common/xout/xoutbase.h index 8a147fe67..367dd59c6 100644 --- a/Common/xout/xoutbase.h +++ b/Common/xout/xoutbase.h @@ -21,6 +21,7 @@ #include #include #include +#include #include namespace xoutlibrary @@ -147,6 +148,11 @@ class xoutbase GetXOutputs(void); protected: + using LockGuardType = std::lock_guard; + + static std::recursive_mutex & + GetRecursiveMutex(); + /** Default-constructor. Only to be used by its derived classes. */ xoutbase() = default; @@ -164,6 +170,8 @@ class xoutbase Self & SendToTargets(const T & _arg) { + const LockGuardType mutexLock(GetRecursiveMutex()); + /** Send input to the target c-streams. */ for (const auto & cell : m_CTargetCells) { diff --git a/Common/xout/xoutcell.cxx b/Common/xout/xoutcell.cxx index 421cbff99..5f390d7f6 100644 --- a/Common/xout/xoutcell.cxx +++ b/Common/xout/xoutcell.cxx @@ -41,6 +41,8 @@ xoutcell::xoutcell() void xoutcell::WriteBufferedData(void) { + const LockGuardType mutexLock(GetRecursiveMutex()); + const std::string strbuf = this->m_InternalBuffer.str(); /** Send the string to the outputs */ diff --git a/Common/xout/xoutrow.cxx b/Common/xout/xoutrow.cxx index f876f3a32..e213024fb 100644 --- a/Common/xout/xoutrow.cxx +++ b/Common/xout/xoutrow.cxx @@ -34,6 +34,8 @@ using namespace std; void xoutrow::WriteBufferedData(void) { + const LockGuardType mutexLock(GetRecursiveMutex()); + /** Write the cell-data to the outputs, separated by tabs. */ auto xit = this->m_XTargetCells.begin(); auto tmpIt = xit; @@ -65,6 +67,8 @@ xoutrow::WriteBufferedData(void) int xoutrow::AddTargetCell(const char * name) { + const LockGuardType mutexLock(GetRecursiveMutex()); + if (this->m_CellMap.count(name) == 0) { /** A new cell (type xoutcell) is created. */ @@ -98,6 +102,8 @@ xoutrow::AddTargetCell(const char * name) int xoutrow::RemoveTargetCell(const char * name) { + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 1; if (this->m_XTargetCells.erase(name) > 0) @@ -122,6 +128,8 @@ xoutrow::RemoveTargetCell(const char * name) void xoutrow::SetTargetCells(const XStreamMapType & cellmap) { + const LockGuardType mutexLock(GetRecursiveMutex()); + /** Clean the this->m_CellMap (cells that are created using the * AddTarget(const char *) method. */ @@ -143,6 +151,8 @@ xoutrow::SetTargetCells(const XStreamMapType & cellmap) int xoutrow::AddOutput(const char * name, std::ostream * output) { + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 0; /** Set the output in all cells. */ @@ -165,6 +175,8 @@ xoutrow::AddOutput(const char * name, std::ostream * output) int xoutrow::AddOutput(const char * name, Superclass * output) { + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 0; /** Set the output in all cells. */ @@ -187,6 +199,8 @@ xoutrow::AddOutput(const char * name, Superclass * output) int xoutrow::RemoveOutput(const char * name) { + const LockGuardType mutexLock(GetRecursiveMutex()); + int returndummy = 0; /** Set the output in all cells. */ for (const auto & cell : this->m_XTargetCells) @@ -208,6 +222,8 @@ xoutrow::RemoveOutput(const char * name) void xoutrow::SetOutputs(const CStreamMapType & outputmap) { + const LockGuardType mutexLock(GetRecursiveMutex()); + /** Set the output in all cells. */ for (const auto & cell : this->m_XTargetCells) { @@ -227,6 +243,8 @@ xoutrow::SetOutputs(const CStreamMapType & outputmap) void xoutrow::SetOutputs(const XStreamMapType & outputmap) { + const LockGuardType mutexLock(GetRecursiveMutex()); + /** Set the output in all cells. */ for (const auto & cell : this->m_XTargetCells) { @@ -246,6 +264,8 @@ xoutrow::SetOutputs(const XStreamMapType & outputmap) void xoutrow::WriteHeaders(void) { + const LockGuardType mutexLock(GetRecursiveMutex()); + /** Copy '*this'. */ Self headerwriter; headerwriter.SetTargetCells(this->m_XTargetCells); From e272873f9add8a66dab4e1d988b581691386367a Mon Sep 17 00:00:00 2001 From: Niels Dekker Date: Wed, 20 Jan 2021 17:09:20 +0100 Subject: [PATCH 3/3] ENH: Add ElastixFilter unit tests UpdateSerially and UpdateInParallel Originally based on a code snippet by Konstantinos Ntatsis at pull request #389 ("ENH: Make ElastixMain database creation + loading components thread safe"): https://github.com/SuperElastix/elastix/pull/389#issuecomment-759744106 --- Core/Main/GTesting/ElastixFilterGTest.cxx | 125 ++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/Core/Main/GTesting/ElastixFilterGTest.cxx b/Core/Main/GTesting/ElastixFilterGTest.cxx index 165fe76fb..fa71e0b57 100644 --- a/Core/Main/GTesting/ElastixFilterGTest.cxx +++ b/Core/Main/GTesting/ElastixFilterGTest.cxx @@ -28,10 +28,123 @@ // GoogleTest header file: #include +#include #include // For transform #include #include #include // For pair +#include + + +namespace +{ +template +std::vector +GetTransformParameters(const elx::ElastixFilter & filter) +{ + const auto transformParameterObject = filter.GetTransformParameterObject(); + + if (transformParameterObject != nullptr) + { + const auto & transformParameterMaps = transformParameterObject->GetParameterMap(); + + if (transformParameterMaps.size() == 1) + { + const auto & transformParameterMap = transformParameterMaps.front(); + const auto found = transformParameterMap.find("TransformParameters"); + + if (found != transformParameterMap.cend()) + { + return found->second; + } + } + } + return {}; +} + + +void +TestUpdatingMultipleFilters(const bool inParallel) +{ + // Create an array of black images (pixel value zero) with a little "white" + // region (pixel value 1) inside. + constexpr auto ImageDimension = 2; + using ImageType = itk::Image; + using SizeType = itk::Size; + + const auto imageSizeValue = 8; + + const itk::ImageRegion imageRegion{ itk::Index::Filled(imageSizeValue / 2), + SizeType::Filled(2) }; + + const auto numberOfImages = 10; + std::array images; + + std::generate(images.begin(), images.end(), ImageType::New); + + for (const auto & image : images) + { + image->SetRegions(SizeType::Filled(imageSizeValue)); + image->Allocate(true); + const itk::Experimental::ImageRegionRange imageRegionRange{ *image, imageRegion }; + std::fill(std::begin(imageRegionRange), std::end(imageRegionRange), 1.0f); + } + + const auto parameterObject = elx::ParameterObject::New(); + parameterObject->SetParameterMap( + elx::ParameterObject::ParameterMapType{ // Parameters in alphabetic order: + { "ImageSampler", { "Full" } }, + { "MaximumNumberOfIterations", { "2" } }, + { "Metric", { "AdvancedNormalizedCorrelation" } }, + { "Optimizer", { "AdaptiveStochasticGradientDescent" } }, + { "Transform", { "TranslationTransform" } }, + { "WriteFinalTransformParameters", { "false" } } }); + + // Create a filter for each image, to register an image with the next one. + std::array>, numberOfImages> filters; + std::generate(filters.begin(), filters.end(), elx::ElastixFilter::New); + + for (auto i = 0; i < numberOfImages; ++i) + { + auto & filter = *(filters[i]); + + filter.LogToConsoleOff(); + filter.LogToFileOff(); + filter.SetFixedImage(images[i]); + // Choose the next image (from the array of images) as moving image. + filter.SetMovingImage(images[(i + std::size_t{ 1 }) % numberOfImages]); + filter.SetParameterObject(parameterObject); + } + + if (inParallel) + { + // Note: The OpenMP implementation of GCC (including GCC 5.5 and GCC 10.2) + // does not seem to support value-initialization of a for-loop index by + // empty pair of braces, as in `for (int i{}; i < n; ++i)`. +#pragma omp parallel for + for (int i = 0; i < numberOfImages; ++i) + { + filters[i]->Update(); + } + } + else + { + for (int i{}; i < numberOfImages; ++i) + { + filters[i]->Update(); + } + } + + // Check if the TransformParameters of each filter are as expected. + const std::vector expectedTransformParameters(ImageDimension, "0"); + + for (const auto & filter : filters) + { + EXPECT_EQ(GetTransformParameters(*filter), expectedTransformParameters); + } +} + +} // namespace // Tests registering two small (5x6) binary images, which are translated with respect to each other. @@ -106,3 +219,15 @@ GTEST_TEST(ElastixFilter, Translation) EXPECT_EQ(std::round(std::stod(transformParameters[i])), translationOffset[i]); } } + + +GTEST_TEST(ElastixFilter, UpdateSerially) +{ + TestUpdatingMultipleFilters(false); +} + + +GTEST_TEST(ElastixFilter, UpdateInParallel) +{ + TestUpdatingMultipleFilters(true); +}