fix: handle large arrays in the monomorphiser and limit their length #11279
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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: dc8c1b1 | Previous: e1b2904 | Ratio |
|---|---|---|---|
test_report_noir-lang_noir_bigcurve_ |
307 s |
236 s |
1.30 |
test_report_zkpassport_noir-ecdsa_ |
3 s |
2 s |
1.50 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 3df8836 | Previous: e48a0d8 | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
sha512-100-bytes |
0.085 s |
0.053 s |
1.60 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs
Outdated
Show resolved
Hide resolved
| source: tooling/nargo_cli/tests/execute.rs | ||
| expression: stderr | ||
| --- | ||
| error: The return value has 4294967294 witnesses which exceeds the limit of 16777216 |
There was a problem hiding this comment.
So we can declare the type, but cannot return the type? This feels like it may confuse users as they may have a type declared with no issue and then once they use the type they start getting an error. I think it would be better to error on the type itself. Wdyt @jfecher?
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: d0018cb | Previous: edcff87 | Ratio |
|---|---|---|---|
private-kernel-reset |
453.66 MB |
306.98 MB |
1.48 |
rollup-checkpoint-root-single-block |
11290 MB |
1750 MB |
6.45 |
rollup-checkpoint-root |
11300 MB |
1750 MB |
6.46 |
rollup-tx-base-private |
994.05 MB |
525.96 MB |
1.89 |
rollup-tx-base-public |
3030 MB |
467.15 MB |
6.49 |
sha512_100_bytes |
186.09 MB |
72.48 MB |
2.57 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 61cb787 | Previous: 0d03696 | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
rollup-checkpoint-merge |
0.002 s |
0.001 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| insta::assert_snapshot!(program, @r" | ||
| unconstrained fn main$f0() -> () { | ||
| let a$l0 = [1, 2]; | ||
| let b$l1 = [a$l0.clone().clone(); 3]; |
There was a problem hiding this comment.
I'm surprised you opted for this, rather than handling it in the SSA. It seems strange to introduce a new AST to capture repeated expressions, and delay their turning into multiple elements until the SSA so that the AST doesn't have N occurrences of Ident nodes, and then to insert N Clone nodes anyway.
I was wondering how this will potentially affect #11124 where I unwrap 1 layer of Clone, but didn't expect it to be nested. It looks like it wouldn't interact with that as this isn't a call argument.
There was a problem hiding this comment.
It's easy to handle it in SSA, but I wanted to keep this inside the ownership pass. However I see your point, and may be it is cleaner to do it in SSA, where the repeated is handled. I'll do it.
Description
Problem
Resolves #11173 and #10524
It also address part of #10522, but we should still add error handling in brillig generation.
Summary
A safe limit has been added in acir_gen to error on very large arrays.
The monomorphizer avoids creating repeated elements in an array.
Additional Context
User Documentation
Check one:
PR Checklist
cargo fmton default settings.