feat: optimize brillig `ToRadix` for common radices #7365

TomAFrench · 2025-02-13T00:33:50Z

Description

Problem*

Resolves

Summary*

For a number of radices (notably 2 and 256) we can avoid the usage of BigUint and just decompose the field into bytes and do a few bitwise operations. I'd expect this to significantly outperform the fully general implementation.

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.

TomAFrench · 2025-02-13T11:53:22Z

I'd like to add some benchmarks for this to check performance.

* master: chore: avoid u128s in brillig memory (#7363) chore: update docs about integer overflows (#7370) fix!: Only decrement the counter of an array if its address has not changed (#7297) fix: let LSP read `noirfmt.toml` for formatting files (#7355) chore: deprecate keccak256 (#7361)

TomAFrench · 2025-02-14T18:55:13Z

This cuts the time to do a field decomposition from 1.9588 µs to 752.21 ns

github-actions

Compilation Time

Benchmark suite	Current: `80f6a01`	Previous: `4aba428`	Ratio
`sha256_regression`	`0.956` s	`0.947` s	`1.01`
`regression_4709`	`0.697` s	`0.681` s	`1.02`
`ram_blowup_regression`	`21.3` s	`21` s	`1.01`
`global_var_regression_entry_points`	`0.514` s	`0.515` s	`1.00`
`private-kernel-inner`	`2.05` s	`2.05` s	`1`
`private-kernel-reset`	`6.432` s	`6.442` s	`1.00`
`private-kernel-tail`	`1.022` s	`1.056` s	`0.97`
`rollup-base-private`	`8.966` s	`8.476` s	`1.06`
`rollup-base-public`	`5.22` s	`5.012` s	`1.04`
`rollup-block-root-empty`	`0.985` s	`0.959` s	`1.03`
`rollup-block-root-single-tx`	`93.2` s	`94.1` s	`0.99`
`rollup-block-root`	`90.5` s	`95.7` s	`0.95`
`rollup-merge`	`0.943` s	`0.942` s	`1.00`
`rollup-root`	`1.518` s	`1.48` s	`1.03`

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

github-actions

Execution Time

Benchmark suite	Current: `80f6a01`	Previous: `4aba428`	Ratio
`private-kernel-inner`	`0.07` s	`0.07` s	`1`
`private-kernel-reset`	`0.296` s	`0.294` s	`1.01`
`private-kernel-tail`	`0.017` s	`0.017` s	`1`
`rollup-base-private`	`0.432` s	`0.43` s	`1.00`
`rollup-base-public`	`0.182` s	`0.183` s	`0.99`
`rollup-block-root`	`35.5` s	`34.9` s	`1.02`
`rollup-merge`	`0.006` s	`0.007` s	`0.86`
`rollup-root`	`0.039` s	`0.038` s	`1.03`

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

github-actions

Compilation Memory

Benchmark suite	Current: `80f6a01`	Previous: `4aba428`	Ratio
`private-kernel-inner`	`285.64` MB	`285.59` MB	`1.00`
`private-kernel-reset`	`597.57` MB	`597.57` MB	`1`
`private-kernel-tail`	`209.2` MB	`209.2` MB	`1`
`rollup-base-private`	`955.94` MB	`955.94` MB	`1`
`rollup-base-public`	`828.6` MB	`828.64` MB	`1.00`
`rollup-block-root-empty`	`338.68` MB	`338.68` MB	`1`
`rollup-block-root-single-tx`	`6880` MB	`6880` MB	`1`
`rollup-block-root`	`6880` MB	`6880` MB	`1`
`rollup-merge`	`337.11` MB	`337.11` MB	`1`
`rollup-root`	`384.8` MB	`384.79` MB	`1.00`

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

github-actions

Execution Memory

Benchmark suite	Current: `80f6a01`	Previous: `4aba428`	Ratio
`private-kernel-inner`	`224.77` MB	`224.77` MB	`1`
`private-kernel-reset`	`259.37` MB	`259.37` MB	`1`
`private-kernel-tail`	`194.78` MB	`194.78` MB	`1`
`rollup-base-private`	`441.42` MB	`441.42` MB	`1`
`rollup-base-public`	`382.59` MB	`382.59` MB	`1`
`rollup-block-root`	`1420` MB	`1420` MB	`1`
`rollup-merge`	`321.52` MB	`321.52` MB	`1`
`rollup-root`	`328.9` MB	`328.9` MB	`1`

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

github-actions

Test Suite Duration

Benchmark suite	Current: `80f6a01`	Previous: `4aba428`	Ratio
`AztecProtocol_aztec-packages_noir-projects_aztec-nr`	`38` s	`39` s	`0.97`
`AztecProtocol_aztec-packages_noir-projects_noir-contracts`	`79` s	`77` s	`1.03`
`AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob`	`63` s	`63` s	`1`
`AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib`	`164` s	`162` s	`1.01`
`AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_reset-kernel-lib`	`11` s	`11` s	`1`
`AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib`	`165` s	`163` s	`1.01`
`AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types`	`55` s	`54` s	`1.02`
`noir-lang_noir-bignum_`	`337` s	`357` s	`0.94`
`noir-lang_noir_bigcurve_`	`272` s	`290` s	`0.94`
`noir-lang_noir_json_parser_`	`10` s	`9` s	`1.11`

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

acvm-repo/brillig_vm/src/black_box.rs

jfecher · 2025-02-18T17:29:26Z

acvm-repo/brillig_vm/benches/criterion.rs

+
+use std::time::Duration;
+
+fn byte_decomposition(c: &mut Criterion) {


Are these benchmarks meant to be temporary or permanent?

I think we can have them be permanent.

acvm-repo/brillig_vm/benches/criterion.rs

Co-authored-by: jfecher <[email protected]>

aakoshh · 2025-02-24T14:57:31Z

acvm-repo/brillig_vm/src/black_box.rs

+    // TODO: raise an error if we cannot fit `input` into `num_limbs`.
+    match radix {
+        256 => {
+            let bytes_le = input.to_be_bytes().into_iter().rev();


We should have to_le_bytes() now.

Alternatively instead of .take(num_limbs) followed by .rev() you could try:

let bytes_be = input.to_be_bytes(); let lowest_bytes_be = bytes_be.skip(num_limbs - bytes_be.len());

feat: optimize brillig ToRadix for common radices

0d258b1

TomAFrench marked this pull request as ready for review February 13, 2025 00:35

TomAFrench requested a review from a team February 13, 2025 00:35

chore: cursed iterators

abaca14

TomAFrench marked this pull request as draft February 13, 2025 01:09

.

dc79f46

TomAFrench and others added 3 commits February 13, 2025 18:48

Merge branch 'master' into tf/optimize-brillig-radix

a899689

chore: add some basic brillig VM criterion benchmarks

02dcc1a

chore: add benchmark for field byte decomposition

ffb3786

TomAFrench marked this pull request as ready for review February 14, 2025 19:02

TomAFrench and others added 2 commits February 14, 2025 19:02

Merge branch 'master' into tf/optimize-brillig-radix

f318fa0

.

7e2888c

TomAFrench added the bench-show Display benchmark results on PR label Feb 14, 2025

Merge branch 'master' into tf/optimize-brillig-radix

cc7079a

github-actions bot reviewed Feb 14, 2025

View reviewed changes

jfecher reviewed Feb 18, 2025

View reviewed changes

TomAFrench and others added 6 commits February 18, 2025 17:35

Update acvm-repo/brillig_vm/benches/criterion.rs

527d39b

Co-authored-by: jfecher <[email protected]>

.

692d232

chore: break up iterator chains

2a901a9

chore: refactor for better testing

cf571bd

.

a18667a

.

f8e3bff

TomAFrench requested a review from a team February 21, 2025 20:14

TomAFrench enabled auto-merge February 21, 2025 20:36

Merge branch 'master' into tf/optimize-brillig-radix

80f6a01

aakoshh reviewed Feb 24, 2025

View reviewed changes

TomAFrench marked this pull request as draft February 25, 2025 11:00

auto-merge was automatically disabled February 25, 2025 11:00
Pull request was converted to draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: optimize brillig `ToRadix` for common radices #7365

feat: optimize brillig `ToRadix` for common radices #7365

TomAFrench commented Feb 13, 2025

TomAFrench commented Feb 13, 2025

TomAFrench commented Feb 14, 2025

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

jfecher Feb 18, 2025

TomAFrench Feb 18, 2025

aakoshh Feb 24, 2025

aakoshh Feb 24, 2025


		use std::time::Duration;

		fn byte_decomposition(c: &mut Criterion) {

feat: optimize brillig ToRadix for common radices #7365

Are you sure you want to change the base?

feat: optimize brillig ToRadix for common radices #7365

Conversation

TomAFrench commented Feb 13, 2025

Description

Problem*

Summary*

Additional Context

Documentation*

PR Checklist*

TomAFrench commented Feb 13, 2025

TomAFrench commented Feb 14, 2025

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Compilation Time

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Execution Time

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Compilation Memory

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Execution Memory

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Test Suite Duration

jfecher Feb 18, 2025

Choose a reason for hiding this comment

TomAFrench Feb 18, 2025

Choose a reason for hiding this comment

aakoshh Feb 24, 2025

Choose a reason for hiding this comment

aakoshh Feb 24, 2025

Choose a reason for hiding this comment

feat: optimize brillig `ToRadix` for common radices #7365

feat: optimize brillig `ToRadix` for common radices #7365

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot left a comment •

edited

Loading