This issue groups 3 related findings.
BENCH-11 — Equivalence-test comment says "no spillway in bench engines"; every bench Chisel engine enables it
Location: bench/tests/equivalence.rs:145-149, bench/src/chisel_engine.rs:51-58 · Severity: SMELL · Category: comment-accuracy · Status: NEW
What the code does. make_chisel is commented "256 pages × 8 KiB = 2 MiB strict cache cap (no spillway in bench engines). Must be large enough for the largest single-tx allocation in any equivalence scenario: scenario_large_overflow allocates 1 MiB, which requires ~130 overflow pages … 256 pages gives comfortable headroom." The constructor it calls, ChiselEngine::open_in_memory, does Options::default().cache_max_bytes(cache_max_bytes).spillway_max_bytes(cache_max_bytes * 1024) and is itself documented "The spillway is enabled at the same production-default scale as open_file".
Why it is a problem. The comment's whole sizing argument ("must be large enough", "comfortable headroom") rests on a cap that no longer exists. A maintainer adding a larger equivalence scenario will either bloat the cache for no reason or, worse, treat a genuine CacheFull as expected because the comment told them the cap is strict.
Direction of a fix. Delete the "(no spillway in bench engines)" parenthetical and the headroom argument, or state the actual spillway budget the engine is opened with.
BENCH-12 — Two runner.rs doc comments contradict their own code: scenario prepop transaction granularity and which modes expose counters
Location: bench/src/runner.rs:509-510, bench/src/runner.rs:71-77 · Severity: SMELL · Category: comment-accuracy
What the code does. run_scenario_cell's doc says step 2 is "Run prepopulate_workload untimed (each Allocate in its own tx / for simplicity; pre-pop time is excluded from the measurement)", but the body batches by POPULATE_TX_MAX_RECORDS (500) and POPULATE_TX_BUDGET_BYTES (1 MiB) — and the inline comment at 532-542 explicitly says single-op-per-tx prepop was abandoned because it is "~12 min for 100K records". Separately, supports_internal_counters is documented "Currently only ChiselStrict (the other engines are black-box)" while the body is matches!(self, Self::ChiselStrict | Self::ChiselMemory).
Why it is a problem. The prepop claim matters for interpretation: a reader reasoning about how much freemap/handle-table COW pressure the pre-populated state carries will model 100K separate commits instead of ~200. The counters claim will send someone hunting for why chisel-mem cells carry counter data the doc says is impossible — and the unit test at runner.rs:647-656 can't catch it because EngineMode::ALL deliberately excludes ChiselMemory.
Direction of a fix. Rewrite the step-2 line to describe the chunked prepop, and update the supports_internal_counters doc to name both Chisel modes.
BENCH-13 — freemap-churn-flat documents its throughput unit as round-trips/sec but sets Criterion's element count to the record count
Location: bench/benches/freemap_churn.rs:117-118, bench/benches/freemap_churn.rs:138-139 · Severity: SMELL · Category: comment-accuracy
What the code does. The doc says "The throughput unit is "delete+realloc round-trips per second" (one round-trip = 2 commits)" and the inline comment repeats "// Throughput: one unit = one (delete + realloc) round-trip." The next line is group.throughput(criterion::Throughput::Elements(live_count as u64)), with live_count ∈ {500, 200, 100}, while the timed routine performs exactly one churn_cycle per iteration.
Why it is a problem. Criterion divides the per-iteration time by the element count, so the reported figure is records/sec, 100–500× larger than the round-trips/sec the comment tells the reader they are looking at. Anyone trend-tracking this number across the three cases will also compare values normalized by three different divisors as though they shared a unit.
Direction of a fix. Either pass Throughput::Elements(1) to match the documented round-trip unit, or restate the doc as "records reclaimed per second" and note that the divisor differs per case.
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.
This issue groups 3 related findings.
BENCH-11 — Equivalence-test comment says "no spillway in bench engines"; every bench Chisel engine enables it
Location:
bench/tests/equivalence.rs:145-149, bench/src/chisel_engine.rs:51-58· Severity: SMELL · Category: comment-accuracy · Status: NEWWhat the code does.
make_chiselis commented "256 pages × 8 KiB = 2 MiB strict cache cap (no spillway in bench engines). Must be large enough for the largest single-tx allocation in any equivalence scenario: scenario_large_overflow allocates 1 MiB, which requires ~130 overflow pages … 256 pages gives comfortable headroom." The constructor it calls,ChiselEngine::open_in_memory, doesOptions::default().cache_max_bytes(cache_max_bytes).spillway_max_bytes(cache_max_bytes * 1024)and is itself documented "The spillway is enabled at the same production-default scale asopen_file".Why it is a problem. The comment's whole sizing argument ("must be large enough", "comfortable headroom") rests on a cap that no longer exists. A maintainer adding a larger equivalence scenario will either bloat the cache for no reason or, worse, treat a genuine CacheFull as expected because the comment told them the cap is strict.
Direction of a fix. Delete the "(no spillway in bench engines)" parenthetical and the headroom argument, or state the actual spillway budget the engine is opened with.
BENCH-12 — Two runner.rs doc comments contradict their own code: scenario prepop transaction granularity and which modes expose counters
Location:
bench/src/runner.rs:509-510, bench/src/runner.rs:71-77· Severity: SMELL · Category: comment-accuracyWhat the code does.
run_scenario_cell's doc says step 2 is "Runprepopulate_workloaduntimed (each Allocate in its own tx / for simplicity; pre-pop time is excluded from the measurement)", but the body batches byPOPULATE_TX_MAX_RECORDS(500) andPOPULATE_TX_BUDGET_BYTES(1 MiB) — and the inline comment at 532-542 explicitly says single-op-per-tx prepop was abandoned because it is "~12 min for 100K records". Separately,supports_internal_countersis documented "Currently onlyChiselStrict(the other engines are black-box)" while the body ismatches!(self, Self::ChiselStrict | Self::ChiselMemory).Why it is a problem. The prepop claim matters for interpretation: a reader reasoning about how much freemap/handle-table COW pressure the pre-populated state carries will model 100K separate commits instead of ~200. The counters claim will send someone hunting for why chisel-mem cells carry counter data the doc says is impossible — and the unit test at runner.rs:647-656 can't catch it because
EngineMode::ALLdeliberately excludes ChiselMemory.Direction of a fix. Rewrite the step-2 line to describe the chunked prepop, and update the
supports_internal_countersdoc to name both Chisel modes.BENCH-13 — freemap-churn-flat documents its throughput unit as round-trips/sec but sets Criterion's element count to the record count
Location:
bench/benches/freemap_churn.rs:117-118, bench/benches/freemap_churn.rs:138-139· Severity: SMELL · Category: comment-accuracyWhat the code does. The doc says "The throughput unit is "delete+realloc round-trips per second" (one round-trip = 2 commits)" and the inline comment repeats "// Throughput: one unit = one (delete + realloc) round-trip." The next line is
group.throughput(criterion::Throughput::Elements(live_count as u64)), with live_count ∈ {500, 200, 100}, while the timed routine performs exactly onechurn_cycleper iteration.Why it is a problem. Criterion divides the per-iteration time by the element count, so the reported figure is records/sec, 100–500× larger than the round-trips/sec the comment tells the reader they are looking at. Anyone trend-tracking this number across the three cases will also compare values normalized by three different divisors as though they shared a unit.
Direction of a fix. Either pass
Throughput::Elements(1)to match the documented round-trip unit, or restate the doc as "records reclaimed per second" and note that the divisor differs per case.Filed from the clean-slate deep review of 2026-07-29. Full context, verification notes, and the delta against
ISSUES.mdare indocs/reviews/review-20260729-183138.md. Baseline at review time: 681 tests passing, clippy and fmt clean — none of these are toolchain-visible.