diff --git a/CLAUDE.md b/CLAUDE.md index 7ee9501bd..d0a4a92ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,6 +74,7 @@ Read before doing anything non-trivial; do not duplicate their content here: - [docs/agent-rules/asset-degradation-and-constructor-preconditions.md](docs/agent-rules/asset-degradation-and-constructor-preconditions.md) — a precondition asserted in a **constructor** delegates safety to every call site and they will disagree (issue #694: `Mesh`'s ctor vs ~30 `Ref::Create` sites); the missing-asset *recovery* path was itself the crash and had never been exercised by a test; relaxing a ctor is only half the change (every accessor on the now-representable invalid object must be total, or the crash just moves one call deeper); "degrades gracefully" means ONE warned-once message, not a per-frame flood; and why a "load the scene, does it crash?" check passes while the bug is fully present — the trigger is *resolution*, not *loading*. - [docs/agent-rules/audio-voice-budget.md](docs/agent-rules/audio-voice-budget.md) — the concurrent-voice cap / stealing / virtualization policy (issue #730): the admission point must live *inside* `AudioSource::Play()` / `SoundGraphSound::Play()` because the engine starts sounds from six call sites, not the two a grep suggests; the policy object must stay miniaudio-free or it is untestable (a real `AudioSource` needs a live device); "it's playing again" is the wrong assertion for a resumed loop — assert the **position**, and let the backend cursor win while audible; the promotion margin is what stops two near-equal voices stop/starting every frame; hosts are called with the lock released; a one-shot must auto-retire or it leaks its slot forever; the graph path can only mute (no seek/suspend API) and inverts priority polarity; and the four edits — one of them silent — a new `AudioSourceConfig` field costs. - [docs/agent-rules/asset-import-usd-alembic.md](docs/agent-rules/asset-import-usd-alembic.md) — the #655 interchange abstraction (`MeshImporterRegistry`/`MeshExporterRegistry` fronting Assimp/USD/Alembic + glTF export, keyed by extension with an Assimp fallback) and every gotcha vendoring the three heavyweight VFX libs into the static-everything build: **OpenUSD is a prebuilt static-monolithic install (`OLO_WITH_USD` + `OLO_USD_INSTALL_DIR`), NOT FetchContent** — its plugInfo tree is install-produced, needs `/WHOLEARCHIVE:usd_m` + `PlugRegistry::RegisterPlugins` at runtime, a `$` static-lib patch, oneTBB (`tbb12.lib` auto-link + `Shlwapi`/`Dbghelp`), git long-paths, ~2 GB lib; **Alembic** (FetchContent, Imath-first) needs `IMATH_INSTALL ON` for Alembic's unconditional `export()` and a `$`-wrapped `Imath_INCLUDE_DIRS` for ``; **MaterialX** collides with assimp's bundled pugixml → `/FORCE:MULTIPLE` (assimp wins, verified runtime-safe). Plus the per-format silent-correctness traps (winding, up-axis, metersPerUnit, UV origin, facevarying indexing). +- [docs/agent-rules/spinlock-payload-cache-line-separation.md](docs/agent-rules/spinlock-payload-cache-line-separation.md) — why a lock and its guarded payload must not share a cache line (Pikus, C++Now 2026): an unlocked reader downgrades the owner's line from Exclusive to Shared, costing the owner an invalidate round-trip on its next acquire/release even with no blocking contention. Covers the judgement bar for when adjacency is actually worth padding (is the flag read by threads that skip the lock entirely; is the path contended; is the adjacency provable, not guessed) versus when it isn't (`Core/Ref.cpp`'s registry mutex is the dominant cost, not its cache-line layout — see [docs/agent-rules/intrusive-refcount-weakref-races.md](docs/agent-rules/intrusive-refcount-weakref-races.md)), and the one real fix found (`Task/TaskPrivate.h`'s `FTaskBase::TSubsequents`). - [docs/ops/build.md](docs/ops/build.md) — full Windows / Linux / WSL build matrix. - [docs/testing.md](docs/testing.md) — opinion document on **why** we test what we test, value heuristic, named anti-patterns, retirement criteria, and how tests are classified. The per-file catalogue tables are **generated, git-ignored** (`docs/test-catalogue.*.md`), not inlined here. diff --git a/OloEngine/src/OloEngine/Task/TaskPrivate.h b/OloEngine/src/OloEngine/Task/TaskPrivate.h index 4b216f499..db983fe2e 100644 --- a/OloEngine/src/OloEngine/Task/TaskPrivate.h +++ b/OloEngine/src/OloEngine/Task/TaskPrivate.h @@ -1061,12 +1061,12 @@ namespace OloEngine::Tasks public: bool PushIfNotClosed(FTaskBase* NewItem) { - if (m_IsClosed.load(std::memory_order_acquire)) + if (m_Close.IsClosed.load(std::memory_order_acquire)) { return false; } - TUniqueLock Lock(m_Mutex); - if (m_IsClosed.load(std::memory_order_relaxed)) + TUniqueLock Lock(m_Close.Mutex); + if (m_Close.IsClosed.load(std::memory_order_relaxed)) { return false; } @@ -1076,20 +1076,37 @@ namespace OloEngine::Tasks TArray Close() { - TUniqueLock Lock(m_Mutex); - m_IsClosed.store(true, std::memory_order_release); + TUniqueLock Lock(m_Close.Mutex); + m_Close.IsClosed.store(true, std::memory_order_release); return MoveTemp(m_Subsequents); } bool IsClosed() const { - return m_IsClosed.load(std::memory_order_acquire); + return m_Close.IsClosed.load(std::memory_order_acquire); } private: TArray m_Subsequents; - std::atomic m_IsClosed{ false }; - FMutex m_Mutex; + // Cache-line separated from m_Subsequents: PushIfNotClosed's unlocked + // fast-path read of IsClosed runs on every prerequisite->subsequent + // edge added anywhere in the task graph (FTaskBase::AddSubsequent), + // often from several worker threads racing to attach themselves to the + // same shared prerequisite. Without separation those reads keep + // dragging m_Subsequents' inline storage between Exclusive and Shared, + // so Close()'s Mutex.Lock() (an Exclusive-owning RMW on the same line) + // pays an extra invalidation round-trip it would not otherwise need + // (Pikus, "Lock-Free Programming is Dead", C++Now 2026). IsClosed and + // Mutex are grouped into one alignas'd struct — rather than alignas on + // IsClosed alone — so the pairing (both must share a line WITH each + // other, just not with m_Subsequents) survives a future member reorder + // or insertion instead of relying on incidental declaration order. + struct alignas(OLO_PLATFORM_CACHE_LINE_SIZE) FCloseState + { + std::atomic IsClosed{ false }; + FMutex Mutex; + }; + FCloseState m_Close; }; TSubsequents> m_Subsequents; diff --git a/docs/agent-rules/spinlock-payload-cache-line-separation.md b/docs/agent-rules/spinlock-payload-cache-line-separation.md new file mode 100644 index 000000000..0fe464187 --- /dev/null +++ b/docs/agent-rules/spinlock-payload-cache-line-separation.md @@ -0,0 +1,120 @@ +# Don't put a lock and its guarded payload on the same cache line + +Source: Fedor Pikus, *Lock-Free Programming is Dead* (C++Now 2026), closing slide. +Written from a repo-wide audit for a C++Now 2026 follow-up branch (issue-free — +see `HANDOVER.md` history on `feature/cppnow26-audits`). + +## The mechanism + +A thread that merely *reads* a lock — an unlocked fast-path check, a spin-wait, +a `TryLock`-and-bail — pulls that cache line into Shared state on its core. If +the lock and the payload it guards share a line, that read also drags the +payload along, even though the reader never touched the payload itself. When +the owner then does a genuinely exclusive operation on the lock (a CAS to +acquire, a release-store), the CPU has to pay an invalidate round-trip it +would not otherwise need — the owner's line was Exclusive, a reader's +unrelated peek downgraded it to Shared, and the owner has to re-acquire +Exclusive before it can proceed. This costs real cycles even when the lock +itself is uncontended in the traditional sense (no thread ever blocks on it). + +The fix is mechanical once you've found a real instance: pad the lock (or the +payload) onto its own line with `alignas(OLO_PLATFORM_CACHE_LINE_SIZE)` — +**use the project's existing constant** (`OloEngine/src/OloEngine/Memory/Platform.h`), +don't invent a second one. `/wd4324` is already suppressed project-wide +precisely because this pattern's padding warning is expected, not a defect. + +## Judgement: not every atomic-bool-next-to-a-mutex qualifies + +The audit that produced this note grepped `Threading/`, `Task/`, `Async/`, +`Audio/`, `Core/` for `std::atomic` / `std::atomic_flag` / lock-shaped +members and found roughly two dozen candidates. Only one was worth fixing. +The bar that separated it from the rest: + +1. **Is the flag actually read by threads that don't also take the lock?** + A flag that's always read-then-immediately-locked (double-checked-locking + idiom) doesn't get the "spinning reader avoids the lock" benefit from + separation — the mutex's own cache-line traffic already dominates. This is + why `Core/Ref.cpp`'s `LiveReferencesData::isValid` was **not** padded + despite sitting next to `FMutex mutex` and being read unlocked at the top + of every `Ref` add/release/lock: every one of those call sites goes on + to acquire the lock in the common (non-shutdown) path anyway. See + [intrusive-refcount-weakref-races.md](intrusive-refcount-weakref-races.md) — + that doc's own conclusion is the more important one here: the *global + mutex itself* (one lock serializing + every `Ref` op process-wide) is the dominant cost, matching Pikus's + framing that "the cost of the lock doesn't matter — disruption of + execution flow matters." Cache-line padding is a rounding error next to a + structural single-global-lock bottleneck; fix the bigger problem first, if + it's ever profiled as hot. +2. **Is the path actually contended?** A flag touched once at init/shutdown + (audio suspend flags polled by a 100µs-sleep teardown loop, async-load + "ready" flags checked once per load) isn't worth a padding byte — and + padding indiscriminately bloats every instance of a hot struct for no + benefit. Several `std::atomic` candidates in `Audio/SoundGraph/` + were rejected on this basis alone. +3. **Is the adjacency provable, not guessed?** A hand-wavy "these two members + are probably on the same line" isn't enough to justify a change — either + the containing struct is small enough that adjacency is obvious by + inspection (the fix below), or it needs an actual `offsetof`/`sizeof` + check before touching it. `Task/WaitingQueue.h`'s `FWaitingQueue` was + considered (its `m_State`/`m_StandbyState` CAS traffic is about as hot as + this codebase gets, with `m_IsShuttingDown` declared nearby) and rejected + specifically because the real layout — depending on `TFunction`'s + captured-callback size — couldn't be confirmed without compiling and + inspecting it, and a wrong guess is worse than no fix. + +## The one fix that cleared the bar + +`Tasks::Private::FTaskBase::TSubsequents` (`OloEngine/src/OloEngine/Task/TaskPrivate.h`): + +```cpp +TArray m_Subsequents; // payload +std::atomic m_IsClosed{ false }; // read unlocked, every AddSubsequent() +FMutex m_Mutex; // guards m_Subsequents, paired with m_IsClosed +``` + +`PushIfNotClosed()` — called via `FTaskBase::AddSubsequent()` for **every** +prerequisite→subsequent edge registered anywhere in the task graph — reads +`m_IsClosed` unlocked before deciding whether to take the lock at all. A busy +task graph routinely has several worker threads racing to attach themselves +as subsequents of the same shared prerequisite task concurrently, so this is +a genuinely hot, multi-reader path — unlike the `Ref.cpp` case, here the +*unlocked* read is the common case that matters (a closed subsequents list +short-circuits before ever touching the mutex). The whole struct — a small +inline-allocator `TArray` plus a `bool` plus a 1-byte `FMutex` — comfortably +fits on one 64-byte line by default, so the adjacency needed no +`offsetof` archaeology to confirm. + +Fix: `m_IsClosed` and `m_Mutex` are the two members that must land on the +*same* line as each other — every access to one happens alongside the other, +in `PushIfNotClosed()`/`Close()` — while staying off `m_Subsequents`'s line. +`alignas(OLO_PLATFORM_CACHE_LINE_SIZE)` on `m_IsClosed` alone achieves that +by relying on `m_Mutex` (1 byte, natural alignment 1) landing immediately +after it in the same padded region — correct today, but implicit: a member +inserted between them, or a reorder, would silently break the guarantee with +no compiler diagnostic. The actual fix groups both into one `alignas`'d +nested struct instead: + +```cpp +struct alignas(OLO_PLATFORM_CACHE_LINE_SIZE) FCloseState +{ + std::atomic IsClosed{ false }; + FMutex Mutex; +}; +FCloseState m_Close; +``` + +— the pairing is then structural, not incidental to declaration order. +This is the same shape as the existing `FPaddedSharedTask` wrapper in +`Task/TaskConcurrencyLimiter.h`: when more than one member needs to share a +line *with each other* while staying separated from something else, wrap +them together rather than `alignas`-ing the first and trusting the rest to +follow. + +## Outcome of the audit + +One site fixed out of ~2 dozen candidates surveyed across `Threading/`, +`Task/`, `Async/`, `Audio/`, `Core/`. That is the expected outcome, not a +sign the audit was too shallow — see the judgement criteria above. If a +future pass through a different subsystem turns up more, the same three +questions apply.