core: StrandExecutor rebuilds a model's Strand on every serial dispatch -- 4 allocations and 752 of the 1995 bytes per local call

[Yaraslaut]

The finding

StrandExecutor::post erases a model's Strand from _strands as soon as it
drains empty, so a workload that dispatches one action at a time against a
model allocates and frees a whole Strand on every call: the map node, the
make_shared<Strand> control block, and the two allocations
std::queue<std::function<void()>>'s deque makes for its map and its first
buffer.

Measured, on f24e225a, with tests/bench/bench_dispatch_allocations.cpp
(morph_bench_alloc, -DMORPH_BUILD_LOAD_TESTS=ON) built -O2 -g -rdynamic
with a backtrace() added to its counting hook and symbolised with
addr2line -f -i -C. x86-64 Linux, GCC 16.2.1 / libstdc++. Workload:
Ping{int} -> Pong{int} through LocalBackend on a one-thread pool, 50
warm-up calls excluded, every call waited out before the next.

Four of the 21 allocations in one steady-state local dispatch, and 752 of the
1995 bytes — 38% — are this:

  #7    32 bytes  strand.hpp:104   _strands map node
  #8   152 bytes  strand.hpp:106   make_shared<Strand>
  #9    64 bytes  <stl_deque>      the Strand's std::queue deque map
  #10  512 bytes  <stl_deque>      the Strand's std::queue first buffer

The whole census, for context:

local execute round-trips  : 200
heap allocations total     : 4165 (20.82 per call)
bytes allocated total      : 398912 (1994.6 per call)

Why it happens

scheduleNext's drain-and-erase block decides "keep running vs.
drain-and-erase" atomically under _mapMtx + strand->mtx, and when the
pending queue is empty it erases the map entry. That erase is correct and
load-bearing — the comment above it records the race that made two strands
for one key run concurrently — and it is also what makes the next post() for
the same model take the if (!slot) branch and build everything again. Serial
dispatch never leaves a task queued at the moment the previous one finishes, so
it never keeps a strand.

Under concurrent load the strand stays armed and the cost amortises; this is a
serial-workload cost, which is also the shape a desktop GUI produces.

Verification status

• Reproduced, by the measurement above, on one revision, one toolchain, one
  platform.
• Read, not measured: that the erase is what causes the re-creation. The
  backtraces place the allocations in post()'s if (!slot) branch on every
  counted call, which is only reachable when the previous call erased the slot,
  but I did not instrument the erase itself.
• Not measured: the time cost. 752 bytes of churn per call is a
  malloc/free pair set, not necessarily a visible latency change; nothing here
  claims one.
• Not measured: what a concurrent workload costs, or what fraction of a
  realistic ladder workload is serial.

Directions, none of them free

1. Keep the slot, drop only the state. Leave an empty Strand in
   _strands and let a later post() re-arm it. Cost: a per-model entry that
   is never reclaimed until deregisterModel, which is a leak of bounded size
   for a long-lived process with many short-lived models — and the erase is
   currently what bounds it.
2. Shrink what is rebuilt. std::queue<std::function<void()>>'s deque
   accounts for 576 of the 752 bytes. A small-vector or an intrusive queue
   would cut most of the cost without touching the lifetime rules at all, and
   is the least invasive option.
3. Pool the Strand objects. Keeps the erase semantics and reuses the
   allocation; more machinery than (2) for a similar saving.

Any change here must preserve the atomicity the drain-and-erase block
documents: the re-arm and the erase must stay one decision under _mapMtx, or
two strands for one key can run model tasks concurrently.

What would change the verdict

• Close if a measurement shows the per-call time cost is in the noise for
  the intended workloads and the allocator churn is acceptable — 752 bytes of
  short-lived churn per dispatch is headroom, not a defect.
• Raise in priority if a serial GUI workload is shown to be allocation
  bound, or if morph#572's dispatch-cost work goes ahead: this is the largest
  single cluster in that census and the only one that needs no public interface
  change.

Found while re-measuring morph#572; filed separately rather than folded in,
since #572's scope is Bridge::executeVia's marshalling and the Completion
pair, not StrandExecutor's lifetime policy.

[Yaraslaut]

Triage: valid — mechanism confirmed by reading, magnitude taken from the filer

The mechanism is in include/morph/core/strand.hpp and it is exactly as described: slot = std::make_shared<Strand>() at :106, and a drain-and-erase step in scheduleNext (:167) that removes the map entry whenever a strand empties. Serial dispatch therefore never keeps one, and each call rebuilds it.

The 4 allocations / 752 of 1995 bytes (38%) is the filer's measurement, not mine — it came from bench_dispatch_allocations, which is not yet on master (it is in PR #661). I have not reproduced it. What I have checked is that the code does what the explanation says, which is the part a reader would otherwise have to take on faith.

triage: valid, enhancement, area: core.

The constraint that makes this harder than it looks, and it is already written down

The erase is not incidental. The comments around it say why it is shaped as it is:

"the drain-and-erase step in scheduleNext makes its 'keep-running vs. erase' decision under the same {_mapMtx, strand->mtx} pair" … "(TSan: data race on destructor vs erase)" … "flip running under strand->mtx, then erase under _mapMtx opened …"

So the obvious fix — keep the slot alive and reuse it — has to preserve an atomicity property that a previous defect already forced into this shape. Anyone taking this should read those comments before writing code, and should treat "the suite passes" as insufficient: the hazard is a race, and races pass suites. A TSan run is the minimum bar, and the ladder's Kanban / ThreadSanitizer leg is the one that would catch a regression.

Sequencing

This is worth doing after #661 lands, not before — #661 carries the instrument that measures it, and without that the fix has no before/after. When it does land, the first step is re-running the bench to confirm the 752 bytes on current master, since #661 itself changes the dispatch path.

Do not batch this with #572. They look related and are not: #572 is about the remote path's cost, this is the strand map, and the #572 re-measurement showed the two have different owners. Keeping them apart is what let the 38% cluster be seen at all.

What would change the verdict

Close as invalid if the strand turns out to be reused in the configurations that matter and the bench's serial pattern is unrepresentative — i.e. if a realistic concurrent workload keeps the slot alive and this only shows up in a microbenchmark.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW