core: add tools/bench (throughput/load/alloc-check) and record the M7 baseline
Three subcommands in one binary, driving vdm::Engine directly (docs/04 §8): - throughput: a single download against a fast local origin (support/local_server.hpp, busybox httpd), reporting Mbps/CPU%/RSS. Gates on --require-mbps/--max-cpu-pct only when passed, so the ctest smoke registration stays a correctness check, not a hardware-dependent perf gate -- the real 1-Gbit-link sign-off is a manual/CI job (see the file's header comment). - load: N concurrent tasks against tools/testserver's `throttled` mode (support/testserver_client.hpp), reporting peak RSS via getrusage(). Paced externally rather than through the engine's own rate::RateLimiter or busybox: the limiter's pause/resume path allocates on every throttle event (would contaminate alloc-check's measurement) and under heavy segment contention was found to starve individual tasks indefinitely (see docs/adr/0016, added here); busybox couldn't sustain the DoD's ~160 concurrent connections (20 tasks * default_segments=8) reliably. The ctest registration runs at reduced concurrency under sanitizer presets -- see the CMakeLists.txt comment and the ADR's postscript. - alloc-check: operator new/delete overridden process-wide, sampling the allocation count across a steady mid-transfer window against a paced tools/testserver origin. Caught a real bug in the same change (see the http_client.cpp commit) and, by dropping its Engine mid-download to end cleanly, also surfaced the quiesce() use-after-free (see that commit). core/docs/m7-baseline.md records actual measured numbers against the M1/M7 DoD lines, including where they don't clear yet (RSS ~70 MB vs a 60 MB target; throughput/CPU only measured on loopback, no 1 Gbit link available here) rather than rounding them away. docs/adr/0016 documents a rate::RateLimiter fairness gap found building the load subcommand: a single shared TokenBucket under heavy segment contention has no fairness ordering across its peek/commit race and can starve a waiter well past what its configured rate implies. Filed as a follow-up (it's a core/src/rate design question, not a tools/bench one) rather than fixed here, along with a related TSan-only load-test straggler that could not be root-caused in this environment. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Q3QrF7rCt21bkAjt9BCDFQ
This commit is contained in:
@@ -0,0 +1,79 @@
|
||||
# 16. Global rate limit fairness under heavy segment contention (known issue, not fixed)
|
||||
|
||||
Status: accepted (documents a known limitation; no code change to `rate::RateLimiter`)
|
||||
|
||||
## Context
|
||||
|
||||
While finishing `tools/bench`'s `load` subcommand (the M1 DoD's 20-task load test), pacing
|
||||
20 concurrent tasks (`default_segments=8` each, so up to 160 segments contending for
|
||||
`max_active_segments=32` slots) via `RateLimiter::set_global_limit()` reproduced 2 of 20
|
||||
tasks hanging past a 120 s per-task wait instead of completing in the ~5 s the configured
|
||||
rate implied. Unthrottled, all 20 tasks complete in well under a second — the stall is
|
||||
specific to many segment workers contending for one global `TokenBucket` through
|
||||
`RateLimiter::acquire()`'s peek-then-commit-all-or-nothing path, not a general deadlock.
|
||||
|
||||
`TokenBucket::consume`/`peek` compute a wait duration assuming the caller retries once that
|
||||
much time has passed and the bucket will then have `n` tokens. Under heavy contention that
|
||||
assumption breaks: many segment workers independently schedule a retry via
|
||||
`TaskHost::schedule()` for whenever they were told tokens *would* be available, but
|
||||
whichever of them acquires `RateLimiter::mu_` first on waking drains the tokens the others
|
||||
were counting on, forcing the losers to recompute and reschedule a fresh wait. There is no
|
||||
fairness ordering (FIFO queue, ticket, or similar) across that race — repeated bad luck for
|
||||
the same task's segments is possible and was observed twice in one run. This gets worse,
|
||||
not better, as contention rises: more competing waiters means more of them lose each round.
|
||||
|
||||
## Decision
|
||||
|
||||
Left unfixed for M1. `tools/bench/vdm_bench.cpp`'s `load` subcommand works around it by
|
||||
giving each task its own independent per-task bucket (`RateLimiter::set_task_limit`)
|
||||
instead of one shared global bucket — each task's `acquire()` then only ever contends with
|
||||
its own ≤8 segments, which the same 20-task run completes in ~6.6 s with zero timeouts. That
|
||||
workaround is sufficient for the bench (it still needs, and gets, real concurrent buffering
|
||||
to measure RSS against) and is documented inline where the choice is made.
|
||||
|
||||
It is **not** sufficient for a real user: `download.setGlobalLimit` (or however DAEMON
|
||||
surfaces it) is a real, everyday feature, and a household running 20+ concurrent downloads
|
||||
against a single global cap is a plausible, not exotic, scenario. This ADR exists so that
|
||||
scenario doesn't get rediscovered from scratch.
|
||||
|
||||
## Consequences
|
||||
|
||||
- `RateLimiter`'s global/queue level should get a fairness mechanism before M1 sign-off
|
||||
treats "global bandwidth cap with many concurrent downloads" as supported: e.g. serve
|
||||
waiters in the order their wait was computed (a min-heap keyed on wake time, or a simple
|
||||
ticket counter checked before committing), or move to a scheme where a waiting caller's
|
||||
reserved allocation can't be stolen by a later arrival.
|
||||
- Needs a regression test once fixed: N tasks (N large enough to exceed
|
||||
`max_active_segments`), one shared global limit, assert every task completes within a
|
||||
bounded multiple of the ideal `total_bytes / global_bps` time — the shape of the bug
|
||||
`tools/bench load` stumbled into, made deterministic.
|
||||
- Filed here rather than fixed in this change because it's a `core/src/rate/` /
|
||||
`core/src/task/` design question (retry/backoff and scheduling policy under contention),
|
||||
not a `tools/bench` one, and deserves its own review rather than a bundled-in fix.
|
||||
|
||||
## Postscript: a second, TSan-only straggler (still unexplained, not proven the same bug)
|
||||
|
||||
After the `--preset tsan` build was made to work (a real, separate ASan-caught bug fixed in
|
||||
the same change: `DownloadTaskState::quiesce()` was clearing `workers` synchronously right
|
||||
after issuing an async `cancel()`, racing the HttpClient worker thread's still-in-flight
|
||||
write callback — see the commit that adds this ADR), `tools/bench load` was run under
|
||||
`--preset tsan` to complete the M1 DoD's sanitizer-clean load test. Even with the
|
||||
per-task-bucket workaround above *and* external (testserver-side) pacing instead of the
|
||||
engine's rate limiter entirely, a single straggler task failed to complete within a
|
||||
generous (300–500s) per-task budget under TSan specifically — reproduced at
|
||||
tasks=20/segments=8 (2 stragglers), tasks=20/segments=2 (1 straggler); tasks=8/segments=2
|
||||
was reliable across repeated runs and is what `tools/bench`'s ctest registration now uses.
|
||||
|
||||
No TSan diagnostic (data race, lock-order inversion, etc.) ever accompanied a straggler —
|
||||
across every run that hit one. That means either: (a) it really is just TSan's per-access
|
||||
instrumentation overhead compounding with this sandbox's own scheduling/virtualization
|
||||
under high simultaneous curl/thread activity, with no engine defect at all, or (b) it's a
|
||||
genuine timing-sensitive bug (a plausible candidate: `HttpClient`'s
|
||||
`CURLOPT_LOW_SPEED_LIMIT`/`CURLOPT_LOW_SPEED_TIME` stall detection — 1024 B/s for 30s by
|
||||
default — false-tripping when TSan's overhead makes real throughput look stalled to curl's
|
||||
own timers, driving a segment into a retry/backoff loop that never catches up) that TSan's
|
||||
slowdown merely makes likelier to manifest, not one it creates. This was not root-caused:
|
||||
doing so needs reproducing outside this sandbox, on hardware not already shared/loaded, to
|
||||
separate "TSan is just slow here" from "there is a real bug TSan is making easier to hit".
|
||||
|
||||
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
||||
Reference in New Issue
Block a user