Skip to content

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

Open
hjmjohnson wants to merge 2 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:perf-threadpool-atomic-singleton-guard
Open

PERF: Guard the ThreadPool singleton with an atomic instead of std::once_flag#6705
hjmjohnson wants to merge 2 commits into
InsightSoftwareConsortium:mainfrom
hjmjohnson:perf-threadpool-atomic-singleton-guard

Conversation

@hjmjohnson

Copy link
Copy Markdown
Member

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 itkGetGlobalSimpleMacroSingletonIndex. 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.

Two dynamically loaded libraries, each statically linking its own copy of
ITKCommon and sharing a single SingletonIndex, must observe the same
ThreadPool. That is the configuration of every wrapped Python module when
ITK is built static, and nothing covered it.
Replaces the 4-byte std::once_flag with a 1-byte atomic guard plus the mutex
ThreadPoolGlobals already owns. The guard stays inside those globals, which
the SingletonIndex shares, so every ITK instance still agrees on one
singleton and registers pthread_atfork once.

Selects std::atomic_flag when C++20 makes test() available and
std::atomic<bool> otherwise. The choice keys off the library feature-test
macro rather than __cplusplus, which MSVC misreports.

Addresses the goal of InsightSoftwareConsortium#4171, which moved the guard to a function-local
static and so gave each loaded module its own.
@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Performance Improvement in terms of compilation or execution time type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Jul 25, 2026
@hjmjohnson
hjmjohnson marked this pull request as ready for review July 25, 2026 18:53
@hjmjohnson
hjmjohnson requested a review from N-Dekker July 25, 2026 18:53
@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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 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.

View all artifacts

T-Rex 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Performance Improvement in terms of compilation or execution time type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant