-
Notifications
You must be signed in to change notification settings - Fork 199
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: Sha256 refactoring and benchmark with longer input (#6318)
# Description ## Problem\* Preparation for #6304 ## Summary\* Preparation for changing the message block type in `sha256.nr`: * Added some type aliases and extra comments, rearranged some functions * Added a new benchmark program with longer input so that we exercise the iteration and the last partial block as well * Running the criterion benchmarks with and without the `--force-brillig` option, to cover what the AVM would do * Added an option to the `stdlib-tests.rs` to pass a filter for test names This is purely to make it a bit easier to see what is going on and to establish some baseline before trying to make changes. Tried to rationalise the code a bit: * Removed `pad_msg_block`: based on the constraints put on its results it looked like it's forbidden from doing anything. This allows the removal of some constraints because for example `msg_block` and `last_block` are equal by definition. Here's just that [diff](https://github.com/noir-lang/noir/compare/2052451..6304-sha-msg-block-size). * Moved the verification of padding with zeroes after the input into the `verify_block_msg_padding` function. This is only called for the last (partially filled) block. According to the Circuit Size report below 👇 there is a 33% reduction in the number of ACIR opcodes in some of the SHA256 benchmarks. ### Testing ```shell cargo test -p nargo_cli --test stdlib-tests -- run_stdlib_tests sha256 cargo test -p nargo_cli --test stdlib-props fuzz_sha256 ``` ### Benchmarking ```shell cargo bench -p nargo_cli --bench criterion sha256_long ``` The baseline benchmarks on my machine as of c600000 were as follows: ```console ❯ cargo bench -p nargo_cli --bench criterion sha256_long ... bench_sha256_long_execute time: [1.3613 ms 1.3688 ms 1.3782 ms] bench_sha256_long_execute_brillig time: [286.64 µs 287.67 µs 288.96 µs] ``` For some reason after merging `master` into the PR the performance is worse in 636c9e9 ```console ❯ cargo bench -p nargo_cli --bench criterion sha256_long ... bench_sha256_long_execute time: [1.7297 ms 1.7918 ms 1.8675 ms] change: [+27.365% +29.911% +32.673%] (p = 0.00 < 0.05) Performance has regressed. Found 2 outliers among 20 measurements (10.00%) 2 (10.00%) high severe bench_sha256_long_execute_brillig time: [354.12 µs 360.31 µs 368.45 µs] change: [+22.390% +24.264% +27.161%] (p = 0.00 < 0.05) Performance has regressed. Found 1 outliers among 20 measurements (5.00%) 1 (5.00%) high severe ``` > __Maxim__: We did just sync aztec packages which now conditionally inlines functions (we previously used to always inline functions). This may be the cause of some execution time increases. Here's the [diff](https://github.com/noir-lang/noir/compare/c600000..636c9e9) between those commits. ## Additional Context In a follow-up PR I'll try to change the type of `msg_block` from `[u8; 64]` to `[u32; 16]` to avoid having to call `msg_u8_to_u32`. This should at least have the benefit of copying the array fewer times: at the moment an array copy is made every time an item in it is written to; with 16 items instead of 64 we get up to 4x less copies. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
- Loading branch information
Showing
7 changed files
with
441 additions
and
183 deletions.
There are no files selected for viewing
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
[package] | ||
name = "bench_sha256_long" | ||
version = "0.1.0" | ||
type = "bin" | ||
authors = [""] | ||
|
||
[dependencies] |
Oops, something went wrong.