From fe45f2cdedc7dc503ab5b19b95c358500ee014c5 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 15:12:45 +0200 Subject: [PATCH 1/3] core: stop a serial dispatch paying for a std::deque it never fills (fixes #660) `StrandExecutor` erases a model's `Strand` as soon as it drains, so a workload that dispatches one action at a time against a model rebuilds the whole strand on every call. Re-measured on 7a343e6f with tests/bench/bench_dispatch_allocations.cpp (the instrument #661 landed, which the original figure predates), x86-64 Linux, GCC 16.2.1 / libstdc++, -O2, per-line attribution from a `backtrace()` in the counting hook symbolised through `dladdr` + `addr2line`: #7 32 bytes strand.hpp:104 _strands map node #8 152 bytes strand.hpp:106 make_shared #9 64 bytes the Strand's std::queue deque map #10 512 bytes the Strand's std::queue first buffer heap allocations total : 4159 (20.80 per call) bytes allocated total : 398072 (1990.4 per call) 760 of 1990 bytes, 38%. But 576 of those 760 are not the strand's lifetime at all: they are libstdc++'s `std::deque` allocating a node map and a 512-byte first buffer *in its default constructor*, for a queue that in this workload never holds more than one task. So this takes the third of the ticket's three directions -- shrink what is rebuilt -- and leaves the lifetime policy alone. `PendingQueue` holds the head task inside the `Strand` and constructs a `std::deque` behind it only when a second task is genuinely queued. Nothing about the locking changes: the erase still fires when `empty()` becomes true, still under the `{_mapMtx, strand->mtx}` pair whose atomicity the comments at :81 and :167 record a previous defect forcing into shape. heap allocations total : 3786 (18.93 per call) bytes allocated total : 279208 (1396.0 per call) #7 32 bytes _strands map node #8 120 bytes make_shared 2 allocations and 608 bytes per local dispatch, 30% of the total. The two remaining strand allocations are inherent to the erase, and keeping the slot alive to remove them trades this churn for a per-model entry nothing reclaims -- `StrandExecutor` has no deregistration hook. That trade is not made here and is not smuggled in. Verification. Reproduced, not inferred: both the before and after figures above are runs on this tree, and the four backtraces are real output, not a reading of the code. The overflow path the change adds is covered -- `_overflow->push_back` -> `push_front` fails 3 test cases including the 50-task FIFO assertion, and disabling the refill-from- overflow arm fails 8. Full gcc-release suite: 1561 cases pass. The race, not the suite, is the hazard, so: full ctest under the clang-tsan preset (clang 22.1.8, MORPH_BUILD_NET=ON, MORPH_BUILD_OFFLINE_SQLITE=ON, cmake/tsan.supp, the `Linux / clang-tsan` leg's own flags) -- 1801/1801 pass, all six binaries confirmed instrumented by check_sanitizer_instrumentation.sh. And that configuration was shown to be able to see this specific hazard: restoring the pre-fix two-step drain-and-erase makes TSan report a data race whose two stacks both run through `scheduleNext`'s lambda into `LoadCountModel::execute` -- two strands for one key, which is exactly the defect the combined lock exists to prevent. Not verified: the time cost -- nothing here claims a latency change. Not verified on libc++, MSVC or the WASM toolchain; the byte figures are libstdc++'s and the `std::deque` behaviour they turn on is an implementation choice, so the saving may differ elsewhere. The `Kanban / ThreadSanitizer` leg (Qt + ladder) was not run locally; the `Linux / clang-tsan` leg was, and it is the one that covers strand.hpp's own tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/core/executor.md | 42 ++++++++++++++++++-- include/morph/core/strand.hpp | 75 +++++++++++++++++++++++++++++++++-- 2 files changed, 110 insertions(+), 7 deletions(-) diff --git a/docs/spec/core/executor.md b/docs/spec/core/executor.md index 340e1867c..0828f0537 100644 --- a/docs/spec/core/executor.md +++ b/docs/spec/core/executor.md @@ -207,6 +207,22 @@ Internally `StrandExecutor` maintains a map of `ModelId → shared_ptr` mutex, a pending queue, and a `running` flag. The executor also tracks an `_inFlight` counter (guarded by the map mutex) that the destructor waits on. +The pending queue is `StrandExecutor::PendingQueue`, not `std::queue`. It is a +FIFO with the head task stored **inside** the `Strand` and a lazily constructed +`std::deque` behind it, and it exists purely to make the common case cheaper: +because the drain step below destroys the whole `Strand` as soon as the queue +empties, a serial workload (one action at a time, each waited out) rebuilds the +queue on every dispatch and puts exactly one task in it — and libstdc++'s +`std::deque` allocates its node map *and* a 512-byte first buffer in its +default constructor, whether or not anything is ever pushed. See +[Lifetime & ownership](#lifetime--ownership) below for the measurement. +`PendingQueue` does no locking of its own; every access is +under the owning `Strand::mtx`, exactly as the `std::queue` it replaced was, +and it tracks occupancy with a flag rather than by testing the callable, so an +empty `std::function` is queued and dispatched like any other. It changes no +lifetime or locking rule: the erase still fires when `empty()` becomes true, +still under the `{_mapMtx, strand->mtx}` pair. + **`_inFlight` is incremented with the *decision* to dispatch, not lazily.** `post()` increments `_inFlight` in the same `_mapMtx` critical section that flips `running` true and decides to schedule, before releasing the lock; the re-arm @@ -323,9 +339,29 @@ The strand map is self-cleaning: when a strand drains (its `pending` queue is empty), `scheduleNext` clears `running` and erases the map entry under the combined `{_mapMtx, strand->mtx}` lock. Live memory therefore tracks the set of *currently active* models rather than every model ever seen — there is no -per-model registration to leak. The cost is allocation churn: a model that is -posted to in bursts allocates a fresh `Strand` each time its queue empties and -refills, rather than keeping one long-lived strand per key. +per-model registration to leak. + +**The cost is allocation churn, and it is bounded rather than removed.** A +model posted to serially — one action at a time, each waited out — never has a +task queued at the instant the previous one finishes, so it never keeps a +strand: every dispatch takes `post()`'s `if (!slot)` branch and rebuilds the +map node and the `Strand`. Measured on `7a343e6f` with +`tests/bench/bench_dispatch_allocations.cpp` (see +[testing_strategy.md](../testing_strategy.md)), x86-64 Linux, GCC 16.2.1 / +libstdc++, `-O2`, that came to **4 allocations and 760 of the 1990 bytes** a +local `execute` round trip cost — 38% of the bytes, for a strand that is +rebuilt and thrown away. 576 of those bytes were not the strand at all but +`std::queue`'s `std::deque` eagerly allocating a node map and a 512-byte first +buffer in its default constructor. Replacing that container with +`PendingQueue`, which holds the head task inline, cut the strand's share to **2 +allocations and 152 bytes** and the whole round trip to 18.9 allocations / +1396 bytes (morph#660). + +The two allocations that remain — the map node and the `Strand` itself — are +inherent to the erase: removing them means keeping the slot alive across the +drain, which trades this churn for a per-model entry that nothing reclaims, +since `StrandExecutor` has no deregistration hook. That trade is deliberately +not made here; the erase is what bounds the map. ## Thread safety diff --git a/include/morph/core/strand.hpp b/include/morph/core/strand.hpp index 65930a236..44fb25049 100644 --- a/include/morph/core/strand.hpp +++ b/include/morph/core/strand.hpp @@ -3,13 +3,14 @@ #pragma once #include #include +#include #include #include #include #include -#include #include #include +#include #include "../attributes.hpp" #include "executor.hpp" @@ -130,10 +131,77 @@ class StrandExecutor { } private: + /// @brief FIFO of tasks queued on one strand, with the head task held inline. + /// + /// Behaviourally a `std::queue>` restricted to the + /// three operations the strand uses, and used under exactly the same + /// discipline: every call happens with the owning `Strand::mtx` held, so + /// this type does no locking of its own. + /// + /// It exists because of what the *container* cost, not what the strand + /// did with it. The drain-and-erase step in `scheduleNext` destroys the + /// whole `Strand` as soon as the queue empties, so a workload that + /// dispatches one action at a time against a model builds a fresh queue on + /// every call and puts exactly one task in it. libstdc++'s `std::deque` + /// allocates its node map *and* a first 512-byte buffer in its default + /// constructor, so that came to 576 bytes of the 760 the strand cost per + /// local dispatch (morph#660). Holding the head task in the strand makes + /// that case allocation-free; the overflow deque is constructed only when + /// a second task is genuinely queued behind a running one, after which the + /// cost is the deque's as before. + /// + /// This changes no lifetime or locking rule: the erase still happens when + /// `empty()` becomes true, still under the `{_mapMtx, strand->mtx}` pair. + class PendingQueue { + public: + /// @brief Reports whether the queue holds no task. + /// @return `true` when nothing is queued. + [[nodiscard]] bool empty() const noexcept { return !_hasHead; } + + /// @brief Appends @p task to the back of the queue. + /// @param task Callable to queue. An *empty* `std::function` is queued + /// and later dispatched like any other: occupancy is + /// tracked by a separate flag rather than by testing the + /// callable, so this type never silently drops one. + void push(std::function&& task) { + if (!_hasHead) { + _head = std::move(task); + _hasHead = true; + return; + } + if (!_overflow) { + _overflow = std::make_unique>>(); + } + _overflow->push_back(std::move(task)); + } + + /// @brief Removes the task at the front of the queue and returns it. + /// @return The front task. + /// @pre `!empty()`. + std::function pop() { + std::function task = std::move(_head); + if (_overflow && !_overflow->empty()) { + _head = std::move(_overflow->front()); + _overflow->pop_front(); + } else { + // A moved-from std::function is valid but unspecified; clear it + // explicitly so the slot holds no captured state while idle. + _head = nullptr; + _hasHead = false; + } + return task; + } + + private: + std::function _head; + std::unique_ptr>> _overflow; + bool _hasHead = false; + }; + struct Strand { IExecutor* base = nullptr; std::mutex mtx; - std::queue> pending; + PendingQueue pending; bool running = false; }; @@ -151,8 +219,7 @@ class StrandExecutor { std::function task; { std::scoped_lock const lock{strand->mtx}; - task = std::move(strand->pending.front()); - strand->pending.pop(); + task = strand->pending.pop(); } try { task(); From 08401793eb3e4d1fda04024afc59e36bacececce Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 15:12:56 +0200 Subject: [PATCH 2/3] gate: repoint the strand.hpp branch-allowlist line hint after PendingQueue (refs #660) Mechanical follow-up to the previous commit, kept separate because it changes no behaviour and no argument. `PendingQueue` inserts 67 lines above the allowlisted `if (iter != _strands.end() && iter->second == strand) {`, so the ST1 entry's `line: 192` hint drifted to 259. The `source` text is unchanged and matches exactly one line in the file, so the disposition itself is untouched -- this is the "text still matches, update the hint" case check_branch_coverage.py's resolve_allowlist_source_line() names, not a re-reading of the invariant. Verified by re-running that resolver's own rule over every entry in branch_partial_allowlist.json, error_path_allowlist.json and mutation_survivors.json against this tree: one drifted entry before, zero after. Not verified: check_branch_coverage.py itself, which needs a coverage profile this lane did not produce. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- scripts/branch_partial_allowlist.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index c8db3ae2f..473029fe1 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -74,7 +74,7 @@ }, { "file": "include/morph/core/strand.hpp", - "line": 192, + "line": 259, "source": "if (iter != _strands.end() && iter->second == strand) {", "reason": "Unreachable by construction given this class's lock discipline (core audit finding ST1, resolved to (b) by a concurrency-focused review pass after an initial (a)/(b)-undecided pass). `_strands` has exactly two mutation sites: `post()`'s insert-if-absent (this file, `if (!slot) { slot = make_shared(); }`) and this exact block's own erase a few lines below, both under `_mapMtx`. At most one lambda per `Strand` runs at a time (`post()` only schedules when `!strand->running`, and re-arming happens only through this same lambda's own `more` branch), so dispatch for one `Strand` is strictly serial; and only a strand's own currently-running lambda can erase its map entry (the erase fires only in the `!more` branch for the entry this frame just found under `_mapMtx`, and a concurrent `post(key)` while this lambda runs can only push onto the existing `Strand`, never replace it). Together these force `_strands.find(key)` to yield this exact strand whenever this line runs, so `iter->second == strand` cannot be false. No stress test needed: one was considered, but given the strength of the lock-discipline argument it would spend CI time re-confirming an already-proven invariant rather than searching for an unknown one." }, From be0c4a6f4872dff99971f4fc746a911d78920345 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 15:28:09 +0200 Subject: [PATCH 3/3] cmake: say what -Wno-shadow-uncaptured-local actually suppresses (refs #662) The comment on this suppression said "lambda param shadowing an uncaptured local". That is one of four constructs the flag owns. Measured on clang 22.1.8, `clang++ -std=c++23 -fsyntax-only -Weverything`, one construct per function: probe.cpp:9:22: declaration shadows a local variable [-Wshadow-uncaptured-local] probe.cpp:16:24: declaration shadows a local variable [-Wshadow-uncaptured-local] probe.cpp:23:16: declaration shadows a local variable [-Wshadow-uncaptured-local] probe.cpp:30:16: declaration shadows a structured binding [-Wshadow-uncaptured-local] probe.cpp:37:11: declaration shadows a local variable [-Wshadow] probe.cpp:44:48: declaration shadows a local variable [-Wshadow] -- lambda parameter, ordinary local inside a lambda, init-capture over a local, init-capture over a structured binding; and, still enforced, a plain nested block and a shadow of a variable the lambda *does* capture. So the group is "any declaration inside a lambda with no capture-default that shadows an uncaptured enclosing local or structured binding", which includes the `[x = std::move(x)]` idiom this codebase uses throughout. A suppression whose stated reason is narrower than its effect is a defect on its own, independent of any cleanup, so the comment is corrected here and nothing else changes. It is corrected rather than narrowed because narrowing to the one named construct is not expressible: clang has no finer flag than -Wshadow-uncaptured-local, so the only alternatives are all-or-nothing plus per-site suppressions. And all-or-nothing is not a one-line diff. Measured on this tree, clang 22.1.8, clang-debug with NET/QT/FORMS_QML/OFFLINE_SQLITE/LOAD_TESTS/HMAC_EXAMPLES/LADDER/ BANK_EXAMPLE configured and -Werror off, with the flag removed: 4856 diagnostics, 41 distinct sites, 16 first-party files 35 of the 41 are in headers, which is why the emission count is two orders of magnitude larger. Eight each in core/backend.hpp and core/remote.hpp, four each in core/bridge.hpp and core/completion.hpp. No dependency is affected; they arrive via -isystem. **This does not close #662.** That issue's own condition is "a diagnostic in this class fails a leg that a developer can run locally, demonstrated by mutating the code and watching that leg go red", and nothing here changes which leg enforces what. It stays open, the 41-site cleanup it needs is filed separately, and folding either into this commit is what AGENTS.md says not to do. Two things this measurement says that #662 does not. First, the split is not "structured binding versus local variable": the flag routes on whether the shadowing declaration sits in an uncapturing lambda, not on what kind of entity is shadowed -- rows 3 and 4 above differ only in the shadowed entity and land in the same group. Second, the tree now has **zero** structured-binding shadows (morph#661 fixed the only four), so the WASM leg's exclusive enforcement of that row currently enforces nothing in practice; it is a trap for the next one written, not a live gate. Verified: the six-row table and the 4856/41/16 figures are real output from this workstation, re-derived from the build log rather than transcribed. `cmake --preset clang-debug` reconfigures clean afterwards with the flag still on all 355 compile commands. Not verified: anything about emsdk -- no emsdk toolchain is available here, so the WASM claim in the comment is cited to the CI log it came from and labelled as such. Not verified: which clang release reclassified the structured-binding case; only clang 22.1.8 was available locally. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- cmake/compiler_options.cmake | 48 +++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/cmake/compiler_options.cmake b/cmake/compiler_options.cmake index 7b38e390f..40e25aaf1 100644 --- a/cmake/compiler_options.cmake +++ b/cmake/compiler_options.cmake @@ -157,8 +157,54 @@ elseif(MORPH_COMPILER_FAMILY STREQUAL "Clang") # oversight this warning should flag. _morph_clang_suppression_if_supported(-Wno-missing-designated-field-initializers) _morph_clang_suppression_if_supported(-Wno-nrvo) # not eliding a trivial-type copy on return + # -Wshadow-uncaptured-local does NOT mean "a lambda parameter shadowing an + # uncaptured local", which is what this line claimed until morph#662. It is + # clang's group for *any* declaration inside a lambda with no + # capture-default that shadows an enclosing local the lambda did not + # capture — the parameter case is one of four. Measured on clang 22.1.8, + # `clang++ -std=c++23 -fsyntax-only -Weverything`, one construct per + # function: + # + # [](int value) { ... } shadows a local variable [-Wshadow-uncaptured-local] + # [] { int value = 2; ... } shadows a local variable [-Wshadow-uncaptured-local] + # [value = 7] { ... } shadows a local variable [-Wshadow-uncaptured-local] + # [first = first + 1] { ... } shadows a structured binding [-Wshadow-uncaptured-local] + # { int value = 2; } (no lambda) shadows a local variable [-Wshadow] + # [value] { int value = inner; ... } shadows a local variable [-Wshadow] + # + # So the init-capture cases — including the `[x = std::move(x)]` idiom this + # codebase uses throughout to move state into a continuation — are + # suppressed here, and the last two rows show what is still enforced: + # -Wshadow (GCC list below, and via -Weverything here) keeps every shadow + # that is not inside an uncapturing lambda. + # + # The group cannot be narrowed to the one construct the old comment named; + # clang has no finer flag, so the alternatives are all-or-nothing plus + # per-site suppressions. Dropping it entirely is a real cleanup, not a + # one-line diff: measured on this tree with every optional feature + # configured (clang 22.1.8, clang-debug + NET/QT/FORMS_QML/OFFLINE_SQLITE/ + # LOAD_TESTS/HMAC_EXAMPLES/LADDER/BANK_EXAMPLE, -Werror off), removing this + # line yields **4856 diagnostics over 41 distinct sites in 16 first-party + # files** — the emission count is that much larger than the site count + # because 35 of the 41 are in headers, re-reported once per translation + # unit that includes them. By file: 8 each in core/backend.hpp and + # core/remote.hpp, 4 each in core/bridge.hpp and core/completion.hpp, 1 + # each in core/registry.hpp, core/callback_scope.hpp, offline/ + # sync_worker.hpp and qt/qt_executor.hpp, 3 across two tests/ files and 10 + # across six examples/ files. No dependency is affected (they arrive via + # -isystem). That cleanup is tracked separately; do not fold it into an + # unrelated change. + # + # One consequence is recorded rather than fixed here (morph#662, still + # open): emsdk 3.1.56's older clang files `declaration shadows a structured + # binding` under plain -Wshadow instead, so the WASM leg is the only leg + # that enforces that one row of the table above. That statement is read off + # the CI log of run 35573507189, not reproduced locally — no emsdk + # toolchain is available here. It also currently enforces nothing in + # practice: morph#661 fixed the only four structured-binding shadows in the + # tree, and the measurement above found zero remaining. list(APPEND MORPH_WARNING_FLAGS - -Wno-shadow-uncaptured-local # lambda param shadowing an uncaptured local + -Wno-shadow-uncaptured-local -Wno-documentation-unknown-command -Wno-unsafe-buffer-usage # flags all pointer arithmetic; needs a hardened API )