Skip to content

Saffron/benches: add bench for read proof #3204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 5, 2025

Conversation

dannywillems
Copy link
Member

@dannywillems dannywillems commented May 5, 2025

This will be used to compare with the upcoming optimization of read proofs.

@dannywillems dannywillems force-pushed the dw/add-bench-saffron-proof branch from 81ebe48 to 66b321d Compare May 5, 2025 08:06
@dannywillems dannywillems force-pushed the dw/add-bench-saffron-proof branch from 66b321d to 40c1709 Compare May 5, 2025 08:12
Copy link
Member

@volhovm volhovm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool stuff 👍

};

// Set up static resources to avoid re-computation during benchmarks
static SRS: Lazy<SRS<Vesta>> = Lazy::new(|| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using pub fn get_srs_test<G>() -> SRS<G> from kimchi/src/precomputed_srs.rs would be a more canonical way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok, but only because we have the same size. I was first thinking about making the change, but wondering now if it is the best as Saffron and Kimchi are too different protocols. We might want to differentiate them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #3208 but I am not totally convinced we should do it. Feel free to accept if you have a strong opinion.

}

fn bench_read_proof_verify(c: &mut Criterion) {
let (data, query, answer, data_comm) = generate_test_data(SRS_SIZE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A word of caution: the default kimchi prover/verifier bench is quite inaccurate, so you might need to set up bench parameters manually if you want to achieve meaningfully low noise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why you are mentioning the kimchi prover/verifier here. Can you explain?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect it will largely generalise, only mentioned it as a particular example. I guess on average it's hard to measure verification precisely (because of multi-threading? I'm not sure why actually).

@@ -0,0 +1,117 @@
//! Run this bench using `cargo criterion -p saffron --bench read_proof_bench`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ cargo criterion -p saffron --bench read_proof_bench
error: no such command: `criterion`

        View all installed commands with `cargo --list`
        Find a package to install `criterion` with `cargo search cargo-criterion`

Is there something I am missing to run this command ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo install cargo-criterion

@dannywillems dannywillems merged commit 3375833 into master May 5, 2025
46 checks passed
@dannywillems dannywillems deleted the dw/add-bench-saffron-proof branch May 5, 2025 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants