Skip to content
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

Poseidon benchmark that also runs in Wasm #2633

Merged
merged 13 commits into from
Oct 11, 2024
Merged

Poseidon benchmark that also runs in Wasm #2633

merged 13 commits into from
Oct 11, 2024

Conversation

mitschabaude
Copy link
Contributor

@mitschabaude mitschabaude commented Sep 27, 2024

This adds a simple benchmark to the poseidon crate that can run in both the native and the wasm target.

For running criterion in Wasm, I followed this approach: https://github.com/bheisler/criterion.rs/blob/version-0.4/book/src/user_guide/wasi.md

This PR prepares for experiments with making the Wasm version faster. It makes sense to scope these experiments to Poseidon at first, because

  • This will be possible with low complexity and limited understanding of the code base
  • An alternative, faster version of Poseidon in Wasm would be independently useful to o1js even if we don't end up making any other changes to the proof system

Results

On this benchmark, Wasm is 4.8x slower than native:

> cargo bench --bench=poseidon_bench

Poseidon/poseidon_hash_kimchi                                                                             
                        time:   [34.578 µs 34.691 µs 34.816 µs]
                        change: [-0.1993% +0.4298% +1.0889%] (p = 0.23 > 0.05)
                        No change in performance detected.

> ./bench-wasm.sh --bench=poseidon_bench

Poseidon/poseidon_hash_kimchi
                        time:   [167.97 µs 168.33 µs 168.70 µs]
                        change: [+379.25% +383.20% +387.09%] (p = 0.00 < 0.05)
                        Performance has regressed.

(note: criterion actually uses the same benchmark data for the native and wasm runs, and compares between them. this doesn't seem too bad if you're mostly interested in improving one of them, and let's you compare the two easily if necessary)

@mitschabaude mitschabaude changed the base branch from compatible to develop September 27, 2024 12:58
@@ -18,6 +18,7 @@ idea
*.sage.py
params
_build
build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

my IDE automatically ran cmake (I think?) and added this folder, so let's gitignore it

bench-wasm.sh Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better place for this script than the repo root?

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest you create a scripts/ folder, and call this script from the Makefile, what do you think? Then it'd be similar to mina.

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.

Generally great, but please take a look at my comments! 👍

@@ -33,7 +33,15 @@ ocaml-gen = { workspace = true, optional = true }
[dev-dependencies]
serde_json.workspace = true
hex.workspace = true
criterion = { version = "0.3", default-features = false, features = [
Copy link
Member

Choose a reason for hiding this comment

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

Would it work if we put this criterion into the top level Cargo.toml? Or is this version/feature selection fundamentally incompatible with criterion used in different packages within proof-systems?

bench-wasm.sh Outdated
}

# Call the cleanup function
cleanup_old_wasm_files
Copy link
Member

Choose a reason for hiding this comment

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

Can you add cd $(git rev-parse --show-toplevel) or something like that? Maybe another argument that would specify from which directory to execute, so we have to pass .? This script is assumed to be called /exactly/ from a directory we need it to run into, and I think it's not a very obvious UX and can potentially erase some files you didn't expect it to touch.

```sh
rustup target add wasm32-wasi
cargo install cargo-wasi
npm install -g @wasmer/cli
Copy link
Member

Choose a reason for hiding this comment

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

Ooh, fun, we now have the npm dependency for proof-systems.

@volhovm volhovm self-assigned this Oct 3, 2024
@volhovm volhovm merged commit befc0ae into develop Oct 11, 2024
6 checks passed
@volhovm volhovm deleted the perf/poseidon-wasm branch October 11, 2024 16:26
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.

2 participants