Repository navigation
Conversation
41abef9 to
a8c6c5c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The setter change breaks downstream overrides and virtual dispatch, while removing the clamp can cause backend work-unit count mismatches that leave per-work-unit results unfilled.
2 open issues
What changed in this PR
This PR lets ITK’s pool backend use more work units than ITK_MAX_THREADS, without increasing the thread pool size or removing the platform backend’s limit.
Changes:
- Replace fixed pool work-unit storage with a vector grown before dispatch.
- Remove upper work-unit clamps from
ProcessObjectandDomainThreader. - Add above-cap regression coverage and update thread-limit assertions.
| File | Description |
|---|---|
Modules/Core/Common/test/itkTimeStampGTest.cxx |
Check the global thread limit without assuming work units are capped. |
Modules/Core/Common/test/itkMultiThreaderBaseTest.cxx |
Test above-cap pool execution and calling-thread participation. |
Modules/Core/Common/test/itkDataObjectAndProcessObjectTest.cxx |
Test above-cap work-unit requests. |
Modules/Core/Common/src/itkPoolMultiThreader.cxx |
Grow work-unit storage before dispatch and remove the work-unit cap. |
Modules/Core/Common/include/itkProcessObject.h |
Retain only the setter’s minimum bound. |
Modules/Core/Common/include/itkPoolMultiThreader.h |
Declare vector storage, resizing helper, and setter override. |
Modules/Core/Common/include/itkDomainThreader.h |
Remove the upper setter bound and clarify backend clamping. |
Review effort: Balanced
| * */ | ||
| /** @ITKStartGrouping */ | ||
| itkSetClampMacro(NumberOfWorkUnits, ThreadIdType, 1, ITK_MAX_THREADS); | ||
| void |
| void | ||
| SetNumberOfWorkUnits(ThreadIdType numberOfWorkUnits) | ||
| { | ||
| const ThreadIdType value = std::max<ThreadIdType>(1, numberOfWorkUnits); |
The array holds per-work-unit state and task futures, not thread info.
SetNumberOfWorkUnits now only enforces a minimum of one. The selected multithreader applies any limit it needs, so the ITK_MAX_THREADS cap of the statically sized threader arrays no longer leaks into the API.
Work-unit state and task futures live in a vector that grows before dispatch, so the number of work units is no longer capped by the statically sized array. The pool still limits the worker threads. PlatformMultiThreader keeps its ITK_MAX_THREADS limit.
a8c6c5c to
9717b71
Compare
[Medium impact] The change reaches shared ITK parallel-work settings and pool-based execution. Restore the virtual Findings
|
| void | ||
| SetNumberOfWorkUnits(ThreadIdType numberOfWorkUnits) |
There was a problem hiding this comment.
Subclass overrides stop working
DomainThreader::SetNumberOfWorkUnits is no longer virtual. The replaced itkSetClampMacro declared a virtual setter, so downstream subclasses that declare this method with override now fail to compile. Subclasses without override also stop receiving calls made through a DomainThreader pointer. Keep virtual on the replacement setter.
| void | |
| SetNumberOfWorkUnits(ThreadIdType numberOfWorkUnits) | |
| virtual void | |
| SetNumberOfWorkUnits(ThreadIdType numberOfWorkUnits) |
| const itk::ThreadIdType moreWorkUnits = itk::ITK_MAX_THREADS + 1; | ||
| const itk::PoolMultiThreader::Pointer poolThreader = itk::PoolMultiThreader::New(); | ||
| poolThreader->SetNumberOfWorkUnits(moreWorkUnits); | ||
| std::atomic<itk::SizeValueType> processedItems{ 0 }; | ||
| poolThreader->ParallelizeArray( | ||
| 0, moreWorkUnits, [&processedItems](itk::SizeValueType) { ++processedItems; }, nullptr); | ||
| result &= poolThreader->GetNumberOfWorkUnits() == moreWorkUnits; | ||
| result &= processedItems == moreWorkUnits; | ||
| SingleMethodExecutionData executionData{ std::this_thread::get_id() }; | ||
| poolThreader->SetSingleMethod(SingleMethodExecution, &executionData); | ||
| poolThreader->SingleMethodExecute(); | ||
| result &= executionData.CallCount == moreWorkUnits; | ||
| result &= executionData.CalledOnCallingThread; |
There was a problem hiding this comment.
Image path lacks limit coverage
The new tests cross ITK_MAX_THREADS through ParallelizeArray and SingleMethodExecute, but not ParallelizeImageRegion. That method has its own new allocation based on splitCount, so a regression there could pass both tests. Add a pool test with an image region that produces more than ITK_MAX_THREADS splits, and check that every part is visited exactly once.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

PoolMultiThreaderstores work-unit state in a growable vector, so its work-unit count is no longer capped byITK_MAX_THREADS.ProcessObjectandDomainThreaderdrop the matching upper clamp.PlatformMultiThreaderkeeps its limit.Behavior and scope
PoolMultiThreader::SetNumberOfWorkUnitsonly enforces a minimum of one; the array grows before each dispatch, never while tasks hold pointers into it.ProcessObjectandDomainThreader;PlatformMultiThreaderstill clamps them.m_ThreadInfoArraytom_WorkUnitInfoArray(no logic change).AI assistance
Test results
Clean configure (
releasepreset) and full rebuild with no errors, thenctest -j30withITK_GLOBAL_DEFAULT_NUMBER_OF_THREADS=4: 5314/5314 passed.itkMultiThreaderBaseTestnow coversParallelizeArrayandSingleMethodExecutewithITK_MAX_THREADS + 1work units on the pool;itkDataObjectAndProcessObjectTestcovers the unclamped setter.PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.