Skip to content

Commit

Permalink
chore: fix poor performance and long compile times in value_note.dere…
Browse files Browse the repository at this point in the history
…ment() (AztecProtocol/aztec-packages#6523)

AztecProtocol/aztec-packages#6405 added a very
complicated call (`get_npk_m_hash`) to `destroy_note`, not realizing
that `destroy_note` is called in a loop by `decrement`. The loop
unrolling caused multiple calls to this function, even though they all
had the same arguments, ultimately resulting in humongous RAM usage
during compilation (over 40GB just for this contract) and very
compilation times (from 30s to multiple minutes).

This PR fixes this simply inlining the code for `destroy_note` and
fetching the key once at the beginning. It does worry me slightly
however that such a large performance hit was not noticed - this likely
affected test times as well.
  • Loading branch information
AztecBot committed May 17, 2024
1 parent 73a635e commit 89b11ae
Show file tree
Hide file tree
Showing 194 changed files with 8,304 additions and 5,132 deletions.
2 changes: 1 addition & 1 deletion .aztec-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1c74387e56b49102043fc6701735325a891e6c65
002b4aa556041aa1a12f0fd09bb5ad0b07f04daa
144 changes: 72 additions & 72 deletions .github/workflows/gates_report.yml
Original file line number Diff line number Diff line change
@@ -1,88 +1,88 @@
name: Report gates diff
# name: Report gates diff

on:
push:
branches:
- master
pull_request:
# on:
# push:
# branches:
# - master
# pull_request:

jobs:
build-nargo:
runs-on: ubuntu-latest
strategy:
matrix:
target: [x86_64-unknown-linux-gnu]
# jobs:
# build-nargo:
# runs-on: ubuntu-latest
# strategy:
# matrix:
# target: [x86_64-unknown-linux-gnu]

steps:
- name: Checkout Noir repo
uses: actions/checkout@v4
# steps:
# - name: Checkout Noir repo
# uses: actions/checkout@v4

- name: Setup toolchain
uses: dtolnay/rust-toolchain@1.74.1
# - name: Setup toolchain
# uses: dtolnay/rust-toolchain@1.74.1

- uses: Swatinem/rust-cache@v2
with:
key: ${{ matrix.target }}
cache-on-failure: true
save-if: ${{ github.event_name != 'merge_group' }}
# - uses: Swatinem/rust-cache@v2
# with:
# key: ${{ matrix.target }}
# cache-on-failure: true
# save-if: ${{ github.event_name != 'merge_group' }}

- name: Build Nargo
run: cargo build --package nargo_cli --release
# - name: Build Nargo
# run: cargo build --package nargo_cli --release

- name: Package artifacts
run: |
mkdir dist
cp ./target/release/nargo ./dist/nargo
7z a -ttar -so -an ./dist/* | 7z a -si ./nargo-x86_64-unknown-linux-gnu.tar.gz
# - name: Package artifacts
# run: |
# mkdir dist
# cp ./target/release/nargo ./dist/nargo
# 7z a -ttar -so -an ./dist/* | 7z a -si ./nargo-x86_64-unknown-linux-gnu.tar.gz

- name: Upload artifact
uses: actions/upload-artifact@v4
with:
name: nargo
path: ./dist/*
retention-days: 3
# - name: Upload artifact
# uses: actions/upload-artifact@v4
# with:
# name: nargo
# path: ./dist/*
# retention-days: 3


compare_gas_reports:
needs: [build-nargo]
runs-on: ubuntu-latest
permissions:
pull-requests: write
# compare_gas_reports:
# needs: [build-nargo]
# runs-on: ubuntu-latest
# permissions:
# pull-requests: write

steps:
- uses: actions/checkout@v4
# steps:
# - uses: actions/checkout@v4

- name: Download nargo binary
uses: actions/download-artifact@v4
with:
name: nargo
path: ./nargo
# - name: Download nargo binary
# uses: actions/download-artifact@v4
# with:
# name: nargo
# path: ./nargo

- name: Set nargo on PATH
run: |
nargo_binary="${{ github.workspace }}/nargo/nargo"
chmod +x $nargo_binary
echo "$(dirname $nargo_binary)" >> $GITHUB_PATH
export PATH="$PATH:$(dirname $nargo_binary)"
nargo -V
# - name: Set nargo on PATH
# run: |
# nargo_binary="${{ github.workspace }}/nargo/nargo"
# chmod +x $nargo_binary
# echo "$(dirname $nargo_binary)" >> $GITHUB_PATH
# export PATH="$PATH:$(dirname $nargo_binary)"
# nargo -V

- name: Generate gates report
working-directory: ./test_programs
run: |
./gates_report.sh
mv gates_report.json ../gates_report.json
# - name: Generate gates report
# working-directory: ./test_programs
# run: |
# ./gates_report.sh
# mv gates_report.json ../gates_report.json

- name: Compare gates reports
id: gates_diff
uses: vezenovm/noir-gates-diff@acf12797860f237117e15c0d6e08d64253af52b6
with:
report: gates_report.json
summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)
# - name: Compare gates reports
# id: gates_diff
# uses: vezenovm/noir-gates-diff@acf12797860f237117e15c0d6e08d64253af52b6
# with:
# report: gates_report.json
# summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)

- name: Add gates diff to sticky comment
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
uses: marocchino/sticky-pull-request-comment@v2
with:
# delete the comment in case changes no longer impact circuit sizes
delete: ${{ !steps.gates_diff.outputs.markdown }}
message: ${{ steps.gates_diff.outputs.markdown }}
# - name: Add gates diff to sticky comment
# if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
# uses: marocchino/sticky-pull-request-comment@v2
# with:
# # delete the comment in case changes no longer impact circuit sizes
# delete: ${{ !steps.gates_diff.outputs.markdown }}
# message: ${{ steps.gates_diff.outputs.markdown }}
5 changes: 5 additions & 0 deletions .github/workflows/test-js-packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,11 @@ jobs:
- name: Checkout
uses: actions/checkout@v4

- name: Download bb binary
run: |
# Adds `bb` to PATH
./scripts/install_bb.sh
- name: Download nargo binary
uses: actions/download-artifact@v4
with:
Expand Down
12 changes: 12 additions & 0 deletions .tokeignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
docs
scripts

# aztec_macros is explicitly considered OOS for Noir audit
aztec_macros

# config files
*.toml
*.md
*.json
*.txt
*.config.mjs
Loading

0 comments on commit 89b11ae

Please sign in to comment.