-
Notifications
You must be signed in to change notification settings - Fork 96
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
Conversation
@@ -18,6 +18,7 @@ idea | |||
*.sage.py | |||
params | |||
_build | |||
build |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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 = [ |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
poseidon/README.md
Outdated
```sh | ||
rustup target add wasm32-wasi | ||
cargo install cargo-wasi | ||
npm install -g @wasmer/cli |
There was a problem hiding this comment.
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
.
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
Results
On this benchmark, Wasm is 4.8x slower than native:
(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)