fix(brillig): Skip decrementing ref-count in array/vector copy and other refactors#10335
Merged
fix(brillig): Skip decrementing ref-count in array/vector copy and other refactors#10335
Conversation
aakoshh
changed the title
Contributor
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Contributor
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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: b761766 | Previous: 2a27b18 | Ratio |
|---|---|---|---|
rollup-block-root-single-tx |
0.003 s |
0.002 s |
1.50 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Contributor
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: b761766 | Previous: 2a27b18 | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
2 s |
1 s |
2 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
aakoshh
changed the title
Contributor
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Contributor
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: 7db34b7 | Previous: 1324e73 | Ratio |
|---|---|---|---|
sha512-100-bytes |
1.965 s |
1.575 s |
1.25 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Contributor
There was a problem hiding this comment.
Test Suite Duration
Details
| Benchmark suite | Current: b761766 | Previous: 2a27b18 | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr |
131 s |
117 s |
1.12 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts |
177 s |
156 s |
1.13 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
386 s |
414 s |
0.93 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib |
295 s |
296 s |
1.00 |
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types |
111 s |
112 s |
0.99 |
test_report_noir-lang_noir-bignum_ |
162 s |
163 s |
0.99 |
test_report_noir-lang_noir_bigcurve_ |
348 s |
403 s |
0.86 |
test_report_noir-lang_sha256_ |
15 s |
16 s |
0.94 |
test_report_noir-lang_sha512_ |
14 s |
14 s |
1 |
test_report_zkpassport_noir-ecdsa_ |
2 s |
1 s |
2 |
test_report_zkpassport_noir_rsa_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Compilation Time
Details
| Benchmark suite | Current: b761766 | Previous: 2a27b18 | Ratio |
|---|---|---|---|
private-kernel-inner |
1.926 s |
2.072 s |
0.93 |
private-kernel-reset |
7.722 s |
7.782 s |
0.99 |
private-kernel-tail |
1.862 s |
1.71 s |
1.09 |
rollup-block-root-first-empty-tx |
1.354 s |
1.478 s |
0.92 |
rollup-block-root-single-tx |
1.37 s |
1.4 s |
0.98 |
rollup-block-root |
1.39 s |
1.4 s |
0.99 |
rollup-checkpoint-merge |
1.422 s |
1.492 s |
0.95 |
rollup-checkpoint-root-single-block |
398 s |
375 s |
1.06 |
rollup-checkpoint-root |
397 s |
404 s |
0.98 |
rollup-root |
1.494 s |
1.482 s |
1.01 |
rollup-tx-base-private |
19.9 s |
21.92 s |
0.91 |
rollup-tx-base-public |
81.9 s |
85.24 s |
0.96 |
rollup-tx-merge |
1.348 s |
1.358 s |
0.99 |
semaphore-depth-10 |
0.82 s |
0.77 s |
1.06 |
sha512-100-bytes |
1.56 s |
1.653 s |
0.94 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Compilation Memory
Details
| Benchmark suite | Current: b761766 | Previous: 2a27b18 | Ratio |
|---|---|---|---|
private-kernel-inner |
267.81 MB |
267.81 MB |
1 |
private-kernel-reset |
494.31 MB |
494.31 MB |
1 |
private-kernel-tail |
237.69 MB |
237.69 MB |
1 |
rollup-block-root-first-empty-tx |
334.33 MB |
334.33 MB |
1 |
rollup-block-root-single-tx |
332.77 MB |
332.77 MB |
1 |
rollup-block-root |
335.55 MB |
335.56 MB |
1.00 |
rollup-checkpoint-merge |
335.53 MB |
335.53 MB |
1 |
rollup-checkpoint-root-single-block |
11250 MB |
11250 MB |
1 |
rollup-checkpoint-root |
11250 MB |
11250 MB |
1 |
rollup-root |
337.06 MB |
337.06 MB |
1 |
rollup-tx-base-private |
1070 MB |
1070 MB |
1 |
rollup-tx-base-public |
3030 MB |
3030 MB |
1 |
rollup-tx-merge |
332.35 MB |
332.35 MB |
1 |
semaphore_depth_10 |
92.32 MB |
92.32 MB |
1 |
sha512_100_bytes |
185.61 MB |
185.63 MB |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Execution Memory
Details
| Benchmark suite | Current: b761766 | Previous: 2a27b18 | Ratio |
|---|---|---|---|
private-kernel-inner |
257.39 MB |
257.39 MB |
1 |
private-kernel-reset |
290.79 MB |
290.79 MB |
1 |
private-kernel-tail |
237.86 MB |
237.86 MB |
1 |
rollup-block-root |
333.67 MB |
333.67 MB |
1 |
rollup-checkpoint-merge |
332.43 MB |
332.43 MB |
1 |
rollup-checkpoint-root-single-block |
1760 MB |
1760 MB |
1 |
rollup-checkpoint-root |
1760 MB |
1760 MB |
1 |
rollup-root |
333.65 MB |
333.65 MB |
1 |
rollup-tx-base-private |
521.11 MB |
521.11 MB |
1 |
rollup-tx-base-public |
467.58 MB |
467.58 MB |
1 |
rollup-tx-merge |
331.89 MB |
331.89 MB |
1 |
semaphore_depth_10 |
73.84 MB |
73.84 MB |
1 |
sha512_100_bytes |
72.08 MB |
72.08 MB |
1 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
Author
|
No noticeable change in memory. @vezenovm @TomAFrench do you have an opinion on whether its worth trying to decrement the ref-count at all? |
Contributor
|
@aakoshh what about for external repos? I'd think they may be more impacted. That being said, this sounds related to when we removed DecRc from ssa-gen after discovering performance improved with it gone |
Contributor
Our benches do contain the external aztec protocol circuits if that is what you are referring to @jfecher. @aakoshh I am not sure it is worth trying to decrement the RC at all. As we currently do not write the decremented RC back to the heap (which is incorrect) it looks like the decrement does not provide much of a benefit based off of the opcode diff. In fact, it looks like it mostly causes an opcode count degradation aside for radix decomposition. I am curious the results if we were to simply remove this decrement. I imagine it would be similar to our results from removing dec_rc from the SSA gen. We could just branch of off this PR to test that as I see you have lots of other cleanup / documentation as part of this PR. |
Contributor
Author
|
@jfecher the “Execution Memory” and “Time” reports are based on the external repos, if that’s what you meant. They didn’t show any change. By not doing decrement, we would save 2 opcodes in the bytecode size (in the procedures) and 2 opcodes in execution per copy we make. Intuitively once a copy is made, saving 2 opcodes shouldn’t make much of a difference. OTOH if we decrease to 1 we might save a future copy. But that’s circumstantial. |
Contributor
Yeah it is very circumstantial and looks to be very unlikely. It also would be good for Brillig gen to be aligned with SSA gen. We can look at bringing back dec_rc more generally in the future. |
Contributor
Author
|
Okay, I’ll open a follow up PR to remove it |
Contributor
|
Could be worth revisiting this in the future when ownership optimizations change since we still have more conceptual clones (inc_rcs) being inserted there. |
5 tasks
aakoshh
changed the title
jfecher
approved these changes
Nov 10, 2025
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
github-merge-queue bot
pushed a commit
to AztecProtocol/aztec-packages
that referenced
this pull request
Nov 12, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix(frontend)!: Preserve int type when quoting tokens (noir-lang/noir#10330) fix: check overflow for Pedersen grumpkin scalars (noir-lang/noir#10462) chore(frontend): Various tests in elaborator expressions submodule and minor refactors (noir-lang/noir#10475) chore: bump external pinned commits (noir-lang/noir#10477) fix: disallow keywords in attributes (noir-lang/noir#10473) chore: refactor codegen_control_flow (noir-lang/noir#10320) fix: builtin with body now errors instead of crashing (noir-lang/noir#10474) fix: handle ambiguous trait methods in assumed traits (noir-lang/noir#10468) fix: force_substitute bindings during monomorphization for associated constants (noir-lang/noir#10467) fix(brillig): Skip decrementing ref-count in array/vector copy and other refactors (noir-lang/noir#10335) fix(ssa): Cast to `u64` when inserting OOB checks in DIE (noir-lang/noir#10463) fix: disallow comptime-only types in non-comptime globals (noir-lang/noir#10458) chore(fuzzing): fix default artifact for brillig target (noir-lang/noir#10465) END_COMMIT_OVERRIDE
github-merge-queue bot
pushed a commit
to AztecProtocol/aztec-packages
that referenced
this pull request
Nov 12, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix(frontend)!: Preserve int type when quoting tokens (noir-lang/noir#10330) fix: check overflow for Pedersen grumpkin scalars (noir-lang/noir#10462) chore(frontend): Various tests in elaborator expressions submodule and minor refactors (noir-lang/noir#10475) chore: bump external pinned commits (noir-lang/noir#10477) fix: disallow keywords in attributes (noir-lang/noir#10473) chore: refactor codegen_control_flow (noir-lang/noir#10320) fix: builtin with body now errors instead of crashing (noir-lang/noir#10474) fix: handle ambiguous trait methods in assumed traits (noir-lang/noir#10468) fix: force_substitute bindings during monomorphization for associated constants (noir-lang/noir#10467) fix(brillig): Skip decrementing ref-count in array/vector copy and other refactors (noir-lang/noir#10335) fix(ssa): Cast to `u64` when inserting OOB checks in DIE (noir-lang/noir#10463) fix: disallow comptime-only types in non-comptime globals (noir-lang/noir#10458) chore(fuzzing): fix default artifact for brillig target (noir-lang/noir#10465) END_COMMIT_OVERRIDE
github-merge-queue bot
pushed a commit
to AztecProtocol/aztec-packages
that referenced
this pull request
Nov 12, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix(frontend)!: Preserve int type when quoting tokens (noir-lang/noir#10330) fix: check overflow for Pedersen grumpkin scalars (noir-lang/noir#10462) chore(frontend): Various tests in elaborator expressions submodule and minor refactors (noir-lang/noir#10475) chore: bump external pinned commits (noir-lang/noir#10477) fix: disallow keywords in attributes (noir-lang/noir#10473) chore: refactor codegen_control_flow (noir-lang/noir#10320) fix: builtin with body now errors instead of crashing (noir-lang/noir#10474) fix: handle ambiguous trait methods in assumed traits (noir-lang/noir#10468) fix: force_substitute bindings during monomorphization for associated constants (noir-lang/noir#10467) fix(brillig): Skip decrementing ref-count in array/vector copy and other refactors (noir-lang/noir#10335) fix(ssa): Cast to `u64` when inserting OOB checks in DIE (noir-lang/noir#10463) fix: disallow comptime-only types in non-comptime globals (noir-lang/noir#10458) chore(fuzzing): fix default artifact for brillig target (noir-lang/noir#10465) END_COMMIT_OVERRIDE
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem*
The last piece of Brillig codegen I haven't audited: procedures.
Summary*
array_copythe decremented value wasn't written back to the heapvector_copythe decrement was missingmake_scratch_registershelper to make it easier to match theallocate_scratch_registersfurther down.codegen_decrease_rchelper to make sure we write back to the heap.codegen_initialize_rcpublicly available so setting the RC to 1 is more uniform.codegen_usize_equals_onehelper to make RC==1 checks a bit easier.codegen_read_vector_metadatato return allocated registers, like all the othercodegen_read_*methods, and make it more difficult to mix up fields and parameter ordering.new_vector_todestination_vector_, so we only have 2 names (destination and target) rather than 3 for the same thing (kept target because it matches the source in the number of letters 🤷 )codegen_read_rcandcodegen_read_vector_rcto reduce the number of places that know the layout of arrays and vectors by reading their pointers into an RC without going through some method.Additional Context
The increase in the number of opcodes
reference_counts_slices_inliner_0test is because I added a newunconstrainedmethod to it.The smaller decreases are because of a reuse of a constant in to_radix for example, rather than declaring it as another constant.
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.