feat: simplify vector_push_back when length is known for complex types#11717
feat: simplify vector_push_back when length is known for complex types#11717
Conversation
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
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: 25b0587 | Previous: 9f2785b | Ratio |
|---|---|---|---|
rollup-checkpoint-merge |
0.003 s |
0.002 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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: fc64662 | Previous: 00a4db9 | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
How does this handle predicated |
|
I think there's no need to look at the predicate because in SSA a vector_push_back returns a copy of the input vector with the added element. Also, this optimization was already happening for vectors with simple elements, now it's also happening for complex elements. (though I understand where the concern is coming from and I'm also trying to always remember the predicate when doing these optimizations) |
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: fc64662 | Previous: 00a4db9 | Ratio |
|---|---|---|---|
rollup-tx-merge |
0.002 s |
0.001 s |
2 |
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 Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: fc64662 | Previous: 9f2785b | Ratio |
|---|---|---|---|
rollup-tx-base-private |
2.192 s |
1.812 s |
1.21 |
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 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: fc64662 | Previous: 9f2785b | Ratio |
|---|---|---|---|
rollup-tx-base-private |
23.02 s |
17.8 s |
1.29 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
aakoshh
left a comment
There was a problem hiding this comment.
Looks good!
I am trying another experiment on top of this to see what happens if we try to remove the ValueMerger from the simplify_vector_push_back method, as it seems to me that set_last_vector is already the content that we want. Having some trouble with it, but if it works, I'll open that as a follow up.
|
Happy to merge and we can merge the followup into master after but will leave up to @aakoshh |


Description
Problem
Extracted from #11659 (more or less)
Summary
The compiler simplifies
vector_push_backmade with a known length where the value to push to is amake_array. However, this optimization was only done when the vector had single elements, not complex ones. The reason was that there was a check to disable an optimization that later comes on (simplify_vector_push_back), which currently doesn't work for the complex case. However, this check was also disabling the simpler optimization we could do that relies onmake_array.In reality we also needed to change the "known length matches the length of the
make_arrayelements" to consider the semi-flattened length for this case to work.Additional Context
User Documentation
Check one:
PR Checklist
cargo fmton default settings.