fix: record actual bench phase durations instead of TimerInfo start instants - #346
Open
0oyun wants to merge 1 commit into
Open
fix: record actual bench phase durations instead of TimerInfo start instants#3460oyun wants to merge 1 commit into
0oyun wants to merge 1 commit into
Conversation
…nstants `BaseTester::bench_builder` stored ark_std `TimerInfo` values in `BenchStats`. A `TimerInfo` only holds the `Instant` at which the timer *started*, so the downstream benches that later read `stats.proof_time.time.elapsed()` measured "phase start -> report time" rather than the phase's own duration. In practice `proof_time` ended up including the subsequent verification (and every earlier phase was inflated by all the work that ran after it), making the CSV timings wrong. Measure each phase with an explicit `Instant` and store `std::time::Duration` in `BenchStats`; update the bn254/secp256k1 bench writers to use the fields directly. `start_timer!`/`end_timer!` are kept purely for their log output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes benchmark timing collection in BaseTester::bench_builder by recording per-phase durations instead of storing ark_std::TimerInfo start instants (which caused inflated/overlapping timings when .elapsed() was evaluated later).
Changes:
- Switch
BenchStatstiming fields fromTimerInfotostd::time::Duration, measured viaInstant::now()+.elapsed()around each phase. - Keep
start_timer!/end_timer!for log output, but decouple recorded stats fromTimerInfo. - Update downstream bench CSV writers to use
stats.proof_time/stats.verify_timedirectly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| halo2-base/src/utils/testing.rs | Record true phase durations (vk/pk/prove/verify) into BenchStats as Duration values. |
| halo2-ecc/src/secp256k1/tests/ecdsa.rs | Write Duration fields directly to CSV instead of calling .time.elapsed() on stored start instants. |
| halo2-ecc/src/bn254/tests/pairing.rs | Same CSV writer update for proof/verify timing fields. |
| halo2-ecc/src/bn254/tests/msm.rs | Same CSV writer update for proof/verify timing fields. |
| halo2-ecc/src/bn254/tests/fixed_base_msm.rs | Same CSV writer update for proof/verify timing fields. |
| halo2-ecc/src/bn254/tests/ec_add.rs | Same CSV writer update for proof/verify timing fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stephenh-axiom-xyz
force-pushed
the
main
branch
from
August 10, 2026 14:25
eef4038 to
4d3bcea
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
BaseTester::bench_builder(inhalo2-base/src/utils/testing.rs) storesark_stdTimerInfovalues inBenchStats:A
TimerInfoonly holds theInstantat which the timer started. Thedownstream benches read the timing later like this:
Because
.elapsed()is called on the start instant at report time, eachvalue measures "phase start → report time", not the duration of the phase
itself. Concretely:
proof_time.time.elapsed()includes the whole verification step (and anywork between proving and the CSV write).
So the timings written to the
*.csvbench outputs are wrong (overlapping andinflated).
Affected bench writers:
bn254/tests/{msm, fixed_base_msm, ec_add, pairing}.rsand
secp256k1/tests/ecdsa.rs.Fix
Measure each phase with an explicit
Instantand storestd::time::Durationin
BenchStats:start_timer!/end_timer!are kept purely for their log output; the recordedvalue is now the actual operation time. Downstream writers use the fields
directly (
stats.proof_time/stats.verify_time), which are stillDuration, so the{:?}formatting is unchanged.Testing
cargo check -p halo2-baseandcargo check -p halo2-ecc --testsboth pass.