Skip to content

Commit 6440e80

Browse files
vezenovmTomAFrench
andauthored
chore: Add Brillig loop bytecode size regression and update noir-gates-diff report (#5747)
# Description ## Problem\* Resolves <!-- Link to GitHub Issue --> ## Summary\* Successful example report: <img width="964" alt="Screenshot 2024-08-19 at 9 31 35 AM" src="https://github.com/user-attachments/assets/ccbddccd-a38a-4853-bcc2-1ac6bcb1fe36"> I originally just had this PR as a draft to test and close, but we can just keep the PR now to add a size regression test for issue #4535: ``` struct EnumEmulation { a: Option<Field>, b: Option<Field>, c: Option<Field>, } unconstrained fn main() -> pub Field { let mut emulated_enum = EnumEmulation { a: Option::some(1), b: Option::none(), c: Option::none() }; assert_eq(emulated_enum.a.unwrap(), 1); emulated_enum.a = Option::some(2); emulated_enum.a.unwrap() } ``` This PR also provides a quicker way of updating the noir-gates-diff commit as the original PR (#5745) will first search for a report on master where a Brillig report does not exist. On this branch we have a reference report on `mv/brillig-opcode-report`. I think we could merge `mv/brillig-opcode-report` into master and then any more commit hash updates for the `noir-gates-diff` repo can be made on this PR. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
1 parent d4e2f0a commit 6440e80

File tree

9 files changed

+158
-8
lines changed

9 files changed

+158
-8
lines changed

.github/workflows/gates_report.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ jobs:
8080
8181
- name: Compare gates reports
8282
id: gates_diff
83-
uses: vezenovm/noir-gates-diff@acf12797860f237117e15c0d6e08d64253af52b6
83+
uses: noir-lang/noir-gates-diff@1931aaaa848a1a009363d6115293f7b7fc72bb87
8484
with:
8585
report: gates_report.json
8686
summaryQuantile: 0.9 # only display the 10% most significant circuit size diffs in the summary (defaults to 20%)
+92
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
name: Report Brillig bytecode size diff
2+
3+
on:
4+
push:
5+
branches:
6+
- master
7+
pull_request:
8+
9+
jobs:
10+
build-nargo:
11+
runs-on: ubuntu-latest
12+
strategy:
13+
matrix:
14+
target: [x86_64-unknown-linux-gnu]
15+
16+
steps:
17+
- name: Checkout Noir repo
18+
uses: actions/checkout@v4
19+
20+
- name: Setup toolchain
21+
uses: dtolnay/rust-toolchain@1.74.1
22+
23+
- uses: Swatinem/rust-cache@v2
24+
with:
25+
key: ${{ matrix.target }}
26+
cache-on-failure: true
27+
save-if: ${{ github.event_name != 'merge_group' }}
28+
29+
- name: Build Nargo
30+
run: cargo build --package nargo_cli --release
31+
32+
- name: Package artifacts
33+
run: |
34+
mkdir dist
35+
cp ./target/release/nargo ./dist/nargo
36+
7z a -ttar -so -an ./dist/* | 7z a -si ./nargo-x86_64-unknown-linux-gnu.tar.gz
37+
38+
- name: Upload artifact
39+
uses: actions/upload-artifact@v4
40+
with:
41+
name: nargo
42+
path: ./dist/*
43+
retention-days: 3
44+
45+
compare_brillig_bytecode_size_reports:
46+
needs: [build-nargo]
47+
runs-on: ubuntu-latest
48+
permissions:
49+
pull-requests: write
50+
51+
steps:
52+
- uses: actions/checkout@v4
53+
54+
- name: Download nargo binary
55+
uses: actions/download-artifact@v4
56+
with:
57+
name: nargo
58+
path: ./nargo
59+
60+
- name: Set nargo on PATH
61+
run: |
62+
nargo_binary="${{ github.workspace }}/nargo/nargo"
63+
chmod +x $nargo_binary
64+
echo "$(dirname $nargo_binary)" >> $GITHUB_PATH
65+
export PATH="$PATH:$(dirname $nargo_binary)"
66+
nargo -V
67+
68+
- name: Generate Brillig bytecode size report
69+
working-directory: ./test_programs
70+
run: |
71+
chmod +x gates_report_brillig.sh
72+
./gates_report_brillig.sh
73+
mv gates_report_brillig.json ../gates_report_brillig.json
74+
75+
- name: Compare Brillig bytecode size reports
76+
id: brillig_bytecode_diff
77+
uses: noir-lang/noir-gates-diff@3fb844067b25d1b59727ea600b614503b33503f4
78+
with:
79+
report: gates_report_brillig.json
80+
header: |
81+
# Changes to Brillig bytecode sizes
82+
brillig_report: true
83+
summaryQuantile: 0.9 # only display the 10% most significant bytecode size diffs in the summary (defaults to 20%)
84+
85+
- name: Add bytecode size diff to sticky comment
86+
if: github.event_name == 'pull_request' || github.event_name == 'pull_request_target'
87+
uses: marocchino/sticky-pull-request-comment@v2
88+
with:
89+
header: brillig
90+
# delete the comment in case changes no longer impact brillig bytecode sizes
91+
delete: ${{ !steps.brillig_bytecode_diff.outputs.markdown }}
92+
message: ${{ steps.brillig_bytecode_diff.outputs.markdown }}

.gitignore

+1
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ tooling/noir_js/lib
3333
!compiler/wasm/noir-script/target
3434

3535
gates_report.json
36+
gates_report_brillig.json
3637

3738
# Github Actions scratch space
3839
# This gives a location to download artifacts into the repository in CI without making git dirty.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
[package]
2+
name = "brillig_loop_size_regression"
3+
type = "bin"
4+
authors = [""]
5+
compiler_version = ">=0.33.0"
6+
7+
[dependencies]

test_programs/execution_success/brillig_loop_size_regression/Prover.toml

Whitespace-only changes.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
struct EnumEmulation {
2+
a: Option<Field>,
3+
b: Option<Field>,
4+
c: Option<Field>,
5+
}
6+
7+
unconstrained fn main() -> pub Field {
8+
let mut emulated_enum = EnumEmulation { a: Option::some(1), b: Option::none(), c: Option::none() };
9+
10+
for _ in 0..1 {
11+
assert_eq(emulated_enum.a.unwrap(), 1);
12+
}
13+
14+
emulated_enum.a = Option::some(2);
15+
emulated_enum.a.unwrap()
16+
}

test_programs/gates_report_brillig.sh

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
#!/usr/bin/env bash
2+
set -e
3+
4+
# These tests are incompatible with gas reporting
5+
excluded_dirs=("workspace" "workspace_default_member" "double_verify_nested_proof" "overlapping_dep_and_mod" "comptime_println")
6+
7+
current_dir=$(pwd)
8+
base_path="$current_dir/execution_success"
9+
test_dirs=$(ls $base_path)
10+
11+
# We generate a Noir workspace which contains all of the test cases
12+
# This allows us to generate a gates report using `nargo info` for all of them at once.
13+
14+
echo "[workspace]" > Nargo.toml
15+
echo "members = [" >> Nargo.toml
16+
17+
for dir in $test_dirs; do
18+
if [[ " ${excluded_dirs[@]} " =~ " ${dir} " ]]; then
19+
continue
20+
fi
21+
22+
if [[ ${CI-false} = "true" ]] && [[ " ${ci_excluded_dirs[@]} " =~ " ${dir} " ]]; then
23+
continue
24+
fi
25+
26+
echo " \"execution_success/$dir\"," >> Nargo.toml
27+
done
28+
29+
echo "]" >> Nargo.toml
30+
31+
nargo info --force-brillig --json > gates_report_brillig.json
32+
33+
rm Nargo.toml

tooling/nargo_cli/build.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ fn generate_compile_success_empty_tests(test_file: &mut File, test_data_dir: &Pa
207207
let json: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap_or_else(|e| {{
208208
panic!("JSON was not well-formatted {:?}\n\n{:?}", e, std::str::from_utf8(&output.stdout))
209209
}});
210-
let num_opcodes = &json["programs"][0]["functions"][0]["acir_opcodes"];
210+
let num_opcodes = &json["programs"][0]["functions"][0]["opcodes"];
211211
assert_eq!(num_opcodes.as_u64().expect("number of opcodes should fit in a u64"), 0);
212212
"#;
213213

tooling/nargo_cli/src/cli/info_cmd.rs

+7-6
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,7 @@ struct ProgramInfo {
175175
#[serde(skip)]
176176
expression_width: ExpressionWidth,
177177
functions: Vec<FunctionInfo>,
178+
#[serde(skip)]
178179
unconstrained_functions_opcodes: usize,
179180
unconstrained_functions: Vec<FunctionInfo>,
180181
}
@@ -186,7 +187,7 @@ impl From<ProgramInfo> for Vec<Row> {
186187
Fm->format!("{}", program_info.package_name),
187188
Fc->format!("{}", function.name),
188189
format!("{:?}", program_info.expression_width),
189-
Fc->format!("{}", function.acir_opcodes),
190+
Fc->format!("{}", function.opcodes),
190191
Fc->format!("{}", program_info.unconstrained_functions_opcodes),
191192
]
192193
});
@@ -196,7 +197,7 @@ impl From<ProgramInfo> for Vec<Row> {
196197
Fc->format!("{}", function.name),
197198
format!("N/A", ),
198199
Fc->format!("N/A"),
199-
Fc->format!("{}", function.acir_opcodes),
200+
Fc->format!("{}", function.opcodes),
200201
]
201202
}));
202203
main
@@ -215,7 +216,7 @@ struct ContractInfo {
215216
#[derive(Debug, Serialize)]
216217
struct FunctionInfo {
217218
name: String,
218-
acir_opcodes: usize,
219+
opcodes: usize,
219220
}
220221

221222
impl From<ContractInfo> for Vec<Row> {
@@ -225,7 +226,7 @@ impl From<ContractInfo> for Vec<Row> {
225226
Fm->format!("{}", contract_info.name),
226227
Fc->format!("{}", function.name),
227228
format!("{:?}", contract_info.expression_width),
228-
Fc->format!("{}", function.acir_opcodes),
229+
Fc->format!("{}", function.opcodes),
229230
]
230231
})
231232
}
@@ -243,7 +244,7 @@ fn count_opcodes_and_gates_in_program(
243244
.enumerate()
244245
.map(|(i, function)| FunctionInfo {
245246
name: compiled_program.names[i].clone(),
246-
acir_opcodes: function.opcodes.len(),
247+
opcodes: function.opcodes.len(),
247248
})
248249
.collect();
249250

@@ -264,7 +265,7 @@ fn count_opcodes_and_gates_in_program(
264265
.clone()
265266
.iter()
266267
.zip(opcodes_len)
267-
.map(|(name, len)| FunctionInfo { name: name.clone(), acir_opcodes: len })
268+
.map(|(name, len)| FunctionInfo { name: name.clone(), opcodes: len })
268269
.collect();
269270

270271
ProgramInfo {

0 commit comments

Comments
 (0)