core: recycle the strand's map node instead of rebuilding it every serial dispatch, with no deregistration hook (fixes #670)

docs/spec/concurrency_and_lifetimes.md

@@ -104,7 +104,11 @@ turns it into a set of per-key serial queues:
   may run concurrently on different pool threads. This is what removes the need
   for per-model mutexes.
 - Each strand is a `shared_ptr<Strand>` in a map guarded by `_mapMtx`. When a
-  strand's queue drains, the map entry is erased. The invariant is: **at most one
+  strand's queue drains, the map entry is removed — `extract`ed into a
+  single-slot `_spare` the next miss re-keys, which recycles the node's memory
+  without changing when the entry leaves the map (see
+  [`core/executor.md`](core/executor.md), "Lifetime & ownership"). The
+  invariant is: **at most one
   live strand per `ModelId`, and any `running` strand is the one currently in the
   map** — that is what keeps a key's tasks from overlapping. Both sides that can
   break it hold `_mapMtx` across their *whole* decision: `post()` takes `_mapMtx`,

docs/spec/core/executor.md

@@ -205,22 +205,26 @@ lifetime of the model. It supports three-way comparison and can be used as an
 Internally `StrandExecutor` maintains a map of `ModelId → shared_ptr<Strand>`
 (shared state per key). A `Strand` holds a pointer to the base `IExecutor`, a
 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.
+`_inFlight` counter (guarded by the map mutex) that the destructor waits on,
+and a single-slot `_spare` node handle (also guarded by the map mutex) that
+recycles one detached map entry — see
+[Lifetime & ownership](#lifetime--ownership).
 
 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
+because the drain step below detaches the whole map entry as soon as the queue
+empties, a serial workload (one action at a time, each waited out) starts from
+an empty 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,
+lifetime or locking rule: the removal still fires when `empty()` becomes true,
 still under the `{_mapMtx, strand->mtx}` pair.
 
 **`_inFlight` is incremented with the *decision* to dispatch, not lazily.**
@@ -336,16 +340,15 @@ data race the `_inFlight` wait exists to prevent. Callers must ensure all task
 sources are shut down before the `StrandExecutor` is destroyed.
 
 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
+empty), `scheduleNext` clears `running` and removes 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, 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
+**The cost was allocation churn.** 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 missed in the map and
+rebuilt 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
@@ -357,11 +360,35 @@ buffer in its default constructor. Replacing that container with
 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.
+**The remaining two allocations — the map node and the `Strand` itself — are
+recycled rather than removed (morph#670).** They looked inherent to the erase:
+removing them appeared to mean keeping the slot alive across the drain, which
+would trade the churn for a per-model entry nothing reclaims, since
+`StrandExecutor` has no deregistration hook. That framing turned out to be
+avoidable. The entry still leaves the map at exactly the same moment, under
+exactly the same locks; the drain simply calls `extract` instead of `erase` and
+parks the detached node in a single-slot `_spare` member, and the next
+`post()` that misses re-keys that node and inserts it back. The map is still
+bounded by the removal — `_spare` holds **at most one** node, is guarded by
+`_mapMtx` like the map itself, and is freed with the executor.
+
+Reusing the parked node's `Strand` object is guarded additionally by
+`use_count() == 1`: the recycled node is then the only owner, so no strand task
+can still reach the object and reusing it is indistinguishable from
+constructing a new one. When that guard fails — a finishing strand lambda still
+holds its `shared_ptr` when the next `post()` looks — a fresh `Strand` is
+constructed exactly as before and only the node is recycled. To make the guard
+usually hold, the strand lambda drops its `shared_ptr` immediately after the
+drain block rather than at its own destruction; nothing after that point
+touches the strand. That timing affects *whether* the object is recycled, never
+whether the recycling is safe.
+
+Re-measured on `7d4ca453` (this change's base) with the same instrument,
+x86-64 Linux, **clang 22.1.8 / libstdc++ 16.2.1, Release**: the round trip went
+from **18.90 allocations / 1394.8 bytes** to **16.95 / 1244.6** — the full 2
+allocations and ~150 bytes the strand had left. Six alternating runs of each
+binary; spread within 0.1 allocations and 2 bytes per call. The magnitude is
+libstdc++-specific, as it was for morph#660.
 
 ## Thread safety
 
@@ -375,8 +402,9 @@ concurrently.
 - `MainThreadExecutor` guards its queue with `_m`. `post()` may be called from
   any thread, but `runFor()` must be called only from the single owning
   ("main") thread; concurrent `runFor()` calls are not supported.
-- `StrandExecutor` uses two lock levels: `_mapMtx` protects the `_strands` map
-  and the `_inFlight` counter, and each `Strand::mtx` protects that strand's
+- `StrandExecutor` uses two lock levels: `_mapMtx` protects the `_strands` map,
+  the `_spare` recycled node and the `_inFlight` counter, and each
+  `Strand::mtx` protects that strand's
   `pending` queue and `running` flag. Both operations that can break the
   per-key invariant hold `_mapMtx` across their whole decision: `post()` takes
   `_mapMtx`, does the slot lookup/create, and then — still holding `_mapMtx` —

include/morph/core/strand.hpp

@@ -102,12 +102,11 @@ class StrandExecutor {
             // is uncontended; an existing strand's mtx can only be held
             // elsewhere under the same _mapMtx-first order, so no deadlock.
             std::scoped_lock const mapLock{_mapMtx};
-            auto& slot = _strands[key];
-            if (!slot) {
-                slot = std::make_shared<Strand>();
-                slot->base = _base;
+            auto slotIter = _strands.find(key);
+            if (slotIter == _strands.end()) {
+                slotIter = installStrand(key);
             }
-            strand = slot;
+            strand = slotIter->second;
             std::scoped_lock const strandLock{strand->mtx};
             strand->pending.push(std::move(task));
             if (!strand->running) {
@@ -126,7 +125,7 @@ class StrandExecutor {
             }
         }
         if (schedule) {
-            scheduleNext(strand, key);
+            scheduleNext(std::move(strand), key);
         }
     }
 
@@ -205,6 +204,64 @@ class StrandExecutor {
         bool running = false;
     };
 
+    /// @brief The `ModelId` → strand map. Named so the recycled node type can be.
+    using StrandMap = std::unordered_map<ModelId, std::shared_ptr<Strand>, ModelIdHash>;
+
+    /// @brief Returns an iterator to the strand for @p key, creating the entry.
+    ///
+    /// **Precondition:** the caller holds `_mapMtx` and has already
+    /// established that `key` has no entry.
+    ///
+    /// This is a pure allocation optimisation and changes no lifetime or
+    /// locking rule. The drain step in `scheduleNext` removes the whole map
+    /// entry as soon as the queue empties, so a workload that dispatches one
+    /// action at a time against a model paid for a fresh map node *and* a
+    /// fresh `make_shared<Strand>` on every call — the 2 allocations / 152
+    /// bytes that were left after morph#660 took the container's share.
+    /// Rather than keep the slot alive across the drain (which would need a
+    /// deregistration hook and would trade this churn for a per-model entry
+    /// nothing reclaims), the drain `extract`s the node instead of erasing it
+    /// and parks it in `_spare`, and this re-keys and re-inserts that one
+    /// node. The entry still leaves the map at the same point under the same
+    /// locks, so the map is bounded exactly as before; `_spare` holds at most
+    /// one node and is freed with the executor.
+    ///
+    /// Reusing the parked node's `Strand` as well is guarded by sole
+    /// ownership. `use_count() == 1` means the recycled node holds the only
+    /// reference, so nothing else can reach the object and reusing it is
+    /// indistinguishable from constructing a new one. That is the whole
+    /// argument, and it is deliberately not "the previous owner makes no
+    /// further access": a strand lambda that is still finishing does hold a
+    /// reference, and when it does, this constructs a fresh `Strand` exactly
+    /// as before and recycles only the node.
+    ///
+    /// The parked strand needs no reset. It is only ever extracted from a
+    /// strand observed `!running` with an empty `pending` under
+    /// `{_mapMtx, strand->mtx}`, which is the state a fresh one is in. A
+    /// runtime re-check of that here would be an arm nothing can take, so it
+    /// is written down rather than branched on.
+    /// @param key Model identifier to install a strand for.
+    /// @return Iterator to the entry for @p key.
+    StrandMap::iterator installStrand(ModelId key) {
+        if (!_spare) {
+            auto const iter = _strands.emplace(key, std::make_shared<Strand>()).first;
+            iter->second->base = _base;
+            return iter;
+        }
+        _spare.key() = key;
+        auto& reused = _spare.mapped();
+        if (reused.use_count() != 1) {
+            reused = std::make_shared<Strand>();
+        }
+        reused->base = _base;
+        // `insert` consumes the node. Were the precondition ever violated it
+        // would instead hand the node back inside the returned object, which
+        // frees it — the same fate the old `erase` gave it — and `position`
+        // would name the existing entry, so the caller is right either way
+        // and there is nothing to branch on.
+        return _strands.insert(std::move(_spare)).position;
+    }
+
     /// @brief Dispatches one strand lambda onto the base executor.
     ///
     /// **Precondition:** the caller must have already incremented `_inFlight`
@@ -214,8 +271,15 @@ class StrandExecutor {
     /// with the *decision* (rather than here) closes the window where
     /// `~StrandExecutor` could observe `_inFlight == 0` between the decision and
     /// this dispatch and destroy `_strands` out from under us.
-    void scheduleNext(const std::shared_ptr<Strand>& strand, ModelId key) {
-        strand->base->post([this, strand, key] {
+    void scheduleNext(std::shared_ptr<Strand> strand, ModelId key) {
+        // Read `base` out before the capture list moves `strand` into the
+        // lambda: `strand->base` and the lambda's construction are
+        // unsequenced within one call expression, so reading through the
+        // moved-from pointer would be a real hazard rather than a stylistic
+        // one. The lambda's capture is non-const (hence `mutable` and the
+        // by-value parameter) so the drain below can release it early.
+        IExecutor* const base = strand->base;
+        base->post([this, strand = std::move(strand), key]() mutable {
             std::function<void()> task;
             {
                 std::scoped_lock const lock{strand->mtx};
@@ -257,7 +321,19 @@ class StrandExecutor {
                     strand->running = false;
                     auto iter = _strands.find(key);
                     if (iter != _strands.end() && iter->second == strand) {
-                        _strands.erase(iter);
+                        // `extract`, not `erase`: same removal, same moment,
+                        // same locks — the entry leaves the map here exactly as
+                        // before, and every reason the erase had to happen
+                        // under {_mapMtx, strand->mtx} still applies unchanged.
+                        // The difference is only that the detached node's
+                        // memory is parked for `installStrand` to re-key
+                        // instead of being returned to the allocator. Any node
+                        // already parked is freed by this assignment, so at
+                        // most one is ever held. Freeing it runs no user code
+                        // under these locks: a parked strand was parked
+                        // because its pending queue was empty, so there is no
+                        // captured task left to destroy.
+                        _spare = _strands.extract(iter);
                     }
                 } else {
                     // Account for the re-armed dispatch *before* releasing
@@ -271,6 +347,24 @@ class StrandExecutor {
             }
             if (more) {
                 scheduleNext(strand, key);
+            } else {
+                // Drop this run's co-ownership here rather than leaving it to
+                // the lambda's destruction a few lines below. Nothing after
+                // this point touches the strand, and releasing it early is
+                // what lets `installStrand` see `use_count() == 1` on the
+                // node just parked in `_spare`: the next post() for this key
+                // is typically already blocked on _mapMtx when the block above
+                // releases it, so a reference held until the lambda dies would
+                // usually still be there when that post looks. This only
+                // affects *whether the object is recycled*, never whether the
+                // recycling is safe — a post that looks too early simply sees
+                // two owners and constructs a fresh Strand.
+                //
+                // Safe to be the last owner here: no lock is held (the block
+                // above released both), so this never destroys a mutex it is
+                // standing on. If the extract above did run, `_spare` owns the
+                // strand and this merely decrements.
+                strand.reset();
             }
             // Decrement after all map access is done; wake destructor if it is waiting.
             {
@@ -286,7 +380,14 @@ class StrandExecutor {
     std::mutex _mapMtx;
     std::condition_variable _cv;
     int _inFlight{0};
-    std::unordered_map<ModelId, std::shared_ptr<Strand>, ModelIdHash> _strands;
+    StrandMap _strands;
+    /// @brief The one detached map node kept for reuse. Guarded by `_mapMtx`.
+    ///
+    /// Declared after `_strands` so it is destroyed first: the node owns
+    /// storage obtained from the map's allocator, and returning it before the
+    /// container goes away keeps that ordering obvious even though the default
+    /// allocator is stateless.
+    StrandMap::node_type _spare;
 };
 
 }  // namespace morph::exec::detail

scripts/branch_partial_allowlist.json

@@ -74,9 +74,9 @@
     },
     {
       "file": "include/morph/core/strand.hpp",
-      "line": 259,
+      "line": 323,
       "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<Strand>(); }`) 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."
+      "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: the insert-if-absent `post()` reaches through `installStrand` (this file) and this exact block's own removal a few lines below -- an `extract` into `_spare` since morph#670, which detaches the entry at the same point the `erase` did -- 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; a node parked in `_spare` is out of the map, so `find` cannot return 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."
     },
     {
       "file": "include/morph/core/backend.hpp",