feat: optimize checked_to_unchecked to take into account multiplications#8188
feat: optimize checked_to_unchecked to take into account multiplications#8188
checked_to_unchecked to take into account multiplications#8188Conversation
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 63a8dfb | Previous: 9845edc | Ratio |
|---|---|---|---|
test_report_noir-lang_noir-bignum_ |
505 s |
405 s |
1.25 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
ACVM Benchmarks
Details
| Benchmark suite | Current: b6ab48a | Previous: 7c6ab0c | Ratio |
|---|---|---|---|
purely_sequential_opcodes |
267385 ns/iter (± 382) |
263545 ns/iter (± 1359) |
1.01 |
perfectly_parallel_opcodes |
237552 ns/iter (± 2021) |
236787 ns/iter (± 4213) |
1.00 |
perfectly_parallel_batch_inversion_opcodes |
3583960 ns/iter (± 12547) |
3581886 ns/iter (± 7418) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Time
Details
| Benchmark suite | Current: b6ab48a | Previous: 7c6ab0c | Ratio |
|---|---|---|---|
private-kernel-inner |
0.028 s |
0.029 s |
0.97 |
private-kernel-reset |
0.16 s |
0.16 s |
1 |
private-kernel-tail |
0.017 s |
0.017 s |
1 |
rollup-base-private |
0.333 s |
0.335 s |
0.99 |
rollup-base-public |
0.213 s |
0.211 s |
1.01 |
rollup-block-root |
11 s |
11.2 s |
0.98 |
rollup-merge |
0.004 s |
0.004 s |
1 |
rollup-root |
0.013 s |
0.014 s |
0.93 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Compilation Time
Details
| Benchmark suite | Current: b6ab48a | Previous: 7c6ab0c | Ratio |
|---|---|---|---|
regression_4709 |
0.663 s |
0.665 s |
1.00 |
ram_blowup_regression |
12.6 s |
12.8 s |
0.98 |
global_var_regression_entry_points |
0.464 s |
0.458 s |
1.01 |
private-kernel-inner |
2.218 s |
2.258 s |
0.98 |
private-kernel-reset |
6.518 s |
6.422 s |
1.01 |
private-kernel-tail |
1.092 s |
1.02 s |
1.07 |
rollup-base-private |
18.2 s |
18.6 s |
0.98 |
rollup-base-public |
13.7 s |
13.24 s |
1.03 |
rollup-block-root-empty |
0.903 s |
0.879 s |
1.03 |
rollup-block-root-single-tx |
124 s |
118 s |
1.05 |
rollup-block-root |
124 s |
131 s |
0.95 |
rollup-merge |
0.864 s |
0.859 s |
1.01 |
rollup-root |
1.36 s |
1.358 s |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Test Suite Duration
Details
| Benchmark suite | Current: b6ab48a | Previous: 7c6ab0c | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr |
50 s |
49 s |
1.02 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts |
88 s |
90 s |
0.98 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
40 s |
39 s |
1.03 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib |
171 s |
175 s |
0.98 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib |
153 s |
160 s |
0.96 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
51 s |
51 s |
1 |
test_report_noir-lang_noir-bignum_ |
407 s |
414 s |
0.98 |
test_report_noir-lang_noir_bigcurve_ |
235 s |
275 s |
0.85 |
test_report_noir-lang_sha512_ |
28 s |
28 s |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Execution Memory
Details
| Benchmark suite | Current: b6ab48a | Previous: 7c6ab0c | Ratio |
|---|---|---|---|
private-kernel-inner |
203.28 MB |
203.28 MB |
1 |
private-kernel-reset |
227.03 MB |
227.03 MB |
1 |
private-kernel-tail |
181.3 MB |
181.3 MB |
1 |
rollup-base-private |
433.5 MB |
433.5 MB |
1 |
rollup-base-public |
425.68 MB |
425.68 MB |
1 |
rollup-block-root |
1420 MB |
1420 MB |
1 |
rollup-merge |
254.29 MB |
254.29 MB |
1 |
rollup-root |
260.03 MB |
260.03 MB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Compilation Memory
Details
| Benchmark suite | Current: b6ab48a | Previous: 7c6ab0c | Ratio |
|---|---|---|---|
private-kernel-inner |
270.75 MB |
270.74 MB |
1.00 |
private-kernel-reset |
535.37 MB |
535.37 MB |
1 |
private-kernel-tail |
200.14 MB |
200.12 MB |
1.00 |
rollup-base-private |
1330 MB |
1330 MB |
1 |
rollup-base-public |
1350 MB |
1350 MB |
1 |
rollup-block-root-empty |
270.76 MB |
270.77 MB |
1.00 |
rollup-block-root-single-tx |
7770 MB |
7770 MB |
1 |
rollup-block-root |
7780 MB |
7780 MB |
1 |
rollup-merge |
271.94 MB |
271.94 MB |
1 |
rollup-root |
322.45 MB |
322.44 MB |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
|
I think the performance alert might have been an outlier as |
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE feat: let `nargo test` use the new greybox fuzzer (noir-lang/noir#8234) chore(docs): add fixed bugs to list (noir-lang/noir#8201) fix(parser): do not use `Ident::default()` (noir-lang/noir#8224) feat(fuzz): Add experimental comptime code generator (noir-lang/noir#8207) fix: let SSA parser parse calls to print (noir-lang/noir#8232) feat: added 1, -1 case for inversions in brillig (noir-lang/noir#8225) chore: clippy (noir-lang/noir#8220) fix: don't take implicitly added named generics when checking where clauses (noir-lang/noir#8184) chore(docs): Defunctionalization and some minor cleanup (noir-lang/noir#8217) feat: add `--no-fuzz` and `--only-fuzz` to `nargo test` (noir-lang/noir#8213) chore: add regression test for panic encountered in bigcurve library. (noir-lang/noir#8211) fix: validate numeric types when passing them to a FunctionBuilder (noir-lang/noir#8215) chore: refactor constrainedness checks on trait impls (noir-lang/noir#8170) chore(test): Add smoke test for AST generation (noir-lang/noir#8048) chore: bump more dependencies (noir-lang/noir#8208) chore: Don't use `i1` in the AST fuzzer (noir-lang/noir#8204) fix: Do not panic if RHS constant in division has more bits than the operand (noir-lang/noir#8197) chore: bump cargo deps (noir-lang/noir#8205) fix: Handle truncating constants to 128 bits (noir-lang/noir#8180) fix!: disallow the `i1` type (noir-lang/noir#8172) chore(fuzz): Make sure `main` makes at least one call (noir-lang/noir#8202) fix: Use `get_local_or_global_instruction` in `try_optimize_array_set_from_previous_get` (noir-lang/noir#8200) fix: returns 0 for right shift overflow (noir-lang/noir#8189) feat: avoid overflow check when subtracting from a value that is the maximum for its bitsize (noir-lang/noir#8190) feat: optimize `checked_to_unchecked` to take into account multiplications (noir-lang/noir#8188) chore: document ssa `inline_simple_functions` (noir-lang/noir#8169) chore(docs): remove_unreachable SSA pass (noir-lang/noir#8196) chore: Change AST fuzzer recursion limit (noir-lang/noir#8173) chore(acir): Unify how we display witness indices (noir-lang/noir#8192) fix: Detect ABI change of fuzzing harnesses (noir-lang/noir#8193) chore(acir): Test that `BLACKBOX::RANGE` display the number of bits (noir-lang/noir#8191) fix(debugger): send idle at loop end + fix test (noir-lang/noir#8187) fix: Use `get_local_or_global_instruction` when simplifying `IfElse` (noir-lang/noir#8185) fix(parser): avoid using `Location::dummy()` (noir-lang/noir#8178) chore: Push a bug list (noir-lang/noir#8186) feat(greybox_fuzzer): Should_fail and should_fail_with (noir-lang/noir#8118) chore: move unsigned overflow check from acir/brillig to ssa (noir-lang/noir#8163) feat: location tree for debug_info (noir-lang/noir#7034) fix: Use `IntegerConstant` for loop boundaries in `unrolling` (noir-lang/noir#8094) fix(ssa): Recursive shared Brillig entry points (noir-lang/noir#8099) chore: enable '--pedantic-solving' on more tests (noir-lang/noir#7701) chore(readme): Update `acvm-repo` READMEs (noir-lang/noir#8150) chore(test): add `test_programs` dir for expected-panic tests (noir-lang/noir#8147) chore(docs): minor updates in solidity doc (noir-lang/noir#8160) feat(debugger): debug test functions (noir-lang/noir#7958) chore: migrate recursive proof test to ultrahonk (noir-lang/noir#8038) chore: bump glob package (noir-lang/noir#8159) chore: bump external pinned commits (noir-lang/noir#8139) chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs (noir-lang/noir#8158) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Ary Borenszweig <asterite@gmail.com> Co-authored-by: TomAFrench <tom@tomfren.ch>
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE feat: let `nargo test` use the new greybox fuzzer (noir-lang/noir#8234) chore(docs): add fixed bugs to list (noir-lang/noir#8201) fix(parser): do not use `Ident::default()` (noir-lang/noir#8224) feat(fuzz): Add experimental comptime code generator (noir-lang/noir#8207) fix: let SSA parser parse calls to print (noir-lang/noir#8232) feat: added 1, -1 case for inversions in brillig (noir-lang/noir#8225) chore: clippy (noir-lang/noir#8220) fix: don't take implicitly added named generics when checking where clauses (noir-lang/noir#8184) chore(docs): Defunctionalization and some minor cleanup (noir-lang/noir#8217) feat: add `--no-fuzz` and `--only-fuzz` to `nargo test` (noir-lang/noir#8213) chore: add regression test for panic encountered in bigcurve library. (noir-lang/noir#8211) fix: validate numeric types when passing them to a FunctionBuilder (noir-lang/noir#8215) chore: refactor constrainedness checks on trait impls (noir-lang/noir#8170) chore(test): Add smoke test for AST generation (noir-lang/noir#8048) chore: bump more dependencies (noir-lang/noir#8208) chore: Don't use `i1` in the AST fuzzer (noir-lang/noir#8204) fix: Do not panic if RHS constant in division has more bits than the operand (noir-lang/noir#8197) chore: bump cargo deps (noir-lang/noir#8205) fix: Handle truncating constants to 128 bits (noir-lang/noir#8180) fix!: disallow the `i1` type (noir-lang/noir#8172) chore(fuzz): Make sure `main` makes at least one call (noir-lang/noir#8202) fix: Use `get_local_or_global_instruction` in `try_optimize_array_set_from_previous_get` (noir-lang/noir#8200) fix: returns 0 for right shift overflow (noir-lang/noir#8189) feat: avoid overflow check when subtracting from a value that is the maximum for its bitsize (noir-lang/noir#8190) feat: optimize `checked_to_unchecked` to take into account multiplications (noir-lang/noir#8188) chore: document ssa `inline_simple_functions` (noir-lang/noir#8169) chore(docs): remove_unreachable SSA pass (noir-lang/noir#8196) chore: Change AST fuzzer recursion limit (noir-lang/noir#8173) chore(acir): Unify how we display witness indices (noir-lang/noir#8192) fix: Detect ABI change of fuzzing harnesses (noir-lang/noir#8193) chore(acir): Test that `BLACKBOX::RANGE` display the number of bits (noir-lang/noir#8191) fix(debugger): send idle at loop end + fix test (noir-lang/noir#8187) fix: Use `get_local_or_global_instruction` when simplifying `IfElse` (noir-lang/noir#8185) fix(parser): avoid using `Location::dummy()` (noir-lang/noir#8178) chore: Push a bug list (noir-lang/noir#8186) feat(greybox_fuzzer): Should_fail and should_fail_with (noir-lang/noir#8118) chore: move unsigned overflow check from acir/brillig to ssa (noir-lang/noir#8163) feat: location tree for debug_info (noir-lang/noir#7034) fix: Use `IntegerConstant` for loop boundaries in `unrolling` (noir-lang/noir#8094) fix(ssa): Recursive shared Brillig entry points (noir-lang/noir#8099) chore: enable '--pedantic-solving' on more tests (noir-lang/noir#7701) chore(readme): Update `acvm-repo` READMEs (noir-lang/noir#8150) chore(test): add `test_programs` dir for expected-panic tests (noir-lang/noir#8147) chore(docs): minor updates in solidity doc (noir-lang/noir#8160) feat(debugger): debug test functions (noir-lang/noir#7958) chore: migrate recursive proof test to ultrahonk (noir-lang/noir#8038) chore: bump glob package (noir-lang/noir#8159) chore: bump external pinned commits (noir-lang/noir#8139) chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs (noir-lang/noir#8158) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Ary Borenszweig <asterite@gmail.com> Co-authored-by: TomAFrench <tom@tomfren.ch>
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE feat: let `nargo test` use the new greybox fuzzer (noir-lang/noir#8234) chore(docs): add fixed bugs to list (noir-lang/noir#8201) fix(parser): do not use `Ident::default()` (noir-lang/noir#8224) feat(fuzz): Add experimental comptime code generator (noir-lang/noir#8207) fix: let SSA parser parse calls to print (noir-lang/noir#8232) feat: added 1, -1 case for inversions in brillig (noir-lang/noir#8225) chore: clippy (noir-lang/noir#8220) fix: don't take implicitly added named generics when checking where clauses (noir-lang/noir#8184) chore(docs): Defunctionalization and some minor cleanup (noir-lang/noir#8217) feat: add `--no-fuzz` and `--only-fuzz` to `nargo test` (noir-lang/noir#8213) chore: add regression test for panic encountered in bigcurve library. (noir-lang/noir#8211) fix: validate numeric types when passing them to a FunctionBuilder (noir-lang/noir#8215) chore: refactor constrainedness checks on trait impls (noir-lang/noir#8170) chore(test): Add smoke test for AST generation (noir-lang/noir#8048) chore: bump more dependencies (noir-lang/noir#8208) chore: Don't use `i1` in the AST fuzzer (noir-lang/noir#8204) fix: Do not panic if RHS constant in division has more bits than the operand (noir-lang/noir#8197) chore: bump cargo deps (noir-lang/noir#8205) fix: Handle truncating constants to 128 bits (noir-lang/noir#8180) fix!: disallow the `i1` type (noir-lang/noir#8172) chore(fuzz): Make sure `main` makes at least one call (noir-lang/noir#8202) fix: Use `get_local_or_global_instruction` in `try_optimize_array_set_from_previous_get` (noir-lang/noir#8200) fix: returns 0 for right shift overflow (noir-lang/noir#8189) feat: avoid overflow check when subtracting from a value that is the maximum for its bitsize (noir-lang/noir#8190) feat: optimize `checked_to_unchecked` to take into account multiplications (noir-lang/noir#8188) chore: document ssa `inline_simple_functions` (noir-lang/noir#8169) chore(docs): remove_unreachable SSA pass (noir-lang/noir#8196) chore: Change AST fuzzer recursion limit (noir-lang/noir#8173) chore(acir): Unify how we display witness indices (noir-lang/noir#8192) fix: Detect ABI change of fuzzing harnesses (noir-lang/noir#8193) chore(acir): Test that `BLACKBOX::RANGE` display the number of bits (noir-lang/noir#8191) fix(debugger): send idle at loop end + fix test (noir-lang/noir#8187) fix: Use `get_local_or_global_instruction` when simplifying `IfElse` (noir-lang/noir#8185) fix(parser): avoid using `Location::dummy()` (noir-lang/noir#8178) chore: Push a bug list (noir-lang/noir#8186) feat(greybox_fuzzer): Should_fail and should_fail_with (noir-lang/noir#8118) chore: move unsigned overflow check from acir/brillig to ssa (noir-lang/noir#8163) feat: location tree for debug_info (noir-lang/noir#7034) fix: Use `IntegerConstant` for loop boundaries in `unrolling` (noir-lang/noir#8094) fix(ssa): Recursive shared Brillig entry points (noir-lang/noir#8099) chore: enable '--pedantic-solving' on more tests (noir-lang/noir#7701) chore(readme): Update `acvm-repo` READMEs (noir-lang/noir#8150) chore(test): add `test_programs` dir for expected-panic tests (noir-lang/noir#8147) chore(docs): minor updates in solidity doc (noir-lang/noir#8160) feat(debugger): debug test functions (noir-lang/noir#7958) chore: migrate recursive proof test to ultrahonk (noir-lang/noir#8038) chore: bump glob package (noir-lang/noir#8159) chore: bump external pinned commits (noir-lang/noir#8139) chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs (noir-lang/noir#8158) END_COMMIT_OVERRIDE --------- Co-authored-by: AztecBot <tech@aztecprotocol.com> Co-authored-by: Ary Borenszweig <asterite@gmail.com> Co-authored-by: TomAFrench <tom@tomfren.ch>
Description
Problem
Another try at #8111 but this time memoizing values max bit sizes to see if it doesn't make performance worse.
Summary
If we have code like this:
then we don't check for overflow in that multiplication because we know
mwas casted fromu1which can only be 0 or 1.If we have code like this:
then we shouldn't need to check for overflow either as we know
x * ycan be 1 or 0. However, there was no such check in our code so that's what this PR does.Additional Context
I tried this in the past in #8111 but it turned out to be slow. The likely reason is that it was recursing without memoizing so hitting the same logic over and over. With memoization this seems to be solved and it maybe opens the door to more optimizations.
Documentation
Check one:
PR Checklist
cargo fmton default settings.