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

chore: Add Instruction::DecrementRc #4525

Merged
merged 6 commits into from
Mar 13, 2024
Merged

chore: Add Instruction::DecrementRc #4525

merged 6 commits into from
Mar 13, 2024

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Mar 11, 2024

Description

Problem*

Resolves #4522

Summary*

Experimenting with this to see how much it improves performance of unconstrained code using arrays.

Additional Context

Currently the new dec_rc instruction is only issued for function parameters when a function is finished - assuming the parameters are not also returned.

CC @sirasistant for visibility

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] 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 Mar 11, 2024

Changes to circuit sizes

Generated at commit: 17a1d9d2094f462dff330f97412a3445a90fb13d, compared to commit: 44e60b67469de88f20842c4eead64d736f7bd4a0

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap -338 ✅ -0.15% -338 ✅ -0.07%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap 219,271 (-338) -0.15% 491,765 (-338) -0.07%

@jfecher
Copy link
Contributor Author

jfecher commented Mar 11, 2024

This seems to be a considerable improvement for pathological cases at least. Here's an example:

unconstrained fn main() {
    let mut array = [0; 10000];

    for i in 0 .. array.len() {
        array[i] = i;

        // This increments the reference count of `array` on each iteration,
        // tanking performance even though the `array2` parameter of `foo`:
        // 1. is not returned, so its lifetime should not prevent the mutation of `array`
        // 2. is not itself mutable
        foo(array);
    }
}

unconstrained fn foo<N>(_array2: [u64; N]) {}

On master this runs in 5m 50s on my desktop.
On this branch, this runs in 0.67s

@jfecher
Copy link
Contributor Author

jfecher commented Mar 11, 2024

Not sure why the brillig_assert_msg_runtime test is incorrectly passing now.
I suppose the inputs are getting mangled somehow since v73 = eq v0, Field 10 passes but v0 is x and x = 5 in the Prover.toml.
Either that or this is skipped entirely somehow.

@sirasistant
Copy link
Contributor

Not sure why the brillig_assert_msg_runtime test is incorrectly passing now.
I suppose the inputs are getting mangled somehow since v73 = eq v0, Field 10 passes but v0 is x and x = 5 in the Prover.toml.
Either that or this is skipped entirely somehow.

Fixed it here #4531

Copy link
Contributor

@sirasistant sirasistant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if we get some nice speedups!

@sirasistant sirasistant added this pull request to the merge queue Mar 12, 2024
@jfecher jfecher removed this pull request from the merge queue due to a manual request Mar 12, 2024
@jfecher
Copy link
Contributor Author

jfecher commented Mar 12, 2024

Removing this from the queue to investigate why poseidonsponge_x5_254 blew up in size

@jfecher
Copy link
Contributor Author

jfecher commented Mar 12, 2024

I'm still unable to find why poseidonsponge_x5_254 has bloated so much. Oddly enough, the SSA is actually shorter for the version on this branch than in master.

The most I've confirmed so far is that it is due to the new DecrementRc instructions rather than the changed location of the IncrementRc instructions from call arguments to function parameters. I'm not sure why this is though, given both of these are ignored in acir-gen and this test only uses acir functions.

@jfecher
Copy link
Contributor Author

jfecher commented Mar 13, 2024

Found the culprit with @vezenovm's help. IncRc was special cased in the DIE pass but dec rc was not. This lead to dec_rc's values getting counted as used and they weren't eliminated from the program as a result.

@jfecher jfecher enabled auto-merge March 13, 2024 20:08
@vezenovm
Copy link
Contributor

hashmap -338 ✅ -0.15% -338 ✅ -0.07%

Now we actually have an improvement in one case :D

@jfecher jfecher added this pull request to the merge queue Mar 13, 2024
Merged via the queue into master with commit d8710c4 Mar 13, 2024
43 checks passed
@jfecher jfecher deleted the jf/dec-rc branch March 13, 2024 20:28
TomAFrench added a commit that referenced this pull request Mar 14, 2024
* master:
  chore: separate tests for execution failures from compilation failures (#4559)
  feat: remove curly braces with fmt  (#4529)
  fix: Make `nargo` the default binary for cargo run (#4554)
  fix: Evaluate operators in globals in types (#4537)
  chore: Add more `Hash` impls to stdlib (#4470)
  feat: Sync from aztec-packages (#4546)
  chore: Add `Instruction::DecrementRc` (#4525)
  feat: add `nargo compile --watch` command (#4464)
  fix: Substitute generics when checking the field count of a type (#4547)
  feat: optimize sha2 implementation (#4441)
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.

Optimize Brillig reference count tracking
3 participants