PERF: Guard the ThreadPool singleton with an atomic instead of std::once_flag

[hjmjohnson]

Keeps the ThreadPool singleton guard inside the SingletonIndex-shared ThreadPoolGlobals while replacing std::once_flag with a smaller atomic guard, and adds the regression test that was missing. Supersedes #4171, which pursued the same goal by moving the guard to a function-local static — per-binary storage, so each statically linked copy of ITKCommon re-runs the initializer.

Why the guard must live in ThreadPoolGlobals

ThreadPoolGlobals is registered through itkGetGlobalSimpleMacro → SingletonIndex. In a static build every wrapped Python module links its own ITKCommon and adopts one shared index (Wrapping/macro_files/itk_end_wrap_module.cmake). A guard with per-binary storage therefore cannot serialize creation of a per-process object: the second module re-enters the initializer, replaces the shared SmartPointer, and registers pthread_atfork a second time.

Measured behaviour of the superseded approach

Built from origin/main with BUILD_SHARED_LIBS=OFF, ITK_WRAP_PYTHON=ON (96 wrapped modules). Real import itk, two filters from two modules, then os.fork():

terminate called after throwing an instance of 'std::system_error'
  what():  Invalid argument

#11 itk::ThreadPool::CleanUp () from .../itk/_ITKSmoothingPython.abi3.so
#12 __run_prefork_handlers () at ./posix/register-atfork.c:141
#13 __libc_fork () at ./posix/fork.c:51

The failing prefork handler belongs to a different module's ITKCommon copy. main forks cleanly. With a pinned reference, two distinct pools and 5→9 worker threads are observed.

C++17 / C++20

std::atomic_flag::test() is C++20, so C++17 uses std::atomic<bool> with a static_assert on is_always_lock_free. Selection keys off __cpp_lib_atomic_flag_test rather than __cplusplus, which MSVC reports as 199711L unless /Zc:__cplusplus is passed. Verified both branches compile and pass (-std=c++17 and -std=c++20).

Local verification

Fresh isolated builds from origin/main, ccache disabled.

	main	#4171	this PR
ITKCommon suite	921/921	920/921	921/921
new sharing test	pass	fail	pass
import itk + fork()	ok	abort	ok
two-module singleton	one pool	two pools	one pool

Threading models, Module_ITKTBB=ON and OFF, all of Platform/Pool/TBB/Single: identical to main. Under a breakpoint on ThreadPool::ThreadPool, the TBB threader never constructs a pool and the Pool threader constructs exactly one — lazy creation is preserved, so non-Pool builds pay nothing.

Warm-path GetInstance() is at parity with call_once (~100–150 ns/call at 4 and 16 contending threads).

Not built here: macOS and Windows. The change adds no platform-specific code; the only conditional is the pre-existing ITK_USE_PTHREADS guard around pthread_atfork.

[greptile-apps]

Greptile Summary

Replaces std::call_once with a shared atomic fast-path and mutex-protected initialization for the ThreadPool singleton.

• Keeps the guard in ThreadPoolGlobals so statically linked ITKCommon copies sharing a SingletonIndex also share initialization state.
• Adds dynamically loaded test modules and a regression test verifying that static ITKCommon copies resolve the same ThreadPool instance.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified in the changed singleton initialization or regression-test path.

The mutex serializes first construction, the release/acquire guard safely publishes the initialized SmartPointer, failed construction remains retryable, and the guard shares the same SingletonIndex-backed lifetime as the pool state.

T-Rex Logs

What T-Rex did

• T-Rex attempted to configure and run the regression for the threadpool-singleton-sharing test, but the configuration could not start because cmake was not found.
• The log records the exact command executed, the working directory, exit code 127, tool versions, and the /bin/sh: cmake: not found error.
• As configuration could not begin, the regression driver and dynamic libraries could not be built or executed, so matching ThreadPool addresses could not be captured.

_{Ran code and verified through T-Rex}

Important Files Changed

Filename	Overview
Modules/Core/Common/src/itkThreadPool.cxx	Replaces call_once with double-checked atomic publication while retaining serialized initialization in shared singleton globals.
Modules/Core/Common/test/CMakeLists.txt	Builds two loadable modules with statically linked ITKCommon copies and registers the singleton-sharing regression test.
Modules/Core/Common/test/itkThreadPoolSingletonSharingTest.cxx	Loads both test modules, shares the driver's SingletonIndex, and verifies that both expose the same ThreadPool address.
Modules/Core/Common/test/ThreadPoolSingletonLibraryA.cxx	Provides exported test entry points for adopting the shared SingletonIndex and retrieving a pinned ThreadPool.
Modules/Core/Common/test/ThreadPoolSingletonLibraryB.cxx	Provides the second independently linked ITKCommon copy used to verify process-wide singleton sharing.

Sequence Diagram

sequenceDiagram
participant Caller
participant Globals as Shared ThreadPoolGlobals
participant Guard as Atomic guard
participant Mutex as Initialization mutex
participant Pool as ThreadPool
Caller->>Guard: acquire-load
alt guard is clear
    Caller->>Mutex: lock
    Caller->>Guard: relaxed-load
    alt still clear
        Caller->>Pool: create singleton
        Caller->>Guard: release-store set
    end
    Caller->>Mutex: unlock
end
Caller-->>Caller: return shared SmartPointer

Loading

_{Reviews (1): Last reviewed commit: "PERF: Guard the ThreadPool singleton wit..." | Re-trigger Greptile}

[N-Dekker]

Very interesting @hjmjohnson However, I still don't really understand the benefit of this PR. Maybe I need to have a closer look!

You write:

Supersedes #4171, which pursued the same goal by moving the guard to a function-local static — per-binary storage, so each statically linked copy of ITKCommon re-runs the initializer.

Well... my PR #4171 was mainly meant to simplify the code (proposing a STYLE commit). I thought that the m_ThreadPoolOnceFlag data member could be replaced by a simple local static variable. This would slightly reduce the number of lines of code (+5 -7). Instead, your PR seems to make the implementation more complicated, adding more lines of code (+243 -12).

Two questions:

1. You marked it "PERF", so did you observe, or do you expect a significant performance improvement from this PR?
2. For C++20, is atomic_flag in this case significantly faster than std::atomic<bool>? Otherwise I would prefer a single implementation for both C++17 and C++20.

[N-Dekker]

As far as I understand, the original call_once + once_flag are in general preferable to atomics, for a use case like this (initialization of a singleton).

If you think otherwise, can you please why? Specifically, do you expect a better runtime performance from atomics, and if so, why?