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: PR 6674 with PR 6585 #6692

Closed
wants to merge 17 commits into from
Closed

chore: PR 6674 with PR 6585 #6692

wants to merge 17 commits into from

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Dec 3, 2024

Description

Problem*

Simply pushing a draft to test against CI

Summary*

Merges PR #6682 and #6585

Additional Context

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.

@vezenovm vezenovm added the run-external-checks Trigger CI job to run tests on external repos label Dec 3, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

Changes to Brillig bytecode sizes

Generated at commit: c1fbb75a0f0d5d6b168de81a5e68153292938ae2, compared to commit: 80c5c7d83abdd29e7cd157dfdb1880cde68f48bb

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
nested_array_dynamic +310 ❌ +14.43%
nested_array_in_slice +127 ❌ +11.49%
nested_dyn_array_regression_5782 +15 ❌ +9.62%
regression_struct_array_conditional +44 ❌ +8.29%
brillig_nested_arrays +11 ❌ +6.43%
brillig_rc_regression_6123 +11 ❌ +6.01%

Full diff report 👇
Program Brillig opcodes (+/-) %
nested_array_dynamic 2,458 (+310) +14.43%
nested_array_in_slice 1,232 (+127) +11.49%
nested_dyn_array_regression_5782 171 (+15) +9.62%
regression_struct_array_conditional 575 (+44) +8.29%
brillig_nested_arrays 182 (+11) +6.43%
brillig_rc_regression_6123 194 (+11) +6.01%
fold_2_to_17 605 (+34) +5.95%
bench_2_to_17 350 (+17) +5.11%
poseidon2 357 (+17) +5.00%
slice_loop 269 (+12) +4.67%
no_predicates_numeric_generic_poseidon 782 (+34) +4.55%
fold_numeric_generic_poseidon 782 (+34) +4.55%
array_dynamic_nested_blackbox_input 900 (+36) +4.17%
array_sort 304 (+12) +4.11%
hashmap 20,622 (+807) +4.07%
slices 1,792 (+58) +3.34%
uhashmap 13,545 (+359) +2.72%
wildcard_type 297 (+7) +2.41%
6 1,135 (+22) +1.98%
conditional_regression_short_circuit 1,208 (+22) +1.85%
array_to_slice 691 (+12) +1.77%
bigint 2,014 (+33) +1.67%
regression_5252 4,594 (+69) +1.52%
to_be_bytes 211 (+3) +1.44%
poseidon_bn254_hash 5,405 (+75) +1.41%
poseidon_bn254_hash_width_3 5,405 (+75) +1.41%
sha256 2,237 (+30) +1.36%
poseidonsponge_x5_254 4,217 (+54) +1.30%
regression_capacity_tracker 237 (+3) +1.28%
regression_4449 733 (+9) +1.24%
slice_regex 2,173 (+24) +1.12%
slice_dynamic_index 2,503 (+27) +1.09%
ecdsa_secp256k1 904 (+9) +1.01%
sha256_var_witness_const_regression 1,241 (+12) +0.98%
array_dynamic_blackbox_input 1,028 (+9) +0.88%
brillig_cow 371 (+3) +0.82%
conditional_1 1,184 (+9) +0.77%
sha256_var_size_regression 1,713 (+12) +0.71%
aes128_encrypt 523 (+3) +0.58%
sha2_byte 2,735 (+15) +0.55%
keccak256 1,793 (+9) +0.50%
sha256_regression 6,553 (+30) +0.46%
sha256_var_padding_regression 4,771 (+20) +0.42%
regression 944 (+3) +0.32%
ram_blowup_regression 954 (+3) +0.32%
sha256_brillig_performance_regression 1,633 (+3) +0.18%
brillig_cow_regression 2,138 (+3) +0.14%

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Changes to number of Brillig opcodes executed

Generated at commit: c1fbb75a0f0d5d6b168de81a5e68153292938ae2, compared to commit: 80c5c7d83abdd29e7cd157dfdb1880cde68f48bb

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
fold_2_to_17 +465,930 ❌ +43.58%
bench_2_to_17 +244,965 ❌ +42.47%
fold_numeric_generic_poseidon +1,427 ❌ +28.25%
no_predicates_numeric_generic_poseidon +1,427 ❌ +28.25%
slice_loop +184 ❌ +24.27%
poseidon2 +165 ❌ +23.78%

Full diff report 👇
Program Brillig opcodes (+/-) %
fold_2_to_17 1,534,991 (+465,930) +43.58%
bench_2_to_17 821,746 (+244,965) +42.47%
fold_numeric_generic_poseidon 6,478 (+1,427) +28.25%
no_predicates_numeric_generic_poseidon 6,478 (+1,427) +28.25%
slice_loop 942 (+184) +24.27%
poseidon2 859 (+165) +23.78%
hashmap 63,333 (+10,397) +19.64%
uhashmap 168,300 (+21,960) +15.01%
nested_dyn_array_regression_5782 170 (+15) +9.68%
slices 3,097 (+228) +7.95%
nested_array_dynamic 3,460 (+246) +7.65%
slice_dynamic_index 4,614 (+326) +7.60%
nested_array_in_slice 1,564 (+105) +7.20%
brillig_rc_regression_6123 327 (+21) +6.86%
brillig_nested_arrays 155 (+9) +6.16%
regression_struct_array_conditional 1,671 (+94) +5.96%
array_sort 589 (+28) +4.99%
wildcard_type 517 (+24) +4.87%
slice_regex 3,420 (+30) +0.88%
regression_5252 916,472 (+7,884) +0.87%
array_to_slice 1,643 (+12) +0.74%
array_dynamic_nested_blackbox_input 4,545 (+33) +0.73%
poseidonsponge_x5_254 183,665 (+1,230) +0.67%
poseidon_bn254_hash 162,524 (+867) +0.54%
poseidon_bn254_hash_width_3 162,524 (+867) +0.54%
regression_capacity_tracker 911 (+3) +0.33%
regression_4449 200,234 (+630) +0.32%
6 7,107 (+22) +0.31%
conditional_regression_short_circuit 7,185 (+22) +0.31%
brillig_cow 1,138 (+3) +0.26%
sha256 13,874 (+30) +0.22%
sha256_var_witness_const_regression 6,752 (+12) +0.18%
conditional_1 5,707 (+9) +0.16%
ecdsa_secp256k1 6,783 (+9) +0.13%
to_be_bytes 2,451 (+3) +0.12%
regression 2,816 (+3) +0.11%
array_dynamic_blackbox_input 18,195 (+18) +0.10%
sha256_var_size_regression 16,354 (+12) +0.07%
aes128_encrypt 4,502 (+3) +0.07%
keccak256 33,056 (+9) +0.03%
sha256_regression 116,143 (+30) +0.03%
sha2_byte 46,700 (+12) +0.03%
sha256_brillig_performance_regression 22,972 (+3) +0.01%
sha256_var_padding_regression 219,641 (+20) +0.01%
brillig_cow_regression 518,781 (+3) +0.00%
ram_blowup_regression 778,411 (+3) +0.00%

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Peak Memory Sample

Program Peak Memory
keccak256 83.10M
workspace 122.04M
regression_4709 333.55M
ram_blowup_regression 2.65G

Copy link
Contributor

github-actions bot commented Dec 3, 2024

Changes to circuit sizes

Generated at commit: c1fbb75a0f0d5d6b168de81a5e68153292938ae2, compared to commit: 80c5c7d83abdd29e7cd157dfdb1880cde68f48bb

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
nested_array_dynamic +161 ❌ +4.91% +283 ❌ +2.24%
array_dynamic_nested_blackbox_input +6 ❌ +2.43% +17 ❌ +0.23%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
nested_array_dynamic 3,442 (+161) +4.91% 12,933 (+283) +2.24%
array_dynamic_nested_blackbox_input 253 (+6) +2.43% 7,326 (+17) +0.23%

@vezenovm vezenovm marked this pull request as ready for review December 3, 2024 15:19
@vezenovm
Copy link
Contributor Author

vezenovm commented Dec 3, 2024

Marking ready for review just to run external checks

@asterite
Copy link
Collaborator

asterite commented Dec 3, 2024

This PR also makes the two regressions tests in #6687 pass.

@vezenovm
Copy link
Contributor Author

vezenovm commented Dec 3, 2024

This has a new failure now:

[private_kernel_lib] Testing private_kernel_tail_to_public::tests::enqueued_public_calls_with_teardown_gas... FAIL
Failed assertion

error: Assertion failed: mismatch sorted values
   ┌─ /home/runner/work/noir/noir/test-repo/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays/assert_split_sorted_transformed_value_arrays.nr:51:23
   │
51 │             assert_eq(value, sorted_value, "mismatch sorted values");
   │                       -------------------
   │
   = Call stack:
     1. src/private_kernel_tail_to_public.nr:291:29
     2. src/private_kernel_tail_to_public.nr:94:13
     3. src/private_kernel_tail_to_public.nr:47:13
     4. src/components/tail_to_public_output_validator.nr:42:9
     5. src/components/tail_to_public_output_validator.nr:126:9
     6. /home/runner/work/noir/noir/test-repo/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays/assert_split_sorted_transformed_value_arrays.nr:120:5
     7. /home/runner/work/noir/noir/test-repo/noir-projects/noir-protocol-circuits/crates/types/src/utils/arrays/assert_split_sorted_transformed_value_arrays.nr:51:23

Looks like #6585 has the same failure.

@vezenovm
Copy link
Contributor Author

vezenovm commented Dec 3, 2024

Commenting out the call to get_non_mutated_arrays entirely makes that test pass

@vezenovm
Copy link
Contributor Author

vezenovm commented Dec 3, 2024

Closing now that #6585 is merged

@vezenovm vezenovm closed this Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-external-checks Trigger CI job to run tests on external repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants