From 1155b5e99bda594cd65a957de5b562e5161d54f5 Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Thu, 2 May 2024 14:47:31 -0500 Subject: [PATCH 1/4] STYLE: Make variables local scope --- ...tkParallelSparseFieldLevelSetImageFilter.hxx | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx index 255d2afac5c..28e77d0e7d9 100644 --- a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx +++ b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx @@ -768,14 +768,13 @@ void ParallelSparseFieldLevelSetImageFilter::ThreadedAllocateData(ThreadIdType ThreadId) { static constexpr float SAFETY_FACTOR = 4.0; - unsigned int i, j; m_Data[ThreadId].m_Semaphore[0] = 0; m_Data[ThreadId].m_Semaphore[1] = 0; // Allocate the layers for the sparse field. m_Data[ThreadId].m_Layers.reserve(2 * m_NumberOfLayers + 1); - for (i = 0; i < 2 * static_cast(m_NumberOfLayers) + 1; ++i) + for (unsigned int i = 0; i < 2 * static_cast(m_NumberOfLayers) + 1; ++i) { m_Data[ThreadId].m_Layers.push_back(LayerType::New()); } @@ -787,11 +786,11 @@ ParallelSparseFieldLevelSetImageFilter::ThreadedAlloc // Layers used as buffers for transferring pixels during load balancing m_Data[ThreadId].m_LoadTransferBufferLayers = new LayerListType[2 * m_NumberOfLayers + 1]; - for (i = 0; i < 2 * static_cast(m_NumberOfLayers) + 1; ++i) + for (unsigned int i = 0; i < 2 * static_cast(m_NumberOfLayers) + 1; ++i) { m_Data[ThreadId].m_LoadTransferBufferLayers[i].reserve(m_NumOfWorkUnits); - for (j = 0; j < m_NumOfWorkUnits; ++j) + for (unsigned int j = 0; j < m_NumOfWorkUnits; ++j) { m_Data[ThreadId].m_LoadTransferBufferLayers[i].push_back(LayerType::New()); } @@ -810,7 +809,7 @@ ParallelSparseFieldLevelSetImageFilter::ThreadedAlloc m_Data[ThreadId].m_RMSChange = m_ValueZero; // UpLists and Downlists - for (i = 0; i < 2; ++i) + for (unsigned int i = 0; i < 2; ++i) { m_Data[ThreadId].UpList[i] = LayerType::New(); m_Data[ThreadId].DownList[i] = LayerType::New(); @@ -824,15 +823,15 @@ ParallelSparseFieldLevelSetImageFilter::ThreadedAlloc // for the Downlists m_Data[ThreadId].m_InterNeighborNodeTransferBufferLayers[1] = new LayerPointerType *[m_NumberOfLayers + 1]; - for (i = 0; i < static_cast(m_NumberOfLayers) + 1; ++i) + for (unsigned int i = 0; i < static_cast(m_NumberOfLayers) + 1; ++i) { m_Data[ThreadId].m_InterNeighborNodeTransferBufferLayers[0][i] = new LayerPointerType[m_NumOfWorkUnits]; m_Data[ThreadId].m_InterNeighborNodeTransferBufferLayers[1][i] = new LayerPointerType[m_NumOfWorkUnits]; } - for (i = 0; i < static_cast(m_NumberOfLayers) + 1; ++i) + for (unsigned int i = 0; i < static_cast(m_NumberOfLayers) + 1; ++i) { - for (j = 0; j < m_NumOfWorkUnits; ++j) + for (unsigned int j = 0; j < m_NumOfWorkUnits; ++j) { m_Data[ThreadId].m_InterNeighborNodeTransferBufferLayers[0][i][j] = LayerType::New(); m_Data[ThreadId].m_InterNeighborNodeTransferBufferLayers[1][i][j] = LayerType::New(); @@ -841,7 +840,7 @@ ParallelSparseFieldLevelSetImageFilter::ThreadedAlloc // Local histogram for every thread (used during Iterate() ) m_Data[ThreadId].m_ZHistogram = new int[m_ZSize]; - for (i = 0; i < m_ZSize; ++i) + for (unsigned int i = 0; i < m_ZSize; ++i) { m_Data[ThreadId].m_ZHistogram[i] = 0; } From 6dcfb74e6b43bddaf5961c4989253c8d4773f4bb Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Thu, 2 May 2024 15:34:45 -0500 Subject: [PATCH 2/4] COMP: Fix gcc 13.2 compiler warning of large allocation size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ITK/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx: In member function ‘ThreadedAllocateData’: ITK/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx:789:49: warning: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=] 789 | m_Data[ThreadId].m_LoadTransferBufferLayers = new LayerListType[2 * m_NumberOfLayers + 1]; | ^ /usr/include/c++/13/new:128:26: note: in a call to allocation function ‘operator new []’ declared here 128 | _GLIBCXX_NODISCARD void* operator new[](std::size_t) _GLIBCXX_THROW (std::bad_alloc) | ^ ITK/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx: In member function ‘ThreadedAllocateData’: ITK/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx:789:49: warning: argument 1 value ‘18446744073709551615’ exceeds maximum object size 9223372036854775807 [-Walloc-size-larger-than=] 789 | m_Data[ThreadId].m_LoadTransferBufferLayers = new LayerListType[2 * m_NumberOfLayers + 1]; | ^ /usr/include/c++/13/new:128:26: note: in a call to allocation function ‘operator new []’ declared here 128 | _GLIBCXX_NODISCARD void* operator new[](std::size_t) _GLIBCXX_THROW (std::bad_alloc) | --- .../itkParallelSparseFieldLevelSetImageFilter.hxx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx index 28e77d0e7d9..ead0b9c43a2 100644 --- a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx +++ b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx @@ -772,8 +772,9 @@ ParallelSparseFieldLevelSetImageFilter::ThreadedAlloc m_Data[ThreadId].m_Semaphore[0] = 0; m_Data[ThreadId].m_Semaphore[1] = 0; + const std::size_t bufferLayerSize = 2 * m_NumberOfLayers + 1; // Allocate the layers for the sparse field. - m_Data[ThreadId].m_Layers.reserve(2 * m_NumberOfLayers + 1); + m_Data[ThreadId].m_Layers.reserve(bufferLayerSize); for (unsigned int i = 0; i < 2 * static_cast(m_NumberOfLayers) + 1; ++i) { m_Data[ThreadId].m_Layers.push_back(LayerType::New()); @@ -785,7 +786,8 @@ ParallelSparseFieldLevelSetImageFilter::ThreadedAlloc } // Layers used as buffers for transferring pixels during load balancing - m_Data[ThreadId].m_LoadTransferBufferLayers = new LayerListType[2 * m_NumberOfLayers + 1]; + + m_Data[ThreadId].m_LoadTransferBufferLayers = new LayerListType[bufferLayerSize]; for (unsigned int i = 0; i < 2 * static_cast(m_NumberOfLayers) + 1; ++i) { m_Data[ThreadId].m_LoadTransferBufferLayers[i].reserve(m_NumOfWorkUnits); @@ -802,8 +804,7 @@ ParallelSparseFieldLevelSetImageFilter::ThreadedAlloc // The SAFETY_FACTOR simple ensures that the number of nodes created // is larger than those required to start with for each thread. - auto nodeNum = - static_cast(SAFETY_FACTOR * m_Layers[0]->Size() * (2 * m_NumberOfLayers + 1) / m_NumOfWorkUnits); + auto nodeNum = static_cast(SAFETY_FACTOR * m_Layers[0]->Size() * (bufferLayerSize) / m_NumOfWorkUnits); m_Data[ThreadId].m_LayerNodeStore->Reserve(nodeNum); m_Data[ThreadId].m_RMSChange = m_ValueZero; From 408cf0c7de587800bb0fa88281ecc1d751322054 Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Thu, 2 May 2024 14:49:33 -0500 Subject: [PATCH 3/4] COMP: Fix gcc 13.2 compiler warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Limit possible index overflow. In member function ‘SetVnlVector’, inlined from ‘_wrap_itkVectorUC2_SetVnlVector’ at ITK/cmake-build-release/Wrapping/Modules/ITKCommon/itkVectorPython.cpp:31005:0: ITK/Modules/Core/Common/include/itkVector.hxx:160: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] 160 | (*this)[i] = v(i); | ITK/Modules/Core/Common/include/itkFixedArray.h: In function ‘_wrap_itkVectorUC2_SetVnlVector’: ITK/Modules/Core/Common/include/itkFixedArray.h:437:10: note: at offset 2 into destination object ‘m_InternalArray’ of size 2 437 | CArray m_InternalArray; | ^ --- Modules/Core/Common/include/itkVector.h | 3 ++- Modules/Core/Common/include/itkVector.hxx | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Modules/Core/Common/include/itkVector.h b/Modules/Core/Common/include/itkVector.h index 02fd8fec328..8a5e1cdf8af 100644 --- a/Modules/Core/Common/include/itkVector.h +++ b/Modules/Core/Common/include/itkVector.h @@ -90,7 +90,8 @@ class ITK_TEMPLATE_EXPORT Vector : public FixedArray return VVectorDimension; } - /** Set a vnl_vector_ref referencing the same memory block. */ + /** Copy values from the vnl_vector input to the internal memory block. The minimum of + * VVectorDimension and vnl_vector::size() elements are copied. */ void SetVnlVector(const vnl_vector &); diff --git a/Modules/Core/Common/include/itkVector.hxx b/Modules/Core/Common/include/itkVector.hxx index 408a9d8cb4f..8d2fca35141 100644 --- a/Modules/Core/Common/include/itkVector.hxx +++ b/Modules/Core/Common/include/itkVector.hxx @@ -155,7 +155,8 @@ template void Vector::SetVnlVector(const vnl_vector & v) { - for (unsigned int i = 0; i < v.size(); ++i) + const unsigned int elements_to_copy = std::min(TVectorDimension, v.size()); + for (unsigned int i = 0; i < elements_to_copy; ++i) { (*this)[i] = v(i); } From e64032ae757fd2d93878a10e96d191b8618a8ace Mon Sep 17 00:00:00 2001 From: Hans Johnson Date: Wed, 8 May 2024 08:42:56 -0500 Subject: [PATCH 4/4] PERF: Prefer initialization to assignment Initialize the memory to zeros upon allocation. Avoid initialization followed by assignment to zero. --- .../include/itkParallelSparseFieldLevelSetImageFilter.hxx | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx index ead0b9c43a2..cd8955db488 100644 --- a/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx +++ b/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx @@ -839,12 +839,8 @@ ParallelSparseFieldLevelSetImageFilter::ThreadedAlloc } } - // Local histogram for every thread (used during Iterate() ) - m_Data[ThreadId].m_ZHistogram = new int[m_ZSize]; - for (unsigned int i = 0; i < m_ZSize; ++i) - { - m_Data[ThreadId].m_ZHistogram[i] = 0; - } + // Local histogram for every thread (used during Iterate()), initialized to zeros. + m_Data[ThreadId].m_ZHistogram = new int[m_ZSize](); // Every thread must have its own copy of the GlobalData struct. m_Data[ThreadId].globalData = this->GetDifferenceFunction()->GetGlobalDataPointer();