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: separate slice and array types in the AST + as_slice builtin #4550

Closed

Conversation

michaeljklein
Copy link
Contributor

Description

Problem*

This draft PR is a combination of:

to evaluate the performance effects of the as_slice builtin.

Summary*

Additional Context

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.

michaeljklein and others added 30 commits March 6, 2024 17:05
…ersand, updating stdlib, fixed nested slice for new type, found missing unification case, added more specific error for missing array length, debugging slice parser, added noirc_frontend test for slices, debugging stdlib failing to build
…type check case, updating test_programs, defaulting unknown array length to zero, cleanup debugging println, add utf8 decoding and serde error to execution json decoding error message, cargo clippy/fmt
…es for slices, fix slice literal in regression test
…e debugger error (timeout in brillig_cow_regression)
… to match similar ones, recreated debugger error in execution test
… out brillig and regular as_slice tests, cargo fmt/clippy
…ference counting changes on master, cargo clippy / fmt
@michaeljklein michaeljklein requested a review from vezenovm March 13, 2024 20:33
Copy link
Contributor

github-actions bot commented Mar 13, 2024

Changes to circuit sizes

Generated at commit: 5f813c39d33d4846cff2ed4e14a7cd33d07b1e41, compared to commit: 4c3ac2958fcaaa90eddb60e92cd30eac50b5d2b5

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
keccak256 +199 ❌ +261.84% +42,624 ❌ +77.74%
double_verify_proof_recursive +839 ❌ +41950.00% +2,810 ❌ +0.56%
double_verify_proof +839 ❌ +41950.00% +2,810 ❌ +0.56%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
keccak256 275 (+199) +261.84% 97,454 (+42,624) +77.74%
double_verify_proof_recursive 841 (+839) +41950.00% 503,777 (+2,810) +0.56%
double_verify_proof 841 (+839) +41950.00% 503,777 (+2,810) +0.56%
ecdsa_secp256r1 260 (+98) +60.49% 67,649 (+318) +0.47%
7 87 (+17) +24.29% 19,350 (0) 0.00%
schnorr 140 (+32) +29.63% 50,240 (0) 0.00%
array_dynamic_blackbox_input 889 (+130) +17.13% 45,970 (0) 0.00%
ecdsa_secp256k1 415 (+182) +78.11% 41,049 (0) 0.00%
array_dynamic_nested_blackbox_input 169 (+4) +2.42% 38,799 (0) 0.00%
conditional_regression_short_circuit 113 (+17) +17.71% 38,799 (0) 0.00%
6 87 (+17) +24.29% 38,799 (0) 0.00%
blake3 87 (+17) +24.29% 18,774 (0) 0.00%
conditional_1 7,183 (+12) +0.17% 38,799 (0) 0.00%

michaeljklein and others added 7 commits March 14, 2024 16:32
… array lengths, support ArrayLen on acir var's when constant and the appropriate type, fix element type in generated ssa ir for as_slice, fix printer not distinguishing between slices and arrays, use slice literal instead of as_slice for merkle insert, update as_slice test with cases from debugging
@jfecher
Copy link
Contributor

jfecher commented Mar 15, 2024

Is this far enough along yet to get the performance results compared to master?

@michaeljklein michaeljklein marked this pull request as ready for review March 15, 2024 17:03
@michaeljklein
Copy link
Contributor Author

comparing cargo test after cargo build locally:

  • this PR
cargo test  417.92s user 18.77s system 672% cpu 1:04.91 total
  • master:
cargo test  415.30s user 18.95s system 659% cpu 1:05.83 total

@michaeljklein michaeljklein deleted the michaeljklein/separate-slice-ast-builtin-as-slice branch March 15, 2024 19:38
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.

3 participants