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.

Sign up for GitHub

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

Jump to bottom

fix(performance): Accurately mark safe constant indices for arrays of complex types #7491

Merged
merged 2 commits into from
Feb 22, 2025
Merged

fix(performance): Accurately mark safe constant indices for arrays of complex types #7491

merged 2 commits into from
Feb 22, 2025

Conversation

vezenovm
Copy link
Contributor

Description

Problem*

No issue but something I noticed while working towards Brillig execution trace optimizations.

We flatten complex types within arrays.
For example this program:

struct Foo {
    bar: Field,
    baz: Field,
}

fn main(foos: [Foo; 3]) -> pub Field {
    foos[2].bar + foos[2].baz
}

Will have the following SSA:

brillig(inline) fn main f0 {
  b0(v0: [(Field, Field); 3]):
    v2 = array_get v0, index u32 4 -> Field
    v4 = array_get v0, index u32 5 -> Field
    v5 = array_get v0, index u32 4 -> Field
    v6 = array_get v0, index u32 5 -> Field
    v7 = add v2, v6
    return v7
}

When it comes times to generate Brillig we will check whether to generate bytecode for array index validation by checking this method:

noir/compiler/noirc_evaluator/src/ssa/ir/dfg.rs

Line 658 in 8e68ce6

pub(crate) fn is_safe_index(&self, index: ValueId, array: ValueId) -> bool {

We are only checkling the len from Type::Array(..). Calling this method will thus fail for all of the above array gets. Even though we know the array index is safe.

Summary*

Use the elements array in Type::Array(elements, len) and check the index against elements.len() * len.

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.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 21, 2025 •
edited
Loading

Changes to Brillig bytecode sizes

Generated at commit: da0eb78f450e4b13a6e8e92bc38fae4ae77e8d88, compared to commit: 8e68ce6403e99ff5af4fb9973b203070d8a9898c

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
fold_complex_outputs_inliner_max -40 ✅ -8.55%
fold_complex_outputs_inliner_min -72 ✅ -10.84%
struct_array_inputs_inliner_max -11 ✅ -17.19%
struct_array_inputs_inliner_min -11 ✅ -17.19%
struct_array_inputs_inliner_zero -11 ✅ -17.19%
Full diff report 👇
Program Brillig opcodes (+/-) %
hashmap_inliner_max 19,748 (-4) -0.02%
uhashmap_inliner_max 11,486 (-4) -0.03%
hashmap_inliner_min 9,205 (-4) -0.04%
hashmap_inliner_zero 7,525 (-4) -0.05%
uhashmap_inliner_min 7,384 (-4) -0.05%
uhashmap_inliner_zero 6,758 (-4) -0.06%
regression_6674_3_inliner_min 834 (-8) -0.95%
brillig_cow_regression_inliner_min 2,277 (-24) -1.04%
brillig_cow_regression_inliner_zero 2,044 (-24) -1.16%
brillig_cow_regression_inliner_max 2,016 (-24) -1.18%
regression_6674_3_inliner_max 498 (-8) -1.58%
regression_6674_3_inliner_zero 498 (-8) -1.58%
simple_shield_inliner_min 820 (-20) -2.38%
simple_shield_inliner_max 806 (-20) -2.42%
simple_shield_inliner_zero 702 (-20) -2.77%
nested_array_dynamic_inliner_max 2,302 (-80) -3.36%
nested_array_dynamic_inliner_min 1,884 (-68) -3.48%
nested_array_dynamic_inliner_zero 1,884 (-68) -3.48%
brillig_pedersen_inliner_min 529 (-20) -3.64%
pedersen_check_inliner_min 529 (-20) -3.64%
brillig_pedersen_inliner_max 490 (-20) -3.92%
pedersen_check_inliner_max 490 (-20) -3.92%
pedersen_check_inliner_zero 479 (-20) -4.01%
brillig_pedersen_inliner_zero 479 (-20) -4.01%
merkle_insert_inliner_min 476 (-20) -4.03%
merkle_insert_inliner_zero 445 (-20) -4.30%
pedersen_hash_inliner_min 338 (-20) -5.59%
merkle_insert_inliner_max 665 (-40) -5.67%
pedersen_hash_inliner_max 314 (-20) -5.99%
pedersen_hash_inliner_zero 314 (-20) -5.99%
nested_arrays_from_brillig_inliner_min 164 (-11) -6.29%
fold_complex_outputs_inliner_zero 511 (-40) -7.26%
nested_array_dynamic_simple_inliner_max 98 (-8) -7.55%
nested_array_dynamic_simple_inliner_min 98 (-8) -7.55%
nested_array_dynamic_simple_inliner_zero 98 (-8) -7.55%
fold_complex_outputs_inliner_max 428 (-40) -8.55%
fold_complex_outputs_inliner_min 592 (-72) -10.84%
struct_array_inputs_inliner_max 53 (-11) -17.19%
struct_array_inputs_inliner_min 53 (-11) -17.19%
struct_array_inputs_inliner_zero 53 (-11) -17.19%

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 21, 2025 •
edited
Loading

Changes to number of Brillig opcodes executed

Generated at commit: da0eb78f450e4b13a6e8e92bc38fae4ae77e8d88, compared to commit: 8e68ce6403e99ff5af4fb9973b203070d8a9898c

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
struct_array_inputs_inliner_max -6 ✅ -6.38%
struct_array_inputs_inliner_min -6 ✅ -6.38%
struct_array_inputs_inliner_zero -6 ✅ -6.38%
nested_array_dynamic_simple_inliner_max -6 ✅ -6.52%
nested_array_dynamic_simple_inliner_min -6 ✅ -6.52%
nested_array_dynamic_simple_inliner_zero -6 ✅ -6.52%
Full diff report 👇
Program Brillig opcodes (+/-) %
uhashmap_inliner_min 183,027 (-3) -0.00%
uhashmap_inliner_zero 163,384 (-3) -0.00%
uhashmap_inliner_max 128,591 (-3) -0.00%
brillig_cow_regression_inliner_min 565,456 (-18) -0.00%
brillig_cow_regression_inliner_zero 561,884 (-18) -0.00%
hashmap_inliner_min 86,959 (-3) -0.00%
brillig_cow_regression_inliner_max 481,937 (-18) -0.00%
hashmap_inliner_zero 72,462 (-3) -0.00%
hashmap_inliner_max 48,290 (-3) -0.01%
regression_6674_3_inliner_min 1,959 (-6) -0.31%
regression_6674_3_inliner_max 1,397 (-6) -0.43%
regression_6674_3_inliner_zero 1,397 (-6) -0.43%
nested_array_dynamic_inliner_min 3,511 (-51) -1.43%
nested_array_dynamic_inliner_zero 3,511 (-51) -1.43%
brillig_pedersen_inliner_min 905 (-15) -1.63%
pedersen_check_inliner_min 905 (-15) -1.63%
simple_shield_inliner_min 2,509 (-45) -1.76%
nested_array_dynamic_inliner_max 3,306 (-60) -1.78%
simple_shield_inliner_max 2,467 (-45) -1.79%
simple_shield_inliner_zero 2,451 (-45) -1.80%
brillig_pedersen_inliner_zero 734 (-15) -2.00%
pedersen_check_inliner_zero 734 (-15) -2.00%
brillig_pedersen_inliner_max 705 (-15) -2.08%
pedersen_check_inliner_max 705 (-15) -2.08%
merkle_insert_inliner_zero 3,548 (-90) -2.47%
merkle_insert_inliner_max 3,413 (-90) -2.57%
merkle_insert_inliner_min 3,336 (-90) -2.63%
pedersen_hash_inliner_min 533 (-15) -2.74%
nested_arrays_from_brillig_inliner_min 205 (-6) -2.84%
pedersen_hash_inliner_max 469 (-15) -3.10%
pedersen_hash_inliner_zero 469 (-15) -3.10%
fold_complex_outputs_inliner_zero 902 (-30) -3.22%
fold_complex_outputs_inliner_max 704 (-30) -4.09%
fold_complex_outputs_inliner_min 985 (-54) -5.20%
struct_array_inputs_inliner_max 88 (-6) -6.38%
struct_array_inputs_inliner_min 88 (-6) -6.38%
struct_array_inputs_inliner_zero 88 (-6) -6.38%
nested_array_dynamic_simple_inliner_max 86 (-6) -6.52%
nested_array_dynamic_simple_inliner_min 86 (-6) -6.52%
nested_array_dynamic_simple_inliner_zero 86 (-6) -6.52%

@vezenovm vezenovm requested a review from a team February 21, 2025 21:55
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 21, 2025 •
edited
Loading

Changes to circuit sizes

Generated at commit: da0eb78f450e4b13a6e8e92bc38fae4ae77e8d88, compared to commit: 8e68ce6403e99ff5af4fb9973b203070d8a9898c

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
nested_array_dynamic -40 ✅ -1.19% -40 ✅ -0.31%
hashmap -5,520 ✅ -15.00% -5,520 ✅ -5.64%
Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
nested_array_dynamic 3,311 (-40) -1.19% 12,954 (-40) -0.31%
hashmap 31,268 (-5,520) -15.00% 92,272 (-5,520) -5.64%

jfecher
jfecher approved these changes Feb 22, 2025
View reviewed changes
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Nice catch!

@jfecher jfecher added this pull request to the merge queue Feb 22, 2025
Merged via the queue into master with commit ffcc6f8 Feb 22, 2025
102 checks passed
@jfecher jfecher deleted the mv/safe-index-for-arrays-of-complex-types branch February 22, 2025 01:19
@noirwhal noirwhal mentioned this pull request Feb 21, 2025
Open
TomAFrench added a commit that referenced this pull request Feb 24, 2025
@TomAFrench
Merge branch 'master' into ab/no-duplicate-error
5916a13 
* master:
  feat(experimental): Support struct constructors in match patterns (#7489)
  feat: use resolved type instead of needing Constructor.struct_type (#7500)
  feat: better error message when keyword is found instead of type in p… (#7501)
  chore: bump external pinned commits (#7497)
  feat(experimental): Add invalid pattern syntax error (#7487)
  fix(performance): Accurately mark safe constant indices for arrays of complex types (#7491)
TomAFrench added a commit that referenced this pull request Feb 25, 2025
@TomAFrench
Merge branch 'master' into michaeljklein/iterative-quicksort
e39bdce 
* master: (74 commits)
  feat: optimize out range checks on limiting cases (#7510)
  chore: clippy fixes (#7505)
  chore(docs): Supplement docs on `modexp` as a required precompile for Barretenberg's Solidity verifier (#7508)
  feat(debugger): REPL add breakpoint by sourcecode line (#5204)
  fix: issue duplicate error on impl function without self (#7490)
  feat(experimental): Support struct constructors in match patterns (#7489)
  feat: use resolved type instead of needing Constructor.struct_type (#7500)
  feat: better error message when keyword is found instead of type in p… (#7501)
  chore: bump external pinned commits (#7497)
  feat(experimental): Add invalid pattern syntax error (#7487)
  fix(performance): Accurately mark safe constant indices for arrays of complex types (#7491)
  fix(experimental): Allow shadowing in match patterns (#7484)
  chore: regression test #7195 (#7233)
  chore(docs): Section on `noir-profiler execution-opcodes` (#7480)
  chore: improve proptesting of 128bit values in `noirc_abi` (#7485)
  chore(profiler): Use brillig names for outputted flamegraphs  (#7470)
  chore(docs): Profiler images reference (#7481)
  fix: don't use dummy location when inserting debug code (#7482)
  feat(cli)!: Add `--unstable-features` to gate unstable features (#7449)
  feat: Sync from aztec-packages (#7474)
  ...
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 26, 2025
@AztecBot
[1 changes] chore: bump external pinned commits (noir-lang/noir#7515)
9646851 
feat: let all compiler errors carry a Location instead of a Span (noir-lang/noir#7486)
chore: Increaes base64's allotted time (noir-lang/noir#7521)
fix: don't crash on broken impl syntax (noir-lang/noir#7512)
feat: optimize out range checks on limiting cases (noir-lang/noir#7510)
chore: clippy fixes (noir-lang/noir#7505)
chore(docs): Supplement docs on `modexp` as a required precompile for Barretenberg's Solidity verifier (noir-lang/noir#7508)
feat(debugger): REPL add breakpoint by sourcecode line (noir-lang/noir#5204)
fix: issue duplicate error on impl function without self (noir-lang/noir#7490)
feat(experimental): Support struct constructors in match patterns (noir-lang/noir#7489)
feat: use resolved type instead of needing Constructor.struct_type (noir-lang/noir#7500)
feat: better error message when keyword is found instead of type in p… (noir-lang/noir#7501)
chore: bump external pinned commits (noir-lang/noir#7497)
feat(experimental): Add invalid pattern syntax error (noir-lang/noir#7487)
fix(performance): Accurately mark safe constant indices for arrays of complex types (noir-lang/noir#7491)
fix(experimental): Allow shadowing in match patterns (noir-lang/noir#7484)
chore: regression test #7195 (noir-lang/noir#7233)
chore(docs): Section on `noir-profiler execution-opcodes` (noir-lang/noir#7480)
chore: improve proptesting of 128bit values in `noirc_abi` (noir-lang/noir#7485)
chore(profiler): Use brillig names for outputted flamegraphs  (noir-lang/noir#7470)
chore(docs): Profiler images reference (noir-lang/noir#7481)
fix: don't use dummy location when inserting debug code (noir-lang/noir#7482)
feat(cli)!: Add `--unstable-features` to gate unstable features (noir-lang/noir#7449)
@AztecBot AztecBot mentioned this pull request Feb 26, 2025
Merged
TomAFrench pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Feb 26, 2025
@AztecBot @guipublic
feat: Sync from noir (#12279)
86dbffb 
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
chore: bump external pinned commits
(noir-lang/noir#7515)
feat: let all compiler errors carry a Location instead of a Span
(noir-lang/noir#7486)
chore: Increaes base64's allotted time
(noir-lang/noir#7521)
fix: don't crash on broken impl syntax
(noir-lang/noir#7512)
feat: optimize out range checks on limiting cases
(noir-lang/noir#7510)
chore: clippy fixes (noir-lang/noir#7505)
chore(docs): Supplement docs on `modexp` as a required precompile for
Barretenberg's Solidity verifier
(noir-lang/noir#7508)
feat(debugger): REPL add breakpoint by sourcecode line
(noir-lang/noir#5204)
fix: issue duplicate error on impl function without self
(noir-lang/noir#7490)
feat(experimental): Support struct constructors in match patterns
(noir-lang/noir#7489)
feat: use resolved type instead of needing Constructor.struct_type
(noir-lang/noir#7500)
feat: better error message when keyword is found instead of type in p…
(noir-lang/noir#7501)
chore: bump external pinned commits
(noir-lang/noir#7497)
feat(experimental): Add invalid pattern syntax error
(noir-lang/noir#7487)
fix(performance): Accurately mark safe constant indices for arrays of
complex types (noir-lang/noir#7491)
fix(experimental): Allow shadowing in match patterns
(noir-lang/noir#7484)
chore: regression test #7195
(noir-lang/noir#7233)
chore(docs): Section on `noir-profiler execution-opcodes`
(noir-lang/noir#7480)
chore: improve proptesting of 128bit values in `noirc_abi`
(noir-lang/noir#7485)
chore(profiler): Use brillig names for outputted flamegraphs
(noir-lang/noir#7470)
chore(docs): Profiler images reference
(noir-lang/noir#7481)
fix: don't use dummy location when inserting debug code
(noir-lang/noir#7482)
feat(cli)!: Add `--unstable-features` to gate unstable features
(noir-lang/noir#7449)
END_COMMIT_OVERRIDE

---------

Co-authored-by: guipublic <guipublic@gmail.com>
Co-authored-by: guipublic <47281315+guipublic@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfecher jfecher jfecher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vezenovm @jfecher