Conversation
3f9ddbb to
c342f0d
Compare
|
unfortunately, it's confirmed that external PRs can't activate the 'comment' feature because the github token provided to external PRs does not have the necessary permissions. |
alxiong
left a comment
There was a problem hiding this comment.
Great work! 👏 very solid starting point for us.
Left some suggestions (open to chat), I will likely push one commit improving on justfile (open to revert if you dislike)
b3cd23c to
c2a3636
Compare
TalDerei
left a comment
There was a problem hiding this comment.
first pass observations
| #[cfg(any(test, feature = "gg-callgrind"))] | ||
| #[doc(hidden)] | ||
| pub mod test_fixtures; |
There was a problem hiding this comment.
I think we should call it
unstable-test-fixtures(sorry for the verbosity!) because I want us to get in the habit of labeling features that might have breaking API changes.
i guess i don't understand here - what's 'unstable' about this? no production API surface should actually change. i just didn't want to default expose things that weren't for consumers (nobody wants to use MySimpleCircuit or see it in their autocomplete)
There was a problem hiding this comment.
changed feature name to unstable-test-fixtures
There was a problem hiding this comment.
#375 (comment) wasn't addressed; deferring to @ebfull on benchmarking granularity in this first pass.
| - name: Missing baseline. Run baseline benchmarks | ||
| id: baseline-bench | ||
| if: github.event_name == 'pull_request' && !steps.check-baseline.outputs.exists | ||
| continue-on-error: true |
There was a problem hiding this comment.
continue-on-error: true can slip into silent benchmark failures?
There was a problem hiding this comment.
this is necessary because it's possible that there will be no benchmark results or broken benchmarks in the base. continue-on-error is present so it may proceed with measuring the present branch and compare it to nothing.
There was a problem hiding this comment.
removed fallback baseline generation
| - name: Cache apt packages | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: /var/cache/apt |
There was a problem hiding this comment.
I’d be in favor of starting with a simpler script that just works, with minimal surface area, and then iteratively layering in this kind of caching complexity.
There was a problem hiding this comment.
removed apt cache behavior
1f62d07 to
11ec59d
Compare
78546eb to
58a4ff3
Compare
ebfull
left a comment
There was a problem hiding this comment.
Main concern is that ragu_pasta should not be a dependency of any of the crates in this workspace. (dev-dependency only)
| components: clippy | ||
| - name: Run clippy | ||
| run: cargo clippy --workspace --lib --tests --benches --locked -- -D warnings | ||
| run: cargo clippy --workspace --lib --tests --benches --locked --features unstable-test-fixtures -- -D warnings |
There was a problem hiding this comment.
I wonder if we want to --all-features this?
There was a problem hiding this comment.
switched to --all-features
| gadgets::{GadgetKind, Kind}, | ||
| maybe::Maybe, | ||
| }; | ||
| use ragu_pasta::Fp; |
There was a problem hiding this comment.
We can avoid ragu_circuits depending on ragu_pasta (strict requirement!) by making the fixtures below be generic over the field type.
There was a problem hiding this comment.
now parameterized.
dependency moved to dev dependency.
created issue suggesting cargo-deny to enforce this strict requirement #402
| pub times: usize, | ||
| } | ||
|
|
||
| impl Circuit<Fp> for SquareCircuit { |
There was a problem hiding this comment.
| impl Circuit<Fp> for SquareCircuit { | |
| impl<F: Field> Circuit<F> for SquareCircuit { |
There was a problem hiding this comment.
now parameterized
| ff = { workspace = true } | ||
| group = { workspace = true } | ||
| ragu_core = { path = "../ragu_core", version = "0.0.0" } | ||
| ragu_pasta = { path = "../ragu_pasta", version = "0.0.0", features = ["baked"] } |
There was a problem hiding this comment.
This breaks a dependency requirement of mine; we definitely don't want ragu_circuits to depend on ragu_pasta because then Ragu is not agnostic to the curve cycle.
There was a problem hiding this comment.
dependency moved to dev dependency.
created issue suggesting cargo-deny to enforce this strict requirement #402
| // ============================================================================ | ||
| // Test fixtures for registration_errors tests | ||
| // ============================================================================ |
There was a problem hiding this comment.
non-blocking nit: I have a general principle, which is that whenever I see "sectioned" comments in code like this I tend to think it means the file is too large and should be broken up into separate files/modules.
There was a problem hiding this comment.
the original main version already had these (local) fixtures and the tests collected in a single file, the section header is really just diff noise.
some of this is now reverted - tests are back in their original location as integration tests rather than unit tests (this also helped address the dependency issue). the set of fixtures is now minimized and lives in a dedicated module.
|
|
||
| [[package]] | ||
| name = "serde_json" | ||
| version = "1.0.148" |
There was a problem hiding this comment.
any reason we can't use serde_json 1.0.149?
There was a problem hiding this comment.
now serde_json 1.0.149
| [[package]] | ||
| name = "zerocopy" | ||
| version = "0.8.25" | ||
| version = "0.8.31" |
There was a problem hiding this comment.
any reason we can't use zerocopy 0.8.33?
There was a problem hiding this comment.
now zerocopy 0.8.33
|
|
||
| [[package]] | ||
| name = "zmij" | ||
| version = "1.0.9" |
There was a problem hiding this comment.
any reason we can't use zmij 1.0.14?
There was a problem hiding this comment.
now zmij 1.0.14
94c802e to
3211cad
Compare
ebfull
left a comment
There was a problem hiding this comment.
Excellent, glad we're going to start having these in the codebase.
Summary
Add gungraun (iai-callgrind) benchmarks for deterministic, instruction-count-based performance tracking. This enables reliable CI regression detection by measuring CPU instructions instead of wall-clock time.
Closes #47. Supersedes #375.
Why gungraun/iai-callgrind?
Changes
CI Workflow (
.github/workflows/bench.yml)Note: External PRs cannot receive benchmark comments due to GitHub token permissions.
Local Development
just bench— auto-detects platform (native Linux, Docker on macOS)Feature Flag
Uses
unstable-test-fixturesfeature to expose test utilities needed by benchmarks. This follows the project convention for features with potentially unstable API surface.Metrics
Transforms gungraun JSON output via
.github/scripts/transform-gungraun.jq. Reports 6 key metrics:Benchmark Coverage
Review Feedback Addressed
--initflagStepRngdeprecatedStdRng::seed_from_u64()unstable-test-fixturesFuture Work
Test Plan
just benchworks on Linux (native) and macOS (via Docker)just bench -- --save-summary=jsongenerates summary filesBenchmark comment appears on internal PRsneeds to merge first