fix(bench): reject implausible wall times; correct polyglot bench.rs setup - #9277
Conversation
honest_bench can record a wall time that is physically impossible and nothing downstream notices. The artifact at 38ff7ec holds 150 negative wall_ms samples out of 300, and REPORT.md is generated from them, so every timing cell in that file currently reads 0.0 ms. The runs themselves are fine (exit_code 0, checksums match); only the timing is meaningless. report.py filters on exit_code == 0 alone, so those samples reach statistics.median() and render as confident results. Add two guards: - run_bench.sh aborts, printing the offending start_ns/end_ns pair, rather than writing a non-positive sample. - report.py refuses to build a report from an artifact containing non-positive successful samples, and says how many. Verified against the current committed results.json (exits 1) and against the pre-PerryTS#7641 artifact at 7beb3a5 (regenerates normally). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each of these is already described in benchmarks/polyglot/RESULTS.md. bench_array_write: suite/03_array_write.ts fills every slot before calling Date.now(), so its timed loop overwrites resident pages. The Rust version used vec![0.0; 10_000_000] -- calloc, lazily mapped -- and timed the loop that first-touches them, paying ~10M page faults the TS loop had already paid. Pre-touch before starting the timer. 19 ms -> 5 ms locally. fib: was i32. RESULTS.md states that Perry's inference refines the TS number parameter to i64, so i64 is the like-for-like peer. 240 ms -> 214 ms. bench_object_create: without a barrier LLVM proves Point never escapes and deletes the loop, which is why the row reports 0 ms. black_box makes it measure the allocation it claims to. 0 ms -> 1 ms. These move published numbers, so the polyglot sweep needs a re-run on the project's reference hardware. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The convolution and JSON values in the performance table match the artifact at 7beb3a5 exactly, so they were correct when written -- but PerryTS#7641 regenerated that file with an invalid clock, and the copy at HEAD no longer contains them. Pin the citation to the revision that does, and note it should be re-pinned to main after the next good regeneration. Also add the 500k-record JSON pipeline row, using the figures already in REPORT.md's own prose (Perry 1,649 / Rust 604 / Node 1,010 / Bun 647). The table showed only the 100-record fixture, and the paragraph directly beneath it says the project publishes the workloads where the JITs win. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe benchmark harness and report reject non-positive timing samples. Rust workloads add compiler-optimization barriers and pre-touch allocated memory. The README adds a JSON pipeline result and updates benchmark source references. ChangesBenchmark measurement integrity
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR improves timing validation and adjusts benchmark setup, but the object-allocation benchmark may no longer represent the documented intentionally eliminable workload, which could affect cross-language result comparability. This is a localized, non-blocking concern that maintainers should explicitly confirm. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and covers the purpose, implementation changes, benchmark validation, and testing results. It does not include the template's explicit Related issue or Checklist sections, but the required change and test information is substantially complete. Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Merged. Validated on a shared branch with #9228, #9257, #9263, #9271, #9272, #9274, #9277, #9279 and #9280 — one build, one validation pass, then split back out and merged individually. Results across the batch:
One probe ( |
2 participants
Summary
honest_benchcan record wall times that are physically impossible, and nothingdownstream notices. The artifact committed at
38ff7ecccontains 150 negativewall_mssamples out of 300, andREPORT.mdis generated from them.This PR adds two guards so that failure is loud instead of silent, re-runs the
suite on Linux to show the harness working end to end, and fixes three setup
issues in
benchmarks/polyglot/bench.rsthatRESULTS.mdalready identifies.1. The harness publishes timing-free samples as results
From
benchmarks/honest_bench/results/results.jsonat38ff7ecc:{"workload": "image_convolution", "language": "rust", "wall_ms": -0.108458, "exit_code": 0, "output_match": true} {"workload": "json_pipeline_full", "language": "node", "wall_ms": 0.026375, "exit_code": 0, "output_match": true}All 300 measured samples are sub-millisecond noise around zero; 150 are negative.
The second row claims Node processed the 108 MB JSON fixture in 26 microseconds.
The runs themselves were fine —
exit_codeis 0 and the FNV checksums match.Only the timing is meaningless.
report.pyfilters onexit_code == 0alone(
scripts/report.py:111), so these samples flow intostatistics.median()andrender as confident cells:
The generated ratio lines then contradict the file's own prose — the
json_pipeline_fulltable reportsperry = 1.00×(fastest) while the Bottomline section above it says Perry is slowest at ~2.7×.
When it started
rust/image_convolutionmedian8a7ea9986b7beb3a50ca6fb58cdead88e0812a7d(#7641)#7641 changed no harness code — it regenerated data on a different host (metadata
switches to
Apple M1 / 8 cores / 8 GB). So this looks host-specific:run_bench.shsamplestime.monotonic_ns()in two separatepython3processesand subtracts them, and Python documents that clock's reference point as
undefined. On Linux the pair is boot-relative and valid (I measured 522 ms for a
known 500 ms child); on the host behind #7641 it evidently is not.
I could not reproduce the underlying clock behaviour — I don't have that machine —
so this PR does not change the timing mechanism. It only makes the harness refuse
to emit or report a sample it cannot stand behind.
The fix
harness/run_bench.sh— abort with the offendingstart_ns/end_nspairrather than writing a non-positive sample.
scripts/report.py— refuse to build a report from an artifact containingnon-positive successful samples, and say how many.
Verified both directions:
results.json→ exits 1 with10 of 20 successful runs have a non-positive wall_ms (min -0.108458)7beb3a50ca) → regenerates normally, restoringrust 392.1 / zig 245.4 / perry 354.0 / node 1206.6 / bun 915.42. A clean run on Linux x86_64 with the fixed harness
To confirm the harness works once the gate is in place, I ran the full suite on
Linux. Setup reproduces your reference exactly: the generated fixture is
byte-identical (112,695,869 bytes), and all three runtimes match the committed
Bun oracle —
checksum=2ba2e053for convolution,hash=7fc66fa8andsha256
a1d5a65c…for the JSON pipeline.Host: i9-12900HK, Linux 6.17, otherwise idle. Perry 0.5.1220 (latest release),
rustc 1.95.0, Node 22.23.1. No Zig or Bun available, so those columns are absent.
5 warmup + 20 measured, medians. 180/180 runs output-verified; 0 implausible
samples — the new gate never fired.
Peak RSS on the same runs:
Three things worth recording from this run:
beat Rust here on x86_64 too (0.90×), the same direction and margin as your
arm64 figure. Your
image_conv/rust/src/main.rsis an ordinarybounds-checked implementation with no handicap. But the result is specific to
the implementation shape, not to the languages — see below.
1.15× Rust; here it is 1.97×, and peak RSS is 38.7 MB against the 3.5 MB in the
README's memory row. Worth a look before the next regeneration.
~2.7× your REPORT prose records on arm64.
Why the convolution row favours Perry
The kernel clamps both coordinates inside the innermost loop. That makes every
index data-dependent, which blocks a chain of LLVM optimisations: it cannot hoist
the bounds check, use fixed offsets, or unroll the 5×5 cleanly. Measured on the
blur alone:
Neither change alone helps much; they unlock together. (Not vectorisation — the
LLVM IR contains no vector ops in either variant.)
Applying that same interior/border split to both implementations, through
this harness, all output-verified against
2ba2e053:The identical source change makes Rust 2.08× faster and Perry ~14× slower. So at
equal naive implementation quality Perry is ahead by 1.12×; at equal optimised
quality Rust is ahead by 28×. Both statements are true, and the README currently
reports only the first. A sentence noting that the row reflects the naive kernel
shape would make it much harder to argue with.
I hit a separate silent-miscompilation bug while building the Perry side of that
comparison — an 8-line repro where a store to a module-global
Bufferis droppedentirely. It is unrelated to benchmarking, so I am filing it on its own rather
than burying it here. Verified on 0.5.1220 (latest release); not yet checked
against
main.These are one host and one Perry release; they are offered as a second data point,
not as a replacement for your Apple Silicon numbers.
3.
benchmarks/polyglot/bench.rsThree changes, each already described in
benchmarks/polyglot/RESULTS.md:bench_array_write—suite/03_array_write.tsfills every slot before callingDate.now(), so its timed loop overwrites resident pages. The Rust version usedvec and timed the loop thatfirst-touches them, additionally paying ~10M page faults the TS loop had already
paid. Pre-touching before the timer mirrors the TS setup. Measured here:
19 ms → 5 ms, which ties Perry 0.5.1220 on the same machine.RESULTS.mdalready notes "the Rust result is
-Owith bounds-checked indexing;.iter_mut()would match Perry."fib— wasi32.RESULTS.mdstates "Perry's type inference refines the TSnumberparameter to i64", soi64is the like-for-like peer:240 ms → 214 ms.(
RESULTS.mdcurrently describes the Rust fib as f64-typed, which hasn't matchedthis file for some time — worth a separate look.)
bench_object_create— without a barrier LLVM provesPointnever escapes anddeletes the loop, which is why the row reports 0 ms.
RESULTS.mdsays exactlythat: "the compiler proves the struct never escapes and eliminates the whole
loop." A
black_boxmakes the row measure the allocation it claims to:0 ms → 1 ms.These move published numbers, so the polyglot sweep needs a re-run on your
reference hardware. Happy to drop this section if you'd rather take the harness
fix alone — it is a separate commit.
4. README
Two changes, both using your own published numbers rather than mine, so
nothing is substituted across architectures:
REPORT.mdcitation to7beb3a50ca. The README's convolution andJSON values match that revision's artifact exactly, so they were correct when
written; the file at HEAD no longer contains them. Re-pin to
mainafter thenext good regeneration.
already in your
REPORT.mdprose (Perry 1,649 / Rust 604 / Node 1,010 /Bun 647). The table currently shows only the 100-record fixture. Given the
paragraph directly beneath it — "We publish everything, including the workloads
where V8's JIT still beats us" — this row seemed to belong there.
Testing
report.pyagainst both the broken and the last-good artifact (above).honest_benchrun on Linux, 180 rows, all output-verified (§2).rustc -O -C codegen-units=1 bench.rsbuilds clean; output format unchanged, sorun_all.sh'sname:elapsed_msparsing is unaffected.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
Tests