diff --git a/cmake/compiler_options.cmake b/cmake/compiler_options.cmake index 7b38e390..40e25aaf 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 ) diff --git a/docs/spec/core/executor.md b/docs/spec/core/executor.md index 340e1867..0828f053 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 65930a23..44fb2504 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(); diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index c8db3ae2..473029fe 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." },