From 23fbd060cb7ed91c073a9d3fbd5779054a4326fa Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 29 Jul 2026 12:38:38 -0700 Subject: [PATCH] fix: discover Criterion samples at their real depth, under new/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `discover_cells` bounded its walk to exactly depth 4 with the comment "Each leaf is at depth 4 (criterion_dir/row/mode/size/sample.json)". Criterion 0.5 writes `///new/sample.json` — depth 5. The walk therefore matched nothing: on this repo's real Criterion tree, `find target/criterion -maxdepth 4 -name sample.json` returns zero hits and `-maxdepth 5` returns two. The failure was silent rather than loud. `NoCellsFound` never fired because aux_metrics.jsonl still supplied cells through the Step-3 leftover path, so every micro-grid cell reached the renderers with `timing: None` and every p50/p95/p99 in summary.md rendered as an em-dash. The only test covering this walked a fixture tree that omits the `new/` level, so the suite passed against a layout Criterion never produces. Walks depth 5 and requires the leaf's parent directory to be named `new`. The `new` check is the load-bearing half: Criterion retains the previous run under a sibling `base/`, which shares the (row, mode, size) key and would otherwise emit a duplicate Cell. The fixture tree gains the `new/` level and a `base/` sibling holding times 10x larger, so the tests now cover the real layout and the suite fails if `base/` is ever picked up (asserted both by an exactly-one-cell count and by the existing p50 == 3000 assertions). `copy_raw_archive` was always unbounded-depth and already archived the `new/` path correctly; the smoke test's expected archive path is updated to match. Verified end to end: against the real `target/criterion`, summarize now reports `smoke/chisel-strict/256B` with p50 84481021 / p95 89508941, where before it emitted the cell with no timing at all. Closes #95. --- bench/src/summary/discover.rs | 45 ++++++++++++++++--- bench/src/summary/mod.rs | 2 +- .../chisel-strict/32B/base/sample.json | 4 ++ .../32B/{ => new}/estimates.json | 0 .../chisel-strict/32B/{ => new}/sample.json | 0 .../redb-strict/32B/{ => new}/estimates.json | 0 .../redb-strict/32B/{ => new}/sample.json | 0 .../chisel-strict/32B/{ => new}/sample.json | 0 bench/tests/summarize_smoke.rs | 5 ++- 9 files changed, 48 insertions(+), 8 deletions(-) create mode 100644 bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/base/sample.json rename bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/{ => new}/estimates.json (100%) rename bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/{ => new}/sample.json (100%) rename bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/{ => new}/estimates.json (100%) rename bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/{ => new}/sample.json (100%) rename bench/tests/fixtures/criterion/corrupt/chisel-strict/32B/{ => new}/sample.json (100%) diff --git a/bench/src/summary/discover.rs b/bench/src/summary/discover.rs index de4adbf..6db5284 100644 --- a/bench/src/summary/discover.rs +++ b/bench/src/summary/discover.rs @@ -1,8 +1,12 @@ // Filesystem discovery layer for the summary post-processor. Walks -// target/criterion////sample.json files, parses +// target/criterion////new/sample.json files, parses // aux_metrics.jsonl, joins them by (row, mode, size) key into Cell // values. Cells with missing-on-one-side data carry None for the // missing field — renderers handle the partial-state cases explicitly. +// +// The `new/` component is Criterion's own, not ours: it writes the +// just-finished run to `new/` and retains the previous one under `base/`. +// Only `new/` is read here; see the walk in `discover_cells`. use crate::runner::ChiselCountersDelta; use crate::summary::format::percentile_linear_interp; @@ -146,23 +150,37 @@ pub fn discover_cells( } // Step 2: walk criterion_dir to find sample.json leaves. - // Each leaf is at depth 4 (criterion_dir/row/mode/size/sample.json). + // + // Each leaf is at depth 5: criterion_dir/row/mode/size/{new,base}/sample.json. + // Criterion 0.5 interposes that last directory — `new/` is the run that just + // finished, `base/` is the previous run kept for its own change-detection. + // We want `new/` only: `base/` is a stale duplicate under the same + // (row, mode, size) key, and counting it would emit two Cells per cell. + // + // The `new` check is the load-bearing filter; the depth bounds only + // restate the grid's shape (a row/mode/size triple is exactly what the + // path walk-back below needs, and what a Cell is keyed by). let mut cells: Vec = Vec::new(); let mut sample_paths_seen = 0usize; for entry in walkdir::WalkDir::new(criterion_dir) - .min_depth(4) - .max_depth(4) + .min_depth(5) + .max_depth(5) .into_iter() .filter_map(|e| e.ok()) { if entry.file_name() != "sample.json" { continue; } + + // Walk the path back: file -> new dir -> size dir -> mode dir -> row dir. + let run_dir = entry.path().parent().unwrap(); + if run_dir.file_name().unwrap_or_default() != "new" { + continue; + } sample_paths_seen += 1; - // Walk the path back: file -> size dir -> mode dir -> row dir. - let size_dir = entry.path().parent().unwrap(); + let size_dir = run_dir.parent().unwrap(); let mode_dir = size_dir.parent().unwrap(); let row_dir = mode_dir.parent().unwrap(); @@ -373,6 +391,21 @@ mod tests { cells.len() ); + // The chisel-strict 32B fixture has BOTH a new/ and a base/ sample.json, + // mirroring a real Criterion tree after a second run. Exactly one cell + // must come out of it, and its numbers must be new/'s (base/ carries + // times 10x larger, so a wrong pick shows up as p50 30000). + assert_eq!( + cells + .iter() + .filter(|c| c.row == "allocate-1pertx" + && c.mode == "chisel-strict" + && c.size == "32B") + .count(), + 1, + "base/ must not produce a second cell under the same key" + ); + let chisel_cell = cells .iter() .find(|c| c.row == "allocate-1pertx" && c.mode == "chisel-strict" && c.size == "32B") diff --git a/bench/src/summary/mod.rs b/bench/src/summary/mod.rs index 7908315..46cda48 100644 --- a/bench/src/summary/mod.rs +++ b/bench/src/summary/mod.rs @@ -1,5 +1,5 @@ // Markdown summary post-processor module. Reads PR 4b's bench output -// (target/criterion////sample.json + bench/results/aux_metrics.jsonl) +// (target/criterion////new/sample.json + bench/results/aux_metrics.jsonl) // and produces three artifacts under bench/results//: // // summary.md — human-readable per-row tables diff --git a/bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/base/sample.json b/bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/base/sample.json new file mode 100644 index 0000000..9c5c074 --- /dev/null +++ b/bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/base/sample.json @@ -0,0 +1,4 @@ +{ + "iters": [10.0, 10.0, 10.0, 10.0, 10.0], + "times": [100000.0, 200000.0, 300000.0, 400000.0, 500000.0] +} diff --git a/bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/estimates.json b/bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/new/estimates.json similarity index 100% rename from bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/estimates.json rename to bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/new/estimates.json diff --git a/bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/sample.json b/bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/new/sample.json similarity index 100% rename from bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/sample.json rename to bench/tests/fixtures/criterion/allocate-1pertx/chisel-strict/32B/new/sample.json diff --git a/bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/estimates.json b/bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/new/estimates.json similarity index 100% rename from bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/estimates.json rename to bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/new/estimates.json diff --git a/bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/sample.json b/bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/new/sample.json similarity index 100% rename from bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/sample.json rename to bench/tests/fixtures/criterion/allocate-1pertx/redb-strict/32B/new/sample.json diff --git a/bench/tests/fixtures/criterion/corrupt/chisel-strict/32B/sample.json b/bench/tests/fixtures/criterion/corrupt/chisel-strict/32B/new/sample.json similarity index 100% rename from bench/tests/fixtures/criterion/corrupt/chisel-strict/32B/sample.json rename to bench/tests/fixtures/criterion/corrupt/chisel-strict/32B/new/sample.json diff --git a/bench/tests/summarize_smoke.rs b/bench/tests/summarize_smoke.rs index 61ddd9b..0e958b3 100644 --- a/bench/tests/summarize_smoke.rs +++ b/bench/tests/summarize_smoke.rs @@ -80,10 +80,13 @@ fn summarize_smoke_runs_against_fixtures() { "summary.md missing ycsb-a row" ); + // `new/` is Criterion's own directory level, and `copy_raw_archive` + // mirrors the source tree verbatim — so the archived path carries it too. let chisel_raw = raw_dir .join("allocate-1pertx") .join("chisel-strict") - .join("32B"); + .join("32B") + .join("new"); assert!( chisel_raw.join("sample.json").exists(), "raw chisel-strict sample.json missing"