C++Now 2026 audit follow-up: cache-line fix + two spikes (Dist link blocker, static-init survey) - #764
Merged
Merged
Conversation
…ad (C++Now 2026 audit) FTaskBase::TSubsequents::PushIfNotClosed does an unlocked fast-path read of m_IsClosed on every prerequisite->subsequent edge added anywhere in the task graph, often from several worker threads racing to attach to the same shared prerequisite. It shared a cache line with the FMutex and the TArray payload those reads should be independent of, so Close()'s lock acquisition paid an avoidable invalidation round-trip (Pikus, "Lock-Free Programming is Dead", C++Now 2026). alignas() separates it, using the project's existing cache-line constant. Also documents the judgement criteria from the repo-wide audit that produced this single fix (~2 dozen candidates surveyed, most rejected) in a new docs/agent-rules note, linked from CLAUDE.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe change documents cache-line separation criteria, records the audit outcome, and updates ChangesCache-Line Separation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OloEngine/src/OloEngine/Task/TaskPrivate.h`:
- Around line 1091-1100: Ensure the synchronization members near m_IsClosed and
m_Mutex occupy separate cache lines by independently aligning m_Mutex or using
an appropriately sized wrapper, rather than relying on m_IsClosed alignment
alone. Update the guidance in
docs/agent-rules/spinlock-payload-cache-line-separation.md:88-89 to describe
this complete separation requirement; no other sites require changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 46a2cb33-cbcc-4912-be99-55d5fb2365cf
📒 Files selected for processing (3)
CLAUDE.mdOloEngine/src/OloEngine/Task/TaskPrivate.hdocs/agent-rules/spinlock-payload-cache-line-separation.md
…s wrapper Review feedback on #764: the previous fix relied on m_Mutex (1 byte, natural alignment 1) landing immediately after alignas(64)'d m_IsClosed to inherit its cache-line separation from m_Subsequents. That's correct today per C++ layout rules, but implicit — a member inserted between them, or a reorder, would silently break it with no diagnostic. Groups both into one alignas'd FCloseState struct instead, matching the existing FPaddedSharedTask pattern in Task/TaskConcurrencyLimiter.h: the pairing is now structural rather than incidental to declaration order. Updated the accompanying docs/agent-rules note to describe the wrapper as the actual guidance rather than "alignas the first member and trust the rest to follow." Validated: OloEngine-Tests full suite (5546/5553 passed, same 6 expected skips, same 1 pre-existing/tracked failure as before this change) plus the Task/SystemScheduler suites in isolation (73/73). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@OloEngine/src/OloEngine/Task/TaskPrivate.h`:
- Around line 1104-1109: Rename the non-static members of FCloseState from
IsClosed and Mutex to m_IsClosed and m_Mutex, then update all corresponding
accesses in the nearby close-state logic (lines 1064-1086) to use the new names.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8ae37007-8310-4330-9e21-a983ccb6a22f
📒 Files selected for processing (2)
OloEngine/src/OloEngine/Task/TaskPrivate.hdocs/agent-rules/spinlock-payload-cache-line-separation.md
|
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
1 participant
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.
Summary
Four small, independent items from a C++Now 2026 conference review, deliberately not issue-sourced (filing would have cost more than doing them, or the output was unknown until measured). Full context in
HANDOVER.mdon this branch (gitignored, not part of this diff).Threading/Task/Async/Audio/Corefor lock-shaped members guarding an adjacent payload on the same cache line. Fixed the one genuinely contended site that cleared the bar:Tasks::Private::FTaskBase::TSubsequentsinTaskPrivate.h— its unlocked fast-pathm_IsClosedread (hit on every prerequisite→subsequent edge added anywhere in the task graph) shared a line with theFMutexandTArraypayload it guards.alignas(OLO_PLATFORM_CACHE_LINE_SIZE)separates them. ~2 dozen other candidates were surveyed and rejected (not contended, or the real cost was a structural single-global-lock bottleneck rather than adjacency — see the new doc). Judgement criteria + rejected candidates written up in docs/agent-rules/spinlock-payload-cache-line-separation.md, linked fromCLAUDE.md./GwA/B on Dist binary size (Gamberini, Leveraging the Linker): blocked before it could start.OloEngine.libfails to link in Dist config on unmodifiedorigin/master(LNK1248, archive >4GiB from/GLwhole-program-optimization IL bloat — e.g. a singleLuaScriptGlue.objwas 360MB). CI never catches it because every workflow builds Release or Debug, never Dist. Filed Dist config: OloEngine.lib exceeds the 4GiB COFF archive limit (LNK1248), CI never catches it #762 with the root-cause writeup. No/Gwchange was made — there's no linkable baseline to measure against.-Wglobal-constructors/-Wexit-time-destructorsto theclangclpreset only (warnings, not-Werror, never on msvc). BuiltOloEngine-Tests: 200 distinct sites in our own code (raw counts ~2600 but hugely inflated byinline staticmembers warning per translation unit). Filed Static-init audit: 200 distinct -Wglobal-constructors/-Wexit-time-destructors sites in our own code (clangcl spike) #763 with the breakdown, top offenders, and representative examples (a cleanstring_viewwin inMaterialAsset.cppvs. legitimate singleton state inNetworkManager.cpp). Flags reverted from this branch — not merged, per the spike's own decision tree.SystemScheduler(Suvorov, What is your Algorithmic Core?): hand-mutated 5 lines inSystemScheduler.cpp's Kahn-sort/tie-break logic (tie-break comparator, droppedBefore()edge, off-by-one in-degree threshold, skipped cycle check, swapped Reads/Writes direction), one at a time, rebuilt, ranSystemSchedulerTest.*. All 5 caught. Good result — every mutation reverted, nothing else changed.Net diff
Only item 1 changes shipped code — everything else is either blocked-and-documented (item 2) or a reverted spike (items 3, 4):
OloEngine/src/OloEngine/Task/TaskPrivate.h— the cache-line fix.docs/agent-rules/spinlock-payload-cache-line-separation.md— new, the reusable lesson.CLAUDE.md— one-line link to the new doc.Follow-up issues filed
OloEngine.libexceeds the 4GiB COFF archive limit (LNK1248), CI blind spot.-Wglobal-constructors/-Wexit-time-destructorssites in our own code.Test plan
OloEngine-Testsfull suite (msvc Debug): 5546/5553 passed, 6 skipped (expected — GPU/app-launch tests unavailable), 1 known/tracked failure unrelated to this change (AtmosphereVisualEvidenceTest.DayNightWeatherMatrixRendersAndHoldsContracts).SystemSchedulerTest.*/SystemSchedulerParallelTest.*: 26/26 passed on the final (unmutated) tree./W4stays clean —/wd4324is already suppressed project-wide for exactly thisalignaspadding pattern.clangclpreset before being reverted.🤖 Generated with Claude Code
Summary by CodeRabbit
Performance
Documentation