feat: using plonky3 maybe-rayon instead of rayon for optional parallelism

[Sahilgill24]

Motivation

solves #4

Description

Using plonky3 maybe-rayon crate as it provides same api for parallel and non-parallel case, thus helping to reduce the usage of the parallel flag.

Progress

rayon was replaced by p3-maybe-rayon currently in the following crates:-

• witness
• multilinear-extensions
• mpcs
• whir

For the whir crate all the imports can be replaced with the p3::maybe_rayon::prelude::* import, but as there are different variations of functions for the parallel and the non-parallel case for ex:- here so the removal of the #[cfg(feature = "parallel")] is a bit tricky here and might require some changes in the core functions.

[Sahilgill24]

Hey @hero78119

I have updated some crates, could you take a look at the whir crate related issue I mentioned above and also review the Draft once.

[hero78119]

Hey @hero78119

I have updated some crates, could you take a look at the whir crate related issue I mentioned above and also review the Draft once.

Hi @Sahilgill24 had a quick go-through and everything looks pretty neat to me! For WHIR what I suggested this PR can just cover change to use maybe-rayon on the place which both sequential/parallel share "almost" identical implementation. For some core function change, it will be nice to left to new issue + PR for handling :)

[Sahilgill24]

Thanks alot for the review @hero78119 .

One last Query, In the trailing issue you had also mentioned not changing the dependancies for the sumcheck crate , should I do the changes for that crate as well.

I would also love to contribute more to new issues if there are any intermediate or beginner friendly ones.

[hero78119]

One last Query, In the trailing issue you had also mentioned not changing the dependancies for the sumcheck crate , should I do the changes for that crate as well.

I would also love to contribute more to new issues if there are any intermediate or beginner friendly ones.

At that moment we are working on sumcheck refactoring thus suggest not touch this crate.
Now everything ready and feel free to also cover sumcheck crate 😄

Thanks for the interests and will try mark few more issues to be good-start-first :)

[Sahilgill24]

Thanks alot @hero78119 , marking this ready for review.

[hero78119]

Hi, it seems to be some left-over (I checked via search the places where in use of "rayon" but no "maybe-rayon")

E.g.

gkr-backend/crates/sumcheck/src/prover.rs

Lines 13 to 16 in a53d834

	use rayon::{
	iter::{IndexedParallelIterator, IntoParallelRefIterator, IntoParallelRefMutIterator},
	prelude::{IntoParallelIterator, ParallelIterator},
	};

And probably so as other place. Would suggest to have a overall check and see whether other place got similar :)

Another place, as ceno are the major dependency of this repo, it will be nice if you can make a separate PR on ceno and point to your p3-maybe-rayon feature branch and see whether any broken during compiler & integration test.

[Sahilgill24]

And probably so as other place. Would suggest to have a overall check and see whether other place got similar :)

Hey @hero78119 , I checked this once, for the sumcheck crate as you had mentioned that it was currently under progress, so I thought not to change it. It is otherwise a trivial change and I can do it.

fixed the whir crate imports along with fixing the linting issues. (which i should have checked earlier, mb)

Another place, as ceno are the major dependency of this repo, it will be nice if you can make a separate PR on ceno and point to your p3-maybe-rayon feature branch and see whether any broken during compiler & integration test.

Surely, will do this :)

[Sahilgill24]

Hey @hero78119 , I fixed the use of all the unnecessary flags and the linting/formatting issues.

Also this could be added to the test documentation, for any other users who might want to check the tests and formatting related stuff, all these commands mirror the CI/CD pipeline.

cargo make tests
cargo make clippy 
cargo fmt --all --check 
taplo fmt --check --diff 

[hero78119]

Awesome job and all looks good to me!

I think one left over is testing with https://github.com/scroll-tech/ceno/

1. assure ceno benchmark no impact, as now parallel flag is optional
2. on test environment, unittest failed not involve messy "rayon" stacktrace => this is the major purpose of this PR to enable clean stacktrace on unittest, better for message debugging

Will be nice if there is a separate Ceno PR to test above, or I will verify it later before we proceed the merge :)

[Sahilgill24]

Thanks for the review @hero78119 .

I think one left over is testing with https://github.com/scroll-tech/ceno/

1. assure ceno benchmark no impact, as now parallel flag is optional
2. on test environment, unittest failed not involve messy "rayon" stacktrace => this is the major purpose of this PR to enable clean stacktrace on unittest, better for message debugging

I shall make a separate PR on the Ceno repo for these additional tests and have also updated the branch to match the main.

[hero78119]

Hi, I reviewed again and think it might need some cosmetics of this PR
I make them into a new commit hero78119@f18ce71 and feel free to cherry-pick

The major adjustment is for sumcheck crate, some style, e.g. with_min_len(), par_iter().chunks(), ... for those api are only in rayon version, so I just remove/rewrite them into the supported api from maybe-rayon

Besides, also refactor whir crate under "whir" features of mpcs. Per discussion earlier in this PR discussion thread, whir now only work on rayon mode and switch to maybe-rayon need some refactor.

[Sahilgill24]

hey @hero78119 , really sorry for the delay :(

I have made the changes from your commit onto this branch.

Just one query in the mpcs crate on adding "whir" as a feature in this crate, the 'rust analyzer' VS code extension shows wrong imports for use whir_external imports in mpcs/src/whir.rs file due to the feature flag, but the code is compiling properly. On adding whir as a default feature this behaviour from the extension stops .

[hero78119]

Hi, as currently Ceno upstream are in benchmark phase with few big features queueing to ship into master branch sooner, so we will onhold this development-tool related PR a bit and get back to this sooner :)

[Sahilgill24]

Surely, thanks for the update :) .

@hero78119 , any updates for this PR
thanks :)

[hero78119]

Hi @Sahilgill24 could you update the PR and resolve conflicts? We finish some big features refactor and now this is good to go

[Sahilgill24]

Resolved the conflicts and the only change that needed to be done was in the sumcheck/src/prover.rs to change the use of the reduce function from the classic rayon to its counterpart in the maybe-rayon.

let evaluations = self
                .workers_states
                .par_iter_mut()
                .map(|state| state.run_round())
                .reduce(|| AdditiveVec::new(max_degree), |a, b| a + b);

to

let evaluations = self
                .workers_states
                .par_iter_mut()
                .map(|state| state.run_round())
                .par_fold_reduce(
                    || AdditiveVec::new(max_degree),
                    |acc, item| acc + item,
                    |a, b| a + b,
                );

[hero78119]

Hi, feel free to cherry-pick commit hero78119@257df73 and resolve the lint error