Skip to content

chore: better logic for checking whether an array index could have si…#11799

Merged
TomAFrench merged 2 commits intomasterfrom
ab/index-side-effects
Mar 12, 2026
Merged

chore: better logic for checking whether an array index could have si…#11799
TomAFrench merged 2 commits intomasterfrom
ab/index-side-effects

Conversation

@asterite
Copy link
Collaborator

@asterite asterite commented Mar 6, 2026

…de effects

Description

Problem

Follow-up to #11792

Summary

This is totally optional, but I find that in most cases it makes the output of nargo expand more reasonable. There's a tradeoff: we need to work (a bit) harder to determine whether we need a separate block of statements for the index. On the other hand, if we determine we don't need that block we can save a bit of memory. And I think doing this check should be pretty fast (it gets expressions from an Arena so it indexes directly into memory).

Additional Context

User Documentation

Check one:

  • No user documentation needed.
  • Changes in docs/ included in this PR.
  • [For Experimental Features] Changes in docs/ 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 github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 4d12c62 Previous: 7214f72 Ratio
sha512-100-bytes 0.068 s 0.056 s 1.21

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Brillig Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 4d12c62 Previous: 7214f72 Ratio
private-kernel-reset 0.051 s 0.042 s 1.21

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: b6ab64a Previous: 7214f72 Ratio
test_report_zkpassport_noir-ecdsa_ 3 s 2 s 1.50

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: b6ab64a Previous: 7214f72 Ratio
rollup-block-root-single-tx 2.04 s 1.69 s 1.21

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@asterite asterite requested a review from a team March 6, 2026 14:54
@TomAFrench TomAFrench added this pull request to the merge queue Mar 12, 2026
Merged via the queue into master with commit 1aacfa9 Mar 12, 2026
137 checks passed
@TomAFrench TomAFrench deleted the ab/index-side-effects branch March 12, 2026 16:29
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