Repository navigation
feat(storage): the benchmark rig, and the first LMDB recording - #1141
chappie-daemon wants to merge 14 commits into
Conversation
…s and repeat the phases
🦋 Changeset detectedLatest commit: d808694 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
 [High impact] The PR appears safe to merge; the remaining output-path check is a non-blocking issue. Findings
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let parent = match arguments.out.parent() { | ||
| Some(parent) if !parent.as_os_str().is_empty() => parent.to_path_buf(), | ||
| _ => PathBuf::from("."), | ||
| }; | ||
| if !parent.is_dir() { | ||
| return Err(refusal(format!( | ||
| "--out {}: {} is not a directory. Create it and run again", | ||
| arguments.out.display(), | ||
| parent.display() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Plan::resolve checks only whether the output's parent exists. For example, --out /tmp passes, runs the entire workload, and then fails because the target is a directory. An unwritable output also fails only at the end. Check that the destination can be written before starting, without truncating an existing artifact.
Coverage Report for CI Build 43719446693Coverage remained the same at 77.582%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
All four are already fixed at this head — verified at The review was taken against
The first three were the findings from the previous review and are answered in full there; the fourth is the same defect one file over, caught by the same pass and fixed at the head that pass did not see. The rig's suite at |
The last slice of #864 (closed): the rig that produces
docs/rewrite/benchmarks.md, which thecutover gate (#869) reads before it can retire the TypeScript implementation. Filed as #1131
because 864 closed with the store slice.
What is here
crates/nostream-storage/examples/bench.rs— one workload, held as named constants in thefile and echoed into the artifact, run through
Arc<dyn EventRepository>so the same call sitesmeasure either backend: a deterministic corpus, bulk load, a per-call ingest, five read shapes,
nearest-rank percentiles, and a JSON artifact whose deterministic part is identical across runs
of a seed while every host- and run-dependent value lives in one
volatileblock.--verify— the oracle. It asserts the corpus's exact expected totals per shape (200 / 20 000 /99 602 / 20 000 / 1 020 000 at 50 000 events), non-empty windows per iteration, percentile
ordering and repeat agreement. Timings are never asserted.
--repeats N— median/p10/p90 per shape and per rate, so a contended host is visible ratherthan hidden by a point estimate.
docs/rewrite/benchmarks.md+docs/rewrite/benchmarks/lmdb-50k-seed7.json— therecording, its conditions, its arithmetic and the decisions D1–D9 that define what it measures
and what it does not.
rust-checksjob: the benchmark unit tests (whichcargo test --workspacedoes not run, measured) and the Postgres arm where a database exists.CLAUDE.mdrequires, and the ADR 0012 amendment that retires "thebenchmark rig, which does not exist".
The recording, and what it is worth
One host, four cores,
Intel i7-10700T @ 2.00GHz, load 43.32 at the first of three repeats, releaseprofile, 50 000 events, seed 7,
--repeats 3, regenerated after the round-6 fixes (load 48.79).create(one write per call, the relay's own path)runs at a 256 ops/s median;
create_many(500 per call) at 31.2 ms p95 per batch. The bands are in the document and in the artifact.The band is the host. The performance lens measured one repeat's 1 000 ingests at 125.5 s
against 4.4 s and 6.3 s in the other two on this box; the document records the spreads and says a
wider band is a busier host, not a slower store.
What it does not measure, and why
CI reaches it;
benchmarks.mdcarries the column asnot measuredwith the command.src/on this branch is 3.11.0 divergedfrom
origin/main, and the job's member has no Docker, nopsql, nonode_modules.search: both arms honour it only withnip50.enabled, and the harness opens eitherbackend without settings, so a
searchrow would time a plain filtered read with the querydropped. D9 says so, with the code.
Review history
Four rounds, three reworks, on a job whose estimate was $2.40 and whose actual is stated below.
counttimed an empty range 49 times of 50 andauthor_kind40 of 200;--verifyasserted onlyrows > 0, so it passed both;per_secondmixed batches/s with events/s, inverting a 69× ratio; the example's unit tests were run by no gate;--outfailed after the whole measurement--storewith--repeats > 1never refused a populated store — the guard inspected<dir>/lmdb/data.mdbwhile the run opened<dir>/repeat-<n>/lmdb/data.mdb; without--verifya second run exited 0 withcreate_many rows 0--verifyfailed correct runs through a.max(1)clamp;--outrefused on the wrong path; profile-dependent seed arithmetic; repeat read-backs unchecked; a workload block that could not tell two by-id workloads apartThe approver then escalated on five citation anchors in the job record —
recon.md×4 andspec.md×1 — none of them in the branch:EnvFlags::empty()isbackend.rs:110, not:145;LmdbBackendis declared inlmdb/mod.rs, notbackend.rs; "The harness map" isnip-parity.md:963; themaxLimitrefusal issubscribe.rs:596-604. Corrected against the tree and re-approved. The check is now filed as a script so it stops being an approver's habit: cameri/skills#302.The pattern worth carrying: every round's defect was a claim about coverage rather than a
wrong answer — an oracle weaker than the defect it was written to catch, a guard validating a path
the run did not use, an exclusion list hiding an unmeasurable shape. None of them was visible by
reading the happy path.
Rework after the automated review (round 6)
The PR's automated reviewer found five things after the PR opened, one of them a P1. All five were verified against the code and all five were true:
--verifycomparedtiming.per_second.max(1)against the truth, so any shape whose integer rate rounds to 0 failed. It now assertsops > 0as a boolean and treats a zero rounded rate as a measurement. The oracle has now been wrong in both directions: too weak in round 1 (rows > 0passed an empty range) and too strict here.--outrefused on its parent rather than its destination, so--out /tmppassed and failed after the whole run. Refused at the destination, with a writability probe that never truncates an existing artifact.wrapping_*, withu64::MAXcovered in both profiles.by_id_strideandby_id_offsetare inworkload.reads(schema 2), and the committed artifact was regenerated by the document's own command rather than edited.The regenerated run was taken at load 48.79 (the previous at 43.32): reads moved 10–21% slower,
create_many26% faster, every shape count unchanged. The document was rewritten to those figures and D4's "widest band" claim went with the artifact it described.Static stage
Four rounds, the clippy provider
ranclean each time over the repository's own CI commands —including
cargo check -p nostream-storage --no-default-features --all-targetsand the strict-D clippy::unwrap_used -D clippy::expect_used -D clippy::panic— the flags an earlier job's shimnever ran.
Greptile Reviewisunavailablefor this job: the shared key is rejected, and thebranch exceeds the service's diff cap in any case.
CodeRabbitreviewsrewrite/mainPRs once#1136 lands.
Known limits
measurement;
--allow-large-corpuslifts the first, the document prints the projection.benchmarks: D9 lists 28 935 as an exception to a predicate it satisfies #1138 (28 935 listed as an exception to a predicate it satisfies), benchmarks: D9 calls the strategy arms "the same call with a different row"; two do more store work #1139 ("the same call with a
different row" holds for
create, not for delete/vanish's extra store work), benchmarks: the opening "Nothing else is cited" is contradicted by four later citations #1140 ("Nothingelse is cited" is contradicted by four later citations, each correctly labelled).