samiandClaude Sonnet 5
322a20efa5
Root cause of the M7 RSS gap (core/docs/m7-baseline.md: ~70 MB measured against a 60 MB target): SegmentBudget::confirm_slot() only checked a task's own held count against its own target -- never the engine-wide active_ sum. reallocate_locked()'s two-pass fairness allocation does bound sum(target) <= max_active_ at the moment it computes a plan, but that bound says nothing about sum(held): a task can be legitimately holding more than its own just-lowered target for a while (yield is deferred to a segment boundary, never mid-segment -- ADR 0011 A1), and another task's target can correctly rise to claim that capacity before the first task has physically released it. Both confirm_slot() calls could then succeed against their own, individually-correct targets while sum(held) exceeded max_active_ -- tools/bench heap-profile caught this directly: budget.active reading 56-86 against a total of 32. confirm_slot() now also checks active_ < max_active_, unconditionally, as a backstop that doesn't depend on any task's target bookkeeping being in sync with what every other task holds. That creates a liveness question the original design never answered: a task denied only by this new check has a target that's already correct, so it never changes again and reallocate_locked()'s plain "fire a callback when a task's target changes" mechanism never revisits it. Task gained a waiting_for_slot flag, set on exactly this denial; release_slot()/deregister_task() (the only two places that free real capacity) now hand a freed slot directly to the highest-priority waiting task via wake_one_waiter_locked(), if reallocate_locked()'s own plan didn't already produce a callback for anyone. download_task.cpp's fill_slots_locked() needed a matching fix: a woken task's stalled segments (SegState::stalled -- backed off mid-retry, its own release_slot() already called) have no live worker and never surface through Segmenter::assign_slot(), which only hands out unassigned or fresh ranges. fill_slots_locked() now restarts any stalled segment with no live worker directly (bounded by slot_target, same as its assign_slot() loop) before looking for new work; a segment it doesn't get to keeps its own scheduled retry_worker() timer as a second chance. Also fixes a real TSan-caught data race this work surfaced: SegWorker:: speed_bps was written only by its own segment's curl callback and, before Progress.speed_bps's polled-path fix, only ever read from that same thread -- safe without synchronization. snapshot_progress() reading it from whatever thread calls DownloadHandle::progress() broke that invariant (workers_mu's shared_lock protects the workers map's structure, not an individual SegWorker's fields). Now std::atomic<double> with relaxed ordering -- an informational EMA, nothing synchronizes real state on it -- rather than adding a lock to the write side. core/tests/segment/budget_test.cpp adds two tests reproducing the actual gap (budget_active_never_exceeds_max_active_segments_under_concurrent_load, budget_wait_list_wakes_a_task_whose_target_never_changed) plus a sanity baseline (budget_release_wakes_a_denied_waiter), and introduces AsyncFakeTask + TestTimer for the one existing test that drives the budget from multiple concurrent threads -- mirroring production's real dispatch (register_task()'s on_target lambda posts through host.schedule(), download_task.cpp, never a synchronous call) rather than adding reentrancy-guarding machinery to SegmentBudget itself to compensate for a synchronous test double being unlike production. See docs/adr/0017 for the full writeup, including what an earlier version of this fix got wrong chasing a same-thread reentrancy hazard that doesn't actually exist in production. core/docs/m7-baseline.md updated: the RSS number now clears the DoD line (45.41 MiB via heap-profile), root-caused rather than just re-measured. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Q3QrF7rCt21bkAjt9BCDFQ
105 lines
6.7 KiB
Markdown
105 lines
6.7 KiB
Markdown
# M7 performance baseline
|
||
|
||
Measured against `docs/04-engine-design.md` §8's targets, via `tools/bench/vdm_bench`
|
||
(see that file's header comment for the exact commands — reproduced below with their
|
||
actual output). `--preset release`, this machine, 2026-09-11 through 2026-09-12. This is
|
||
a baseline record, not a sign-off: one of the three numbers below is loopback-only rather
|
||
than measured against a real 1 Gbit link, stated plainly rather than rounded away — see
|
||
"Open gaps". The RSS number *did* fail the DoD line in the first pass through this file;
|
||
it's since been root-caused and fixed (`docs/adr/0017`), not just re-measured.
|
||
|
||
## Commands and results
|
||
|
||
```
|
||
$ cmake --preset release && cmake --build --preset release
|
||
|
||
$ bin/vdm_bench throughput --size 5G --require-mbps 940 --max-cpu-pct 8
|
||
throughput: 5368709120 bytes in 2.99s
|
||
throughput 14371.00 Mbps
|
||
cpu 196.52 % of one core
|
||
peak RSS 22.81 MiB
|
||
```
|
||
Against `tools/bench/support/local_server.hpp`'s busybox loopback server, not a real 1
|
||
Gbit link — no such link was available to test against in this environment, so
|
||
`--require-mbps`/`--max-cpu-pct` weren't meaningfully exercised here (loopback trivially
|
||
clears 940 Mbps; the 196% CPU figure reflects driving a link far faster than 1 Gbit, not
|
||
the 1-Gbit-saturated cost the target is about). This needs re-running against a real
|
||
1 Gbit peer before it can stand as the actual M1/M7 sign-off number.
|
||
|
||
```
|
||
$ bin/vdm_bench heap-profile --tasks 20 --task-size 4M --top 5
|
||
segment budget: 32 live across tasks, engine reports total=32 active=32 starved=9
|
||
heap-profile: peak RSS 45.41 MiB, 20 tasks, 8 segments assumed
|
||
```
|
||
Default `Config` (`default_segments=8`, `max_active_segments=32`, `default_buffer_bytes=1
|
||
MiB`), default `--task-size 4M`. **Now clears the 60 MB line** (45.41 MiB), after the real
|
||
fix below — root-caused, not just re-measured. Superseded the original `load` run's ~70
|
||
MiB number quoted in earlier drafts of this file; see `docs/adr/0017`.
|
||
|
||
```
|
||
$ bin/vdm_bench alloc-check --size 512M --window-s 2
|
||
alloc-check: 4 allocations in 2.00s (budget 15)
|
||
```
|
||
Clears the no-allocation-on-the-hot-path bar (docs/agents/AGENT-CORE.md) comfortably.
|
||
This number is *after* a real fix landed in the same change:
|
||
`net::HttpClient::Impl::drain_commands` was constructing an (always-allocating, in
|
||
libstdc++) `std::deque` on every worker-loop iteration regardless of whether any command
|
||
was actually pending — once per curl_multi_poll wake, i.e. on the transfer hot path. Fixed
|
||
by checking `w.queue.empty()` under the lock before touching `local` at all. Before the
|
||
fix this bench reported thousands of allocations/sec under any sustained transfer.
|
||
|
||
## ASan / UBSan / TSan (M1 DoD: "20-task load test... clean")
|
||
|
||
- `--preset dev` (ASan+UBSan) and `--preset tsan`: the full `core/` test suite (40 ctest
|
||
cases, including `veloxcore_engine_test`'s hostile-mode suite and `veloxcore_budget_test`)
|
||
and all three `tools/bench` smoke tests pass clean on both presets.
|
||
- Two real bugs were caught and fixed getting here:
|
||
- `DownloadTaskState::quiesce()` (engine shutdown / `Engine`'s destructor) cleared the
|
||
`workers` map synchronously right after issuing an async `transfer.cancel()`, racing
|
||
the HttpClient worker thread's still-in-flight write callback into a heap-use-after-free
|
||
on the segment's ring buffer — ASan-caught via `alloc-check`, which (by design) drops
|
||
its `Engine` while a download is still active. Fixed by having `quiesce()` wait for
|
||
each worker to drain itself through the same `seg_finished` path every other exit uses,
|
||
instead of tearing the map down itself.
|
||
- `SegWorker::speed_bps` (the polled-progress fix, see `engine_polled_progress_reports_
|
||
nonzero_speed`) was written only by a segment's own curl callback and, before this
|
||
session, only ever read from that same thread (`emit_progress_if_due`, called from the
|
||
same callback) — safe without synchronization. Reading it from `snapshot_progress()`
|
||
(any thread calling `DownloadHandle::progress()`) broke that invariant: `workers_mu`'s
|
||
shared_lock protects the `workers` map's structure, not an individual `SegWorker`'s
|
||
mutable fields. TSan-caught. Fixed with `std::atomic<double>` (relaxed: this is an
|
||
informational EMA, nothing synchronizes real state on it) rather than adding a lock to
|
||
the write side.
|
||
- The `tools/bench load` ctest registration still runs at reduced concurrency
|
||
(`--tasks 8 --segments 2`) under sanitizer presets (`tools/bench/CMakeLists.txt`) from
|
||
when this was written against `docs/adr/0016`'s postscript — see `docs/adr/0017`'s "Open
|
||
gaps" note: that straggler is now suspected to have been the *same* root cause as the RSS
|
||
bug, not re-verified at the DoD's full shape under `--preset tsan` in this change.
|
||
|
||
## Open gaps
|
||
|
||
1. ~~RSS is ~70 MB against a 60 MB target~~ **Fixed — see `docs/adr/0017`.** Root cause
|
||
was not, as first guessed here, an `ADR 0012` arithmetic gap or per-queued-handle curl
|
||
overhead: `SegmentBudget::confirm_slot()` only checked a task's *own* target against
|
||
its own held count, never the engine-wide `active_` sum, so it could (and under real
|
||
20-task/8-segment contention, reliably did) admit segments well past
|
||
`max_active_segments` — `heap-profile` caught it directly: `budget.active` reading
|
||
56–86 against a `total` of 32. Fixed at the budget level (the one place that can
|
||
actually enforce the invariant); `docs/adr/0012`'s own arithmetic was fine all along.
|
||
2. **Throughput/CPU numbers are loopback-only.** No 1 Gbit link was available to test
|
||
against; re-run `throughput --size 5G --require-mbps 940 --max-cpu-pct 8` against a real
|
||
one before treating this as signed off.
|
||
3. **`docs/adr/0016`**: `rate::RateLimiter`'s global-limit path (byte-rate pacing, a
|
||
different subsystem from the segment-admission bug in `docs/adr/0017`) has no fairness
|
||
ordering under heavy segment contention (a shared `TokenBucket`'s peek/commit race can
|
||
starve a waiter indefinitely) — a real, separate, still-open gap for the "global
|
||
bandwidth cap with many concurrent downloads" scenario.
|
||
4. **The TSan-only load-test straggler** noted in `docs/adr/0016`'s postscript, found
|
||
before `docs/adr/0017`'s fix landed: plausibly the *same* root cause (a segment denied
|
||
admission with nothing to wake it, worse under TSan's slowdown widening the window a
|
||
deferred yield can sit in) rather than the `CURLOPT_LOW_SPEED_TIME` guess that ADR
|
||
originally offered — not reverified at the DoD's full 20-task/8-segment shape under
|
||
`--preset tsan` in this change (the sanitizer-preset smoke registration still runs at
|
||
reduced concurrency; see `tools/bench/CMakeLists.txt`). Worth re-running before treating
|
||
it as closed.
|