unitsync: link generic cpu_topology instead of per-platform detection#3049
Closed
tomjn wants to merge 1 commit into
Closed
unitsync: link generic cpu_topology instead of per-platform detection#3049tomjn wants to merge 1 commit into
tomjn wants to merge 1 commit into
Conversation
unitsync wraps FileSystemInitializer::Initialize() in the thread pool so
archive scanning/hashing parallelizes via for_mt, so it genuinely needs
the pool. It does not, however, pin threads, so it does not need real CPU
topology detection (P/E core masks, cache groups, pin policy).
The topology dependency was incidental: CPUID::EnumerateCores() (called
from the CPUID singleton ctor) eagerly calls cpu_topology::GetProcessorMasks()
and GetProcessorCache(), and Threading::GetChosenThreadPinPolicy() calls
cpu_topology::GetThreadPinPolicy(). These are link-time references in TUs
unitsync compiles, so it had to link Platform/{Linux,Win,Mac}/CpuTopology.cpp
even though it never pins.
Provide a generic, platform-agnostic cpu_topology implementation
(CpuTopologyGeneric.cpp) that reports every logical core as a single group
of performance cores and requests no pinning, and link that into unitsync
instead of the per-platform file. This removes the per-platform CpuTopology
from unitsync on all platforms (notably Mac/CpuTopology.cpp), making the
build consistent across Linux/Windows/macOS.
The engine and dedicated server are untouched and keep linking the real
per-platform topology for sim-worker pinning.
sprunk
requested changes
Jun 22, 2026
sprunk
left a comment
Collaborator
There was a problem hiding this comment.
Turns out the unitsync use of threading is reasonable so I think the original approach in that other PR (to actually add the proper mac cpu topology) is better after all.
Contributor
Author
|
Closed in favor of #3042 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
unitsync links a generic, platform-agnostic
cpu_topologyimplementation instead of the per-platformPlatform/{Linux,Win,Mac}/CpuTopology.cpp, removing unitsync's dependency on real CPU topology detection on all platforms.This is an alternative to #3042's "add
Mac/CpuTopology.cppto unitsync" — it instead makes unitsync stop needing per-platform topology at all, which is what @sprunk asked for on that PR.Why
unitsync wraps
FileSystemInitializer::Initialize()in the thread pool so archive scanning/hashing parallelizes viafor_mt, so it genuinely needs the pool. It does not pin threads, so it does not need real CPU topology (P/E core masks, cache groups, pin policy).The dependency was incidental:
CPUID::EnumerateCores()(called from theCPUIDsingleton ctor) eagerly callscpu_topology::GetProcessorMasks()+GetProcessorCache(), andThreading::GetChosenThreadPinPolicy()callscpu_topology::GetThreadPinPolicy(). Those are link-time references in TUs unitsync compiles, so it had to link a per-platformCpuTopology.cppeven though it never pins.Approach
A generic
CpuTopologyGeneric.cppreports every logical core as a single group of performance cores (soCPUID::EnumerateCores()still derives a correct logical-core count for the pool) and requestsTHREAD_PIN_POLICY_NONE. unitsync links that instead of the per-platform file. The change is contained totools/unitsync/CMakeLists.txt+ the new file; no shared engine CMake or threading logic is touched, so the engine/dedicated server keep linking the real per-platform topology for sim-worker pinning.Note on the "make CPUID lazy" alternative
An earlier idea was to make
CPUID::EnumerateCores()compute topology lazily so the core-count path wouldn't pullcpu_topology. That does not work for unitsync: it's aSHAREDlibrary and on macOS/Windows the build sets no-fvisibility=hidden(gated to Linux inTestCXXFlags.cmake), so theThreading/CpuTopologyCommonfree functions that reference the symbols stay exported and can't be dead-stripped. Deferring when a call runs doesn't remove a link-time reference. So a generic impl is the approach that actually drops the per-platform file.This satisfies the dependency with a trivial stub rather than eliminating the symbol references entirely; a literal "drop" would require
#ifdef UNITSYNCstubs throughCpuID.cpp/Threading.cpp/CpuTopologyCommon.cpp(shared core files). Happy to go that route instead if preferred.Verification
Built
unitsyncon macOS (Homebrew GCC 16):CpuTopologyGeneric.cpp.o, notMac/CpuTopology.cpp.o.nmonlibunitsync.dylib: zero undefinedcpu_topologysymbols; none of the real platform internals (detect_cpu_vendor,collect_intel,get_thread_siblings) are linked.Engine targets are untouched in their source wiring, so they keep the real topology.
Refs #3042.
Assisted by Claude Code; verified by building on macOS.