Skip to content

Bench harness: documented behaviour does not match the runner #100

Description

@Xof

This issue groups 3 related findings.

BENCH-5 — Grid cells and three whole rows are skipped on a "Chisel cache hard ceiling" rationale that the enabled spillway removed

Location: bench/benches/micro_grid.rs:50-54, bench/benches/micro_grid.rs:173-175, bench/benches/micro_grid.rs:300-302, bench/benches/micro_grid.rs:412-416 · Severity: DESIGN · Category: docs-vs-reality

What the code does. micro_grid.rs says "Cells where ops_per_tx * size_bytes exceeds this are skipped to avoid Chisel's CacheFull at large 1000-per-tx writes (cache hard ceiling is ~16 MB…)" and skips with "continue; // skip cells too large to fit in Chisel's cache", and micro_grid() disables two rows entirely: "update-1000pertx and delete-1000pertx skipped: … exceeding Chisel's 2048-page cache ceiling. The cells are not measurable under default cache settings". The same crate states the opposite: runner.rs:330-338 "Bench engines now run with the spillway enabled at the production-default scale (1024 × cache budget) … so there is no strict cache-page ceiling on transaction size", and chisel_engine.rs:38-40 does set .spillway_max_bytes(cache_max_bytes * 1024). runner.rs's own test populate_snapshot_chisel_large_size_chunks allocates 24 MiB through a 2 MB cache and passes.

Why it is a problem. Coverage is silently lost on an invalidated premise: allocate-1000pertx and update-1000pertx drop the 16KB/128KB/1MB columns, and update-1000pertx / delete-1000pertx are never registered at all. The grid that is supposed to characterise Chisel under large transactions measures nothing there, and the comments will lead the next maintainer to re-derive the same wrong ceiling.

Direction of a fix. Re-run the excluded cells against the spillway-enabled engine; if they now complete, delete TX_BUDGET_BYTES and the skips and re-register the two rows. If a real limit remains, restate the comments in terms of the spillway budget instead of the removed 2048-page cache cap.

BENCH-6 — Module docs claim a 270-cell, 9-row micro grid; the code registers 6 rows and emits 165 cells

Location: bench/benches/micro_grid.rs:1-2, bench/benches/micro_grid.rs:407-417, bench/src/runner.rs:405-407 · Severity: DESIGN · Category: docs-vs-reality

What the code does. micro_grid.rs opens with "Bench binary: the 270-cell micro grid. Iterates EngineMode::ALL × SIZES × the 9 row groups", and runner.rs documents capture_aux_metrics_snapshot_restore as covering "the snapshot-restore-style rows (8 of 9 rows: rows 1, 2, 4–9)". micro_grid() actually calls six row builders (allocate-1pertx, allocate-1000pertx, read-warm, read-cold, update-1pertx, delete-1pertx). The committed bench/results/aux_metrics.jsonl has 165 lines, and discover.rs:258 independently says "165 cells × 2 small JSON files".

Why it is a problem. A reader sizing runtime, reviewing coverage, or checking that a run completed will compare against 270 cells / 9 rows and conclude 105 cells silently failed. seed_for still carries live arms for "update-1000pertx" and "delete-1000pertx" that nothing calls, reinforcing the illusion that those rows run.

Direction of a fix. Update both headers to the six rows / 165 cells actually registered (or re-enable the missing rows per BENCH-5 and make the number true), and drop the dead seed_for arms if the rows stay disabled.

BENCH-7 — cross-engine.md's methodology footer claims "a single fsync per commit" for every engine; Chisel performs three

Location: bench/src/summary/render_cross_engine.rs:145-147 · Severity: DESIGN · Category: docs-vs-reality · Status: NEW

What the code does. The footer rendered into the published comparison document states: "Each engine takes / a single fsync per commit through the disk write cache". Chisel's commit protocol is three fsyncs — src/lib.rs:503 "Commit the active transaction. Performs three fsyncs before", src/transaction/lifecycle.rs:121 "This is the FIRST of the three fsyncs", and the bench's own runner.rs:878-879 comment says "Commits executed (3 protocol fsyncs each)".

Why it is a problem. This renderer's module doc says the output is "suitable for the README and 1.0 release notes", so the wrong claim propagates into public performance material. It understates Chisel's per-commit durability cost by 3× and invites the reader to conclude the three engines' commit paths are equivalent when Chisel is doing strictly more syscall work for the same throughput number.

Direction of a fix. State the per-engine commit protocol honestly in the footer (Chisel: three fsyncs; redb Durability::Immediate; SQLite WAL + synchronous=FULL + fullfsync), since that asymmetry is precisely what the throughput table is being read against.


Filed from the clean-slate deep review of 2026-07-29. Full context, verification notes, and the delta against ISSUES.md are in docs/reviews/review-20260729-183138.md. Baseline at review time: 681 tests passing, clippy and fmt clean — none of these are toolchain-visible.

Metadata

Metadata

Assignees

No one assigned

    Labels

    review-2026-07-29Found by the clean-slate deep review of 2026-07-29severity:designWrong shape: bad abstraction, unenforced invariant, doc contradicts codetype:docsDocs contradict code; stale or wrong comments

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions