search

noir-lang / noir

Noir is a domain specific language for zero knowledge proofs
https://noir-lang.org
Apache License 2.0
905 stars 206 forks source link

feat: Don't increment parameter ref counts at all #6570

Open jfecher opened 5 days ago

jfecher commented 5 days ago

Description

Problem*

Summary*

We may not need to increment reference counts in parameters at all since https://github.com/noir-lang/noir/pull/6568 eliminates them for arrays, and previously we had eliminated them for references as well, though this was reverted. This PR is speculation that the reversion was only needed because we forgot to also remove the dec_rc in that case, leading to reference counts drifting down too far.

Additional Context

This is passing locally but I think we're missing a inc_rc when dereferencing that is now required with this change. Adding it may be more expensive than just keeping inc_rc for reference parameters though.

Documentation*

Check one:

PR Checklist*

github-actions[bot] commented 5 days ago

Changes to number of Brillig opcodes executed

Generated at commit: ead9d26d9de6cf44aaf92b573bce2296c991ac01, compared to commit: 1b43b06c81c3d173992fd949bad5df7f2435ba4b

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
array_sort -42 ✅ -7.17%
reference_only_used_as_alias -30 ✅ -15.00%
brillig_rc_regression_6123 -228 ✅ -76.00%
Full diff report 👇
| Program | Brillig opcodes (+/-) | % | |:-|-:|-:| | **fold_2_to_17** | 1,168,465 (+21,730) | **+1.89%** | | **bench_2_to_17** | 629,043 (+11,425) | **+1.85%** | | **fold_numeric_generic_poseidon** | 5,416 (+52) | **+0.97%** | | **no_predicates_numeric_generic_poseidon** | 5,416 (+52) | **+0.97%** | | **poseidon2** | 732 (+1) | **+0.14%** | | **regression_5252** | 979,225 (+72) | **+0.01%** | | **eddsa** | 749,056 (-11) | **-0.00%** | | **uhashmap** | 148,074 (-1,740) | **-1.16%** | | **hashmap** | 54,294 (-3,225) | **-5.61%** | | **array_sort** | 544 (-42) | **-7.17%** | | **reference_only_used_as_alias** | 170 (-30) | **-15.00%** | | **brillig_rc_regression_6123** | 72 (-228) | **-76.00%** |
github-actions[bot] commented 5 days ago

Changes to Brillig bytecode sizes

Generated at commit: ead9d26d9de6cf44aaf92b573bce2296c991ac01, compared to commit: 1b43b06c81c3d173992fd949bad5df7f2435ba4b

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
hashmap -666 ✅ -3.24%
reference_only_used_as_alias -30 ✅ -16.76%
brillig_rc_regression_6123 -105 ✅ -58.66%
Full diff report 👇
| Program | Brillig opcodes (+/-) | % | |:-|-:|-:| | **regression_5252** | 4,651 (-6) | **-0.13%** | | **eddsa** | 10,669 (-18) | **-0.17%** | | **array_sort** | 301 (-3) | **-0.99%** | | **fold_numeric_generic_poseidon** | 783 (-11) | **-1.39%** | | **no_predicates_numeric_generic_poseidon** | 783 (-11) | **-1.39%** | | **poseidon2** | 357 (-6) | **-1.65%** | | **bench_2_to_17** | 350 (-6) | **-1.69%** | | **uhashmap** | 13,280 (-252) | **-1.86%** | | **fold_2_to_17** | 605 (-12) | **-1.94%** | | **hashmap** | 19,863 (-666) | **-3.24%** | | **reference_only_used_as_alias** | 149 (-30) | **-16.76%** | | **brillig_rc_regression_6123** | 74 (-105) | **-58.66%** |
jfecher commented 5 days ago

fold_2_to_17 1,168,465 (+21,730) +1.89%

I wonder what more is being executed here? When the only change is to issue fewer inc/decs, reference counts should in theory remain at 1 more often and just lead to more direct mutation.