From 39f280ba1736e075f9fd0c41eb38e289b1299c8b Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 5 Jan 2024 09:59:22 +0100 Subject: [PATCH 1/7] change: Move from `maybe_rayon` to `rayon-1.8` After seeing the issues with WASM that the last release (`0.5.0`) of this crate made, the idea is to now control the compatibility with WASM while at the same time, making it easy to handle. In this line of work, the idea was to simply do the following: - Use `rayon 1.8` as a dependency for paralellism which fixes the WASM compat issues with `multicore`-related things. See: https://github.com/rayon-rs/rayon/pull/1019 thanks @han0110 for the suggestion. - Use conditional dev-dependency loading for `getrandom` such that we can compile the lib for WASM-targets in the CI without needing to have the dependency being pulled downstream. - The `multicore` module is gone, and the rest of the code has been refactored since the "fallback" is now handled by rayon itself. --- Cargo.toml | 10 +++++++--- src/fft.rs | 5 ++--- src/lib.rs | 1 - src/msm.rs | 15 +++++---------- src/multicore.rs | 16 ---------------- 5 files changed, 14 insertions(+), 33 deletions(-) delete mode 100644 src/multicore.rs diff --git a/Cargo.toml b/Cargo.toml index 1ce899fb..a452b187 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,11 @@ serde_json = "1.0.105" hex = "0.4" rand_chacha = "0.3.1" +# Added to make sure we are able to build the lib in the CI. +# Notice this will never be loaded for someone using this lib as dep. +[target.'cfg(all(target_arch = "wasm32", target_os = "unknown"))'.dev-dependencies] +getrandom = { version = "0.2", features = ["js"] } + [dependencies] subtle = "2.4" ff = { version = "0.13.0", default-features = false, features = ["std"] } @@ -35,11 +40,10 @@ serde = { version = "1.0", default-features = false, optional = true } serde_arrays = { version = "0.1.0", optional = true } hex = { version = "0.4", optional = true, default-features = false, features = ["alloc", "serde"] } blake2b_simd = "1" -maybe-rayon = { version = "0.1.0", default-features = false } +rayon = "1.8" [features] -default = ["bits", "multicore"] -multicore = ["maybe-rayon/threads"] +default = ["bits"] asm = [] bits = ["ff/bits"] bn256-table = [] diff --git a/src/fft.rs b/src/fft.rs index 6eb3487e..00eca39a 100644 --- a/src/fft.rs +++ b/src/fft.rs @@ -1,4 +1,3 @@ -use crate::multicore; pub use crate::{CurveAffine, CurveExt}; use ff::Field; use group::{GroupOpsOwned, ScalarMulOwned}; @@ -38,7 +37,7 @@ pub fn best_fft>(a: &mut [G], omega: Scalar, r } - let threads = multicore::current_num_threads(); + let threads = rayon::current_num_threads(); let log_threads = threads.ilog2(); let n = a.len(); assert_eq!(n, 1 << log_n); @@ -107,7 +106,7 @@ pub fn recursive_butterfly_arithmetic>( a[1] -= &t; } else { let (left, right) = a.split_at_mut(n / 2); - multicore::join( + rayon::join( || recursive_butterfly_arithmetic(left, n / 2, twiddle_chunk * 2, twiddles), || recursive_butterfly_arithmetic(right, n / 2, twiddle_chunk * 2, twiddles), ); diff --git a/src/lib.rs b/src/lib.rs index b28ed89e..cba3af3a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3,7 +3,6 @@ pub mod ff_ext; pub mod fft; pub mod hash_to_curve; pub mod msm; -pub mod multicore; pub mod serde; pub mod bn256; diff --git a/src/msm.rs b/src/msm.rs index 1a3709c1..7ac24655 100644 --- a/src/msm.rs +++ b/src/msm.rs @@ -4,8 +4,6 @@ use ff::PrimeField; use group::Group; use pasta_curves::arithmetic::CurveAffine; -use crate::multicore; - fn get_booth_index(window_index: usize, window_size: usize, el: &[u8]) -> i32 { // Booth encoding: // * step by `window` size @@ -155,12 +153,12 @@ pub fn small_multiexp(coeffs: &[C::Scalar], bases: &[C]) -> C::C pub fn best_multiexp(coeffs: &[C::Scalar], bases: &[C]) -> C::Curve { assert_eq!(coeffs.len(), bases.len()); - let num_threads = multicore::current_num_threads(); + let num_threads = rayon::current_num_threads(); if coeffs.len() > num_threads { let chunk = coeffs.len() / num_threads; let num_chunks = coeffs.chunks(chunk).len(); let mut results = vec![C::Curve::identity(); num_chunks]; - multicore::scope(|scope| { + rayon::scope(|scope| { let chunk = coeffs.len() / num_threads; for ((coeffs, bases), acc) in coeffs @@ -186,10 +184,7 @@ mod test { use std::ops::Neg; - use crate::{ - bn256::{Fr, G1Affine, G1}, - multicore, - }; + use crate::bn256::{Fr, G1Affine, G1}; use ark_std::{end_timer, start_timer}; use ff::{Field, PrimeField}; use group::{Curve, Group}; @@ -200,12 +195,12 @@ mod test { fn best_multiexp(coeffs: &[C::Scalar], bases: &[C]) -> C::Curve { assert_eq!(coeffs.len(), bases.len()); - let num_threads = multicore::current_num_threads(); + let num_threads = rayon::current_num_threads(); if coeffs.len() > num_threads { let chunk = coeffs.len() / num_threads; let num_chunks = coeffs.chunks(chunk).len(); let mut results = vec![C::Curve::identity(); num_chunks]; - multicore::scope(|scope| { + rayon::scope(|scope| { let chunk = coeffs.len() / num_threads; for ((coeffs, bases), acc) in coeffs diff --git a/src/multicore.rs b/src/multicore.rs deleted file mode 100644 index d8323553..00000000 --- a/src/multicore.rs +++ /dev/null @@ -1,16 +0,0 @@ -pub use maybe_rayon::{ - iter::{IntoParallelIterator, IntoParallelRefMutIterator, ParallelIterator}, - join, scope, Scope, -}; - -#[cfg(feature = "multicore")] -pub use maybe_rayon::{ - current_num_threads, - iter::{IndexedParallelIterator, IntoParallelRefIterator}, - slice::ParallelSliceMut, -}; - -#[cfg(not(feature = "multicore"))] -pub fn current_num_threads() -> usize { - 1 -} From deca3dce6b56de05fd464699627968452c9bab26 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 5 Jan 2024 10:14:30 +0100 Subject: [PATCH 2/7] change: Update CI to account for WASM compat --- .github/workflows/ci.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 646f40ca..b1881293 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,6 @@ env: concurrency: group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} cancel-in-progress: true - jobs: test: @@ -42,8 +41,10 @@ jobs: strategy: matrix: include: - - feature: - feature: default + target: + - wasm32-unknown-unknown + - wasm32-wasi steps: - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 @@ -54,6 +55,10 @@ jobs: # This build will be reused by nextest, # and also checks (--all-targets) that benches don't bit-rot run: cargo build --release --all-targets --no-default-features --features "${{ matrix.feature }}" + # We run WASM build (for tests) which compiles the lib allowig us to have + # `getrandom` as a dev-dependency. + - name: Build (WASM compat) + run: cargi build --tests --release --no-default-features --features "${{ matrix.feature }} - name: Test run: | cargo nextest run --profile ci --release --workspace --no-default-features --features "${{ matrix.feature }}" From 2588674e2a79e19cb03f64666ce7dbba5a5ad6ba Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 5 Jan 2024 10:19:58 +0100 Subject: [PATCH 3/7] chore: Add paralellism section to README --- README.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/README.md b/README.md index e7385218..e2ea7bf3 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,19 @@ The implementations were originally ported from [matterlabs/pairing](https://git * Various features related to serialization and deserialization of curve points and field elements. * Curve-specific optimizations and benchmarking capabilities. +## Controlling parallelism + +`halo2curves` currently uses [rayon](https://github.com/rayon-rs/rayon) for parallel +computation. + +The `RAYON_NUM_THREADS` environment variable can be used to set the number of +threads. + +When compiling to WASM-targets, notice that since version `1.7`, `rayon` will fallback automatically (with no need to handle features) to require `getrandom` in order to be able to work. +For more info related to WASM-compilation. + +See: [Rayon: Usage with WebAssembly](https://github.com/rayon-rs/rayon#usage-with-webassembly) for more info. + ## Benchmarks Benchmarking is supported through the use of Rust's built-in test framework. Benchmarks can be run without assembly optimizations: From a96d02e69f791bdfaee6a681aae0836e4152a528 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 5 Jan 2024 10:21:11 +0100 Subject: [PATCH 4/7] chore: Fix CI missing " --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b1881293..a1ca9e27 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -58,7 +58,7 @@ jobs: # We run WASM build (for tests) which compiles the lib allowig us to have # `getrandom` as a dev-dependency. - name: Build (WASM compat) - run: cargi build --tests --release --no-default-features --features "${{ matrix.feature }} + run: cargo build --tests --release --no-default-features --features "${{ matrix.feature }}" - name: Test run: | cargo nextest run --profile ci --release --workspace --no-default-features --features "${{ matrix.feature }}" From c5acdafea008745b513f05dd169f72da71c28a22 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 5 Jan 2024 12:05:24 +0100 Subject: [PATCH 5/7] chore: Split WASM build & add targets --- .github/workflows/ci.yml | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a1ca9e27..0340f5f5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,20 +34,36 @@ concurrency: cancel-in-progress: true jobs: - test: + compat: if: github.event.pull_request.draft == false name: Test runs-on: ubuntu-latest strategy: matrix: - include: - - feature: default target: - wasm32-unknown-unknown - wasm32-wasi steps: - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 + + - name: Download WASM targets + run: rustup target add "${{ matrix.target }}" + # We run WASM build (for tests) which compiles the lib allowig us to have + # `getrandom` as a dev-dependency. + - name: Build (WASM compat) + run: cargo build --tests --release --features "bn256-table derive_serde prefetch" --target "${{ matrix.target }}" + test: + if: github.event.pull_request.draft == false + name: Test + runs-on: ubuntu-latest + strategy: + matrix: + include: + - feature: default + steps: + - uses: actions/checkout@v2 + - uses: actions-rs/toolchain@v1 # use the more efficient nextest - uses: taiki-e/install-action@nextest - uses: Swatinem/rust-cache@v2 @@ -55,10 +71,6 @@ jobs: # This build will be reused by nextest, # and also checks (--all-targets) that benches don't bit-rot run: cargo build --release --all-targets --no-default-features --features "${{ matrix.feature }}" - # We run WASM build (for tests) which compiles the lib allowig us to have - # `getrandom` as a dev-dependency. - - name: Build (WASM compat) - run: cargo build --tests --release --no-default-features --features "${{ matrix.feature }}" - name: Test run: | cargo nextest run --profile ci --release --workspace --no-default-features --features "${{ matrix.feature }}" From 9189a10afae4aa90d171d886013b8a35048332c4 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 5 Jan 2024 12:32:57 +0100 Subject: [PATCH 6/7] chore: Test all features for regular-target builds --- .github/workflows/ci.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0340f5f5..7f167fe7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,6 +43,10 @@ jobs: target: - wasm32-unknown-unknown - wasm32-wasi + include: + - feature: default + - feature: bn256-table + - feature: derive_serde steps: - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 @@ -61,6 +65,9 @@ jobs: matrix: include: - feature: default + - feature: bn256-table + - feature: derive_serde + - feature: asm steps: - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 From 3f58eff4cf11bebe74450e22d8043733740c8097 Mon Sep 17 00:00:00 2001 From: CPerezz Date: Fri, 5 Jan 2024 17:35:15 +0100 Subject: [PATCH 7/7] chore: Address review nits --- .github/workflows/ci.yml | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7f167fe7..e9875350 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,17 +36,13 @@ concurrency: jobs: compat: if: github.event.pull_request.draft == false - name: Test + name: Wasm-compatibility runs-on: ubuntu-latest strategy: matrix: target: - wasm32-unknown-unknown - wasm32-wasi - include: - - feature: default - - feature: bn256-table - - feature: derive_serde steps: - uses: actions/checkout@v2 - uses: actions-rs/toolchain@v1 @@ -55,7 +51,7 @@ jobs: run: rustup target add "${{ matrix.target }}" # We run WASM build (for tests) which compiles the lib allowig us to have # `getrandom` as a dev-dependency. - - name: Build (WASM compat) + - name: Build run: cargo build --tests --release --features "bn256-table derive_serde prefetch" --target "${{ matrix.target }}" test: if: github.event.pull_request.draft == false