Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into from

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Nov 20, 2024

Description

Problem*

Summary*

We may not need to increment reference counts in parameters at all since #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:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

Copy link
Contributor

github-actions bot commented Nov 20, 2024

Changes to number of Brillig opcodes executed

Generated at commit: b2b1c32d8184712b920d69362263a1619dd9c4d4, compared to commit: b0245811bfd84e0bf3559aa1e2f37ec52d08691e

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
array_sort -42 ✅ -7.49%
brillig_rc_regression_6123 -39 ✅ -12.75%

Full diff report 👇
Program Brillig opcodes (+/-) %
fold_2_to_17 1,090,791 (+21,730) +2.03%
bench_2_to_17 588,206 (+11,425) +1.98%
fold_numeric_generic_poseidon 5,103 (+52) +1.03%
no_predicates_numeric_generic_poseidon 5,103 (+52) +1.03%
poseidon2 695 (+1) +0.14%
regression_5252 908,660 (+72) +0.01%
reference_counts 306 (-3) -0.97%
uhashmap 144,588 (-1,752) -1.20%
hashmap 51,261 (-1,675) -3.16%
array_sort 519 (-42) -7.49%
brillig_rc_regression_6123 267 (-39) -12.75%

Copy link
Contributor

github-actions bot commented Nov 20, 2024

Changes to Brillig bytecode sizes

Generated at commit: b2b1c32d8184712b920d69362263a1619dd9c4d4, compared to commit: b0245811bfd84e0bf3559aa1e2f37ec52d08691e

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
hashmap -753 ✅ -3.80%
brillig_rc_regression_6123 -27 ✅ -14.75%

Full diff report 👇
Program Brillig opcodes (+/-) %
regression_5252 4,519 (-6) -0.13%
reference_counts 337 (-3) -0.88%
array_sort 289 (-3) -1.03%
no_predicates_numeric_generic_poseidon 737 (-11) -1.47%
fold_numeric_generic_poseidon 737 (-11) -1.47%
poseidon2 334 (-6) -1.76%
bench_2_to_17 327 (-6) -1.80%
uhashmap 12,922 (-264) -2.00%
fold_2_to_17 559 (-12) -2.10%
hashmap 19,062 (-753) -3.80%
brillig_rc_regression_6123 156 (-27) -14.75%

@jfecher
Copy link
Contributor Author

jfecher commented Nov 20, 2024

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.

Base automatically changed from jf/rearrange-inc-rcs to master November 21, 2024 20:36
@jfecher
Copy link
Contributor Author

jfecher commented Dec 4, 2024

Approach taken here is incorrect. It'd lead to mutation if the same parameter was passed as a reference twice.

@jfecher jfecher closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants