From 5c3b420e923f529d3f409d9bf80fb3eb043317c9 Mon Sep 17 00:00:00 2001 From: Tom French Date: Sat, 21 Oct 2023 22:32:11 +0100 Subject: [PATCH 1/7] chore(ci): add circuit size diff reporting --- .github/workflows/gates_report.yml | 87 +++++++++++++++++++++++++ .gitignore | 3 + cspell.json | 1 + tooling/nargo_cli/tests/gates_report.sh | 32 +++++++++ 4 files changed, 123 insertions(+) create mode 100644 .github/workflows/gates_report.yml create mode 100755 tooling/nargo_cli/tests/gates_report.sh diff --git a/.github/workflows/gates_report.yml b/.github/workflows/gates_report.yml new file mode 100644 index 00000000000..715c10d5043 --- /dev/null +++ b/.github/workflows/gates_report.yml @@ -0,0 +1,87 @@ +name: Report gates diff + +on: + push: + branches: + - master + pull_request: + +jobs: + build-nargo: + runs-on: ubuntu-latest + strategy: + matrix: + target: [x86_64-unknown-linux-gnu] + + steps: + - name: Checkout Noir repo + uses: actions/checkout@v4 + + - name: Setup toolchain + uses: dtolnay/rust-toolchain@1.66.0 + + - 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: 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@v3 + with: + name: nargo + path: ./dist/* + retention-days: 3 + + + compare_gas_reports: + runs-on: ubuntu-latest + permissions: + pull-requests: write + + steps: + - uses: actions/checkout@v3 + with: + submodules: recursive + + - name: Download nargo binary + uses: actions/download-artifact@v3 + 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: Generate gates report + working-directory: ./tooling/nargo_cli/tests + run: ./gas_report.sh + + - name: Compare gates reports + id: gates_diff + uses: TomAFrench/noir-gates-diff@e7cf131b7e7f044c01615f93f0b855f65ddc02d4 + with: + report: ./tooling/nargo_cli/tests/gates_report.json + summaryQuantile: 0.9 # only display the 10% most significant gas 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 gas costs + delete: ${{ !steps.gates_diff.outputs.markdown }} + message: ${{ steps.gates_diff.outputs.markdown }} diff --git a/.gitignore b/.gitignore index 94e8f1a8db0..169353af2b6 100644 --- a/.gitignore +++ b/.gitignore @@ -40,6 +40,9 @@ result !tooling/nargo_cli/tests/acir_artifacts/*/target !tooling/nargo_cli/tests/acir_artifacts/*/target/witness.gz !compiler/wasm/noir-script/target + +gates_report.json + # Github Actions scratch space # This gives a location to download artifacts into the repository in CI without making git dirty. libbarretenberg-wasm32 diff --git a/cspell.json b/cspell.json index ac7953e0653..4df858ffcfa 100644 --- a/cspell.json +++ b/cspell.json @@ -88,6 +88,7 @@ "prettytable", "printstd", "pseudocode", + "quantile", "rustc", "rustup", "schnorr", diff --git a/tooling/nargo_cli/tests/gates_report.sh b/tooling/nargo_cli/tests/gates_report.sh new file mode 100755 index 00000000000..18688e5110c --- /dev/null +++ b/tooling/nargo_cli/tests/gates_report.sh @@ -0,0 +1,32 @@ +#!/bin/bash +set -e + +# These tests are incompatible with gas reporting +excluded_dirs=("workspace" "workspace_default_member") +# These tests are temporarily causing issues with gates reporting. +# They should be re-added later. +temp_excluded_dirs=("eddsa") + +current_dir=$(pwd) +base_path="$current_dir/execution_success" +test_dirs=$(ls $base_path) + +# We generate a Noir workspace which contains all of the test cases +# This allows us to generate a gates report using `nargo info` for all of them at once. + +echo "[workspace]" > Nargo.toml +echo "members = [" >> Nargo.toml + +for dir in $test_dirs; do + if [[ " ${excluded_dirs[@]} " =~ " ${dir} " ]] || [[ " ${temp_excluded_dirs[@]} " =~ " ${dir} " ]]; then + continue + fi + + echo " \"execution_success/$dir\"," >> Nargo.toml +done + +echo "]" >> Nargo.toml + +nargo info --json > gates_report.json + +rm Nargo.toml From 71d8ce7a11712edd730cd9db8f2950ed0c8d0452 Mon Sep 17 00:00:00 2001 From: Tom French Date: Sat, 21 Oct 2023 22:33:33 +0100 Subject: [PATCH 2/7] chore: update rayon to avoid stack overflow on eddsa --- Cargo.lock | 20 ++++---------------- tooling/nargo_cli/Cargo.toml | 2 +- tooling/nargo_cli/tests/gates_report.sh | 5 +---- 3 files changed, 6 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c9c60fc62ed..8fb282dda98 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1116,16 +1116,6 @@ dependencies = [ "itertools", ] -[[package]] -name = "crossbeam-channel" -version = "0.5.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a33c2bf77f2df06183c3aa30d1e96c0695a313d4f9c453cc3762a6db39f99200" -dependencies = [ - "cfg-if", - "crossbeam-utils", -] - [[package]] name = "crossbeam-deque" version = "0.8.3" @@ -3187,9 +3177,9 @@ dependencies = [ [[package]] name = "rayon" -version = "1.7.0" +version = "1.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1d2df5196e37bcc87abebc0053e20787d73847bb33134a69841207dd0a47f03b" +checksum = "9c27db03db7734835b3f53954b534c91069375ce6ccaa2e065441e07d9b6cdb1" dependencies = [ "either", "rayon-core", @@ -3197,14 +3187,12 @@ dependencies = [ [[package]] name = "rayon-core" -version = "1.11.0" +version = "1.12.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4b8f95bd6966f5c87776639160a66bd8ab9895d9d4ab01ddba9fc60661aebe8d" +checksum = "5ce3fb6ad83f861aac485e76e1985cd109d9a3713802152be56c3b1f0e0658ed" dependencies = [ - "crossbeam-channel", "crossbeam-deque", "crossbeam-utils", - "num_cpus", ] [[package]] diff --git a/tooling/nargo_cli/Cargo.toml b/tooling/nargo_cli/Cargo.toml index cb824b41428..a1440dc2ecb 100644 --- a/tooling/nargo_cli/Cargo.toml +++ b/tooling/nargo_cli/Cargo.toml @@ -37,7 +37,7 @@ toml.workspace = true serde.workspace = true serde_json.workspace = true prettytable-rs = "0.10" -rayon = "1.7.0" +rayon = "1.8.0" thiserror.workspace = true tower.workspace = true async-lsp = { version = "0.0.5", default-features = false, features = [ diff --git a/tooling/nargo_cli/tests/gates_report.sh b/tooling/nargo_cli/tests/gates_report.sh index 18688e5110c..3286ff1cdf2 100755 --- a/tooling/nargo_cli/tests/gates_report.sh +++ b/tooling/nargo_cli/tests/gates_report.sh @@ -3,9 +3,6 @@ set -e # These tests are incompatible with gas reporting excluded_dirs=("workspace" "workspace_default_member") -# These tests are temporarily causing issues with gates reporting. -# They should be re-added later. -temp_excluded_dirs=("eddsa") current_dir=$(pwd) base_path="$current_dir/execution_success" @@ -18,7 +15,7 @@ echo "[workspace]" > Nargo.toml echo "members = [" >> Nargo.toml for dir in $test_dirs; do - if [[ " ${excluded_dirs[@]} " =~ " ${dir} " ]] || [[ " ${temp_excluded_dirs[@]} " =~ " ${dir} " ]]; then + if [[ " ${excluded_dirs[@]} " =~ " ${dir} " ]]; then continue fi From 85a5b83e48eab1612d742efcfe3811d1b6d809ad Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 23 Oct 2023 13:16:32 +0100 Subject: [PATCH 3/7] chore: wait until nargo is built to run benchmarking --- .github/workflows/gates_report.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/gates_report.yml b/.github/workflows/gates_report.yml index 715c10d5043..1fa4b2e8828 100644 --- a/.github/workflows/gates_report.yml +++ b/.github/workflows/gates_report.yml @@ -44,6 +44,7 @@ jobs: compare_gas_reports: + needs: [build-nargo] runs-on: ubuntu-latest permissions: pull-requests: write From 8f95ad7b3f94c13aff25b8401985c116bef128a1 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 23 Oct 2023 13:36:46 +0100 Subject: [PATCH 4/7] chore: fix typo --- .github/workflows/gates_report.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/gates_report.yml b/.github/workflows/gates_report.yml index 1fa4b2e8828..4df14b48753 100644 --- a/.github/workflows/gates_report.yml +++ b/.github/workflows/gates_report.yml @@ -70,7 +70,7 @@ jobs: - name: Generate gates report working-directory: ./tooling/nargo_cli/tests - run: ./gas_report.sh + run: ./gates_report.sh - name: Compare gates reports id: gates_diff From 8736d8a6094123d6de0b0d30315f1858c5c5df59 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 23 Oct 2023 13:37:19 +0100 Subject: [PATCH 5/7] chore: remove references to gas --- .github/workflows/gates_report.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/gates_report.yml b/.github/workflows/gates_report.yml index 4df14b48753..97bcfa08e91 100644 --- a/.github/workflows/gates_report.yml +++ b/.github/workflows/gates_report.yml @@ -77,12 +77,12 @@ jobs: uses: TomAFrench/noir-gates-diff@e7cf131b7e7f044c01615f93f0b855f65ddc02d4 with: report: ./tooling/nargo_cli/tests/gates_report.json - summaryQuantile: 0.9 # only display the 10% most significant gas diffs in the summary (defaults to 20%) + 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 gas costs + # delete the comment in case changes no longer impact circuit sizes delete: ${{ !steps.gates_diff.outputs.markdown }} message: ${{ steps.gates_diff.outputs.markdown }} From 019ac5badf6bbd709c4cdf2c7f0beccc132b6798 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 23 Oct 2023 13:59:30 +0100 Subject: [PATCH 6/7] chore: disable eddsa in CI --- tooling/nargo_cli/tests/gates_report.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tooling/nargo_cli/tests/gates_report.sh b/tooling/nargo_cli/tests/gates_report.sh index 3286ff1cdf2..e06e6812e9d 100755 --- a/tooling/nargo_cli/tests/gates_report.sh +++ b/tooling/nargo_cli/tests/gates_report.sh @@ -4,6 +4,9 @@ set -e # These tests are incompatible with gas reporting excluded_dirs=("workspace" "workspace_default_member") +# These tests cause failures in CI with a stack overflow for some reason. +ci_excluded_dirs=("eddsa") + current_dir=$(pwd) base_path="$current_dir/execution_success" test_dirs=$(ls $base_path) @@ -19,6 +22,10 @@ for dir in $test_dirs; do continue fi + if [[ ${CI-false} = "true" ]] && [[ " ${ci_excluded_dirs[@]} " =~ " ${dir} " ]]; then + continue + fi + echo " \"execution_success/$dir\"," >> Nargo.toml done From 06358aab887f6022ed4738a87ae382c8192728f9 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 23 Oct 2023 14:15:36 +0100 Subject: [PATCH 7/7] chore: move gates report to repo root --- .github/workflows/gates_report.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/gates_report.yml b/.github/workflows/gates_report.yml index 97bcfa08e91..e2d6bdd56b7 100644 --- a/.github/workflows/gates_report.yml +++ b/.github/workflows/gates_report.yml @@ -70,13 +70,15 @@ jobs: - name: Generate gates report working-directory: ./tooling/nargo_cli/tests - run: ./gates_report.sh + run: | + ./gates_report.sh + mv gates_report.json ../../../gates_report.json - name: Compare gates reports id: gates_diff uses: TomAFrench/noir-gates-diff@e7cf131b7e7f044c01615f93f0b855f65ddc02d4 with: - report: ./tooling/nargo_cli/tests/gates_report.json + 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