Files
vdm/core/docs/m7-baseline.md
samiandClaude Sonnet 5 322a20efa5 core: fix SegmentBudget over-admission and add a wait-list wakeup
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
2026-09-12 10:58:14 +04:00

6.7 KiB
Raw Permalink Blame History

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_segmentsheap-profile caught it directly: budget.active reading 5686 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.