perf(ingest): narrow ingest_lock to the manifest commit, calibrated by a new load-test harness#51
Open
explise wants to merge 3 commits into
Open
perf(ingest): narrow ingest_lock to the manifest commit, calibrated by a new load-test harness#51explise wants to merge 3 commits into
explise wants to merge 3 commits into
Conversation
The HTTP ingest path wrapped the whole of `Ingestor::ingest` in the process-wide `ingest_lock`, so every request waited out the *encode* of the one ahead of it — and encode is the expensive half (~13.3 ms/MiB of Arrow + ZSTD + bloom-filter work, versus ~9 ms for a commit). Measured on a 16-core host, that pinned ingest at ~43 MiB/s while the node used 0.84 of its 8 cores: throughput was flat from concurrency 1 to 32 while p50 latency scaled linearly (107 -> 3023 ms), the signature of a one-at-a-time server. Split `Ingestor::ingest` into `write_batches` (route, batch, encode, PUT) and `commit_files` (publish to the manifest), and lock only the latter. Encoding outside the lock is safe because data files are content-addressed, so concurrent writers cannot collide on a path; the manifest commit is a read-modify-write of one object and still needs mutual exclusion. In-flight bodies stay bounded by the `ingest_sem` permit the caller already holds. `ingest` keeps its previous behaviour by calling both halves, so no existing caller changes. `concurrent_ingest.rs` pins both properties — concurrent writes are lossless, and the content-addressing that makes the narrow lock sound — so a refactor that widens or drops the lock fails there rather than in production. Also exposes `max_batch_rows` / `max_batch_bytes` on `[ingest]`. Without a server-side knob the *client's* request size silently decides the PUT rate, since `ingest` flushes leftover buffers at the end of every call — a batch-size sweep would only be measuring the load generator. Defaults reproduce `BatchPolicy::default()` exactly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ADR-001 is emphatic that DST and calibration are never substitutes for each other: a green sim run proves orchestration, scheduling and the timing *model*, and proves nothing about absolute throughput. This is the other half — a Dockerized MinIO + `vdg` stack, a load generator, and sweep scripts that produce a throughput curve and name the bottleneck rather than checking a fixed target was hit. Covers ROADMAP M1.3's third acceptance criterion and M5.3's "reproducible benchmark harness". `bench/loadgen` is deliberately excluded from the workspace. A load generator must read the real clock to measure anything, and `crates/vdg/tests/seams.rs` fails the build on wall-clock calls anywhere under `crates/` — excluding it keeps that DST gate strict instead of growing an exemption for benchmark code, and keeps reqwest's TLS stack out of the workspace lockfile and CI. Its separate target dir needs its own .gitignore rule, since the existing one is anchored to the root. Results from the 2026-07-19 run are committed alongside as the evidence trail for the two changes either side of this commit; `RESULTS.md` reads them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`modeled_mibps_per_core = 250.0` was a placeholder, and ADR-001 requires this number be calibrated rather than assumed — `ModeledExecutor` divides `ScanPlan::total_bytes()` by it, so every simulated query duration inherits the error. DataFusion over MinIO at 8 cores sustained 892 MiB/s on a 4-column scan and 635 MiB/s on a full-projection aggregate: 79-111 MiB/s/core. 100 is the round number in that band. Only a metadata-only COUNT(*), which reads no column data at all, ever reached 250. Still optimistic in one direction — measured against local MinIO, so it carries no real-S3 first-byte latency. Documented as the CPU-side decode ceiling so the next calibration knows what it is looking at. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ingest saturated at ~43 MiB/s (~205k records/s) while the node used 0.84 of the 8 cores it was given. The limit was serialization, not hardware. This PR builds the harness that found that, fixes the bottleneck, and corrects a simulator constant the harness proved wrong.
Three commits, each independently reviewable:
f5685adingest_lockto the manifest commit onlyc77841f9bda6f9modeled_mibps_per_core250 → 100The bottleneck
ingest_lockwas held across the whole ofIngestor::ingest— encode, compress, PUT, and manifest commit — so every request waited out the encode of the one ahead of it. Encode is the expensive half (~13.3 ms/MiB versus ~9 ms for a commit).The signature was unmistakable: throughput flat across a 32× concurrency range while p50 latency scaled linearly with it.
The fix
Ingestor::ingestsplits intowrite_batches(route → batch → encode → PUT) andcommit_files(publish to manifest). Only the commit takes the lock.Encoding outside the lock is safe because data files are content-addressed — two writers encoding the same records produce the same path and the same bytes, and writers encoding different records cannot collide. The manifest commit is a read-modify-write of a single object and genuinely does need mutual exclusion.
ingestcalls both halves, so its behaviour is unchanged and no existing caller moves. Concurrency stays bounded by theingest_sempermit the handler already holds, so in-flight bodies still can't pile up in memory.Pre-existing failure mode, not introduced here: files land in the store before the manifest references them, so a commit that never happens leaves orphaned objects. That was already true of
ingest; the split doesn't change it. Called out in the rustdoc.Calibration
modeled_mibps_per_core = 250.0was a placeholder, andModeledExecutordividesScanPlan::total_bytes()by it — every simulated query duration inherited the error. Measured: 892 MiB/s on a 4-column scan, 635 MiB/s on a full-projection aggregate at 8 cores, i.e. 79–111 MiB/s/core. Only a metadata-onlyCOUNT(*), which reads no column data, ever reached 250.Still optimistic in one direction — measured against local MinIO, so it carries no real-S3 first-byte latency. It's the CPU-side decode ceiling, documented as such.
Notes for review
bench/loadgenis excluded from the workspace on purpose. A load generator must read the real clock, andcrates/vdg/tests/seams.rsfails the build on wall-clock calls undercrates/. Excluding it keeps that DST gate strict rather than adding an exemption for benchmark code, and keeps reqwest's TLS stack out of the workspace lockfile and CI.[ingest]knobs (max_batch_rows/max_batch_bytes). Without them the client's request size silently decides the PUT rate, sinceingestflushes leftover buffers every call — a batch-size sweep would only have been measuring the load generator. Defaults reproduceBatchPolicy::default()exactly, with a test asserting it.bench/results/is committed as the evidence trail (~152K of JSON). Happy to drop it or trim to just the artifactsRESULTS.mdcites if you'd rather keep it out of the tree.Testing
scripts/verify.sh --all rust— green.crates/ingest/tests/concurrent_ingest.rspins the two properties the narrow lock depends on: concurrent writes are lossless, and content-addressing prevents path collisions. A refactor that widens or drops the remaining lock fails there rather than in production.docs/load-test.mdhas the reproduction steps.🤖 Generated with Claude Code