Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature "multicore" and you can disable rayon by disabling the "multicore" feature. #728

Merged
merged 6 commits into from
Feb 14, 2023

Conversation

nagatoism
Copy link
Contributor

@nagatoism nagatoism commented Jan 29, 2023

Close #648 All test has been passed with/without rayon.

@nagatoism nagatoism changed the title Closes #648 Add feature "multicore" and you can disable rayon by disabling the "multicore" feature. Add feature "multicore" and you can disable rayon by disabling the "multicore" feature. Jan 30, 2023
Copy link
Contributor

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Only two comments:

  1. I wonder if cfg_if! could abbreviate some of the repeated chunks of code here. Maybe worth looking at it.
  2. I think this doesn't need to go through the nightly feature first. As it doesn't include anything new. But worth checking with @str4d .

For the rest, looks good!

README.md Outdated Show resolved Hide resolved
halo2_proofs/Cargo.toml Outdated Show resolved Hide resolved
halo2_proofs/Cargo.toml Outdated Show resolved Hide resolved
halo2_proofs/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK with suggested changes.

@str4d
Copy link
Contributor

str4d commented Jan 30, 2023

There is a lot of duplicated logic here, and the conditionals are spread out over the codebase. I'd prefer we take a similar approach to bellman and provide fake versions of the rayon types in the halo2_proofs::multicore module that just implement serial logic. Maybe we should move this "maybe rayon" logic into a crate?

And now that I go searching, someone else had the same idea a year ago: https://crates.io/crates/maybe-rayon

Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switch to using the https://crates.io/crates/maybe-rayon crate, I think that the only code changes in this PR should be to halo2_proofs/src/multicore.rs, which would greatly simplify it and make future maintenance simpler.

halo2_proofs/Cargo.toml Outdated Show resolved Hide resolved
halo2_proofs/Cargo.toml Outdated Show resolved Hide resolved
halo2_proofs/src/arithmetic.rs Outdated Show resolved Hide resolved
halo2_proofs/src/arithmetic.rs Outdated Show resolved Hide resolved
halo2_proofs/src/arithmetic.rs Outdated Show resolved Hide resolved
halo2_proofs/src/multicore.rs Outdated Show resolved Hide resolved
halo2_proofs/src/plonk/verifier/batch.rs Outdated Show resolved Hide resolved
halo2_proofs/src/poly/evaluator.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 57a0ecc61a1c0c49c4d118a8afd48bad10807393.

halo2_proofs/Cargo.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
halo2_proofs/README.md Outdated Show resolved Hide resolved
halo2_proofs/src/multicore.rs Outdated Show resolved Hide resolved
halo2_proofs/src/multicore.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please investigate the wasm32-wasi failures. Otherwise this is looking pretty good modulo str4d's comments.

@nagatoism nagatoism requested review from daira and str4d and removed request for daira February 2, 2023 15:57
@nagatoism
Copy link
Contributor Author

Hi Found that IndexedParallelIter is not supported by maybe-rayon. The new code fixes this.

The code is necessary because try_fold rayon and try_fold in rust std are different functions so I cannot reuse code.
Here https://docs.rs/rayon/latest/rayon/iter/trait.ParallelIterator.html#method.fold

PS pls help with the mut on final_msm. No idea why the mut is needed.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK with a proposed refactoring.

@str4d str4d force-pushed the disable-rayon branch 3 times, most recently from c9144b3 to b29306c Compare February 14, 2023 17:50
@str4d
Copy link
Contributor

str4d commented Feb 14, 2023

@str4d
Copy link
Contributor

str4d commented Feb 14, 2023

Force-pushed to address @daira's comment.

@str4d
Copy link
Contributor

str4d commented Feb 14, 2023

CI is currently failing because of an unrelated issue. Fixing that in #734, and then I'll rebase this PR so we can see it fail in the expected way :)

@str4d
Copy link
Contributor

str4d commented Feb 14, 2023

Rebased on main to include #734.

@str4d
Copy link
Contributor

str4d commented Feb 14, 2023

And after another force-push to fix my last commit, the WASM CI test is failing as expected: in addition to the expected compilation issues (due to maybe-rayon falling back to single-threading for non-atomics WASM if threads is enabled) we now see our compiler error that multicore is not supported on non-atomics WASM.

`maybe-rayon` has this case fall back to not using rayon, which while
correct is potentially confusingly non-performant. Developers building
for WASM will have similar things they need to handle themselves; they
can conditionally enable `multicore` there.
Now that `multicore` is a default feature instead of just being always
on, we can ensure it is not enabled just as a result of depending on
`halo2_gadgets` or `halo2`.
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Thanks for the PR @nagatoism!

@str4d str4d merged commit 47f2cc8 into zcash:main Feb 14, 2023
@str4d str4d added this to the 0.3.0 milestone Mar 2, 2023
han0110 added a commit to han0110/halo2 that referenced this pull request Aug 29, 2023
han0110 added a commit to han0110/halo2 that referenced this pull request Sep 4, 2023
han0110 added a commit to privacy-scaling-explorations/halo2 that referenced this pull request Sep 6, 2023
* refactor: add default impl for `SyncDeps` for backward compatability

* feat: pick changes from zcash#728 and changes of flag `test-dev-graph`

* feat: pick changes from zcash#622

* feat: pick changes about mod `circuit` and mod `dev`

* feat: pick rest changes of `halo2_proofs`

* fix: when `--no-default-features`

* ci: sync from upstream, and deduplicate jobs when
push to `main`, and remove always failing job `codecov`.

* fix: make `commit_zk` runnable when `--no-default-features`
jonathanpwang pushed a commit to axiom-crypto/halo2 that referenced this pull request Sep 17, 2023
* feat: expose `transcript_repr` of `VerifyingKey` and reduce the trait constraint (privacy-scaling-explorations#200)

* Synchronize with upstream (privacy-scaling-explorations#199)

* refactor: add default impl for `SyncDeps` for backward compatability

* feat: pick changes from zcash#728 and changes of flag `test-dev-graph`

* feat: pick changes from zcash#622

* feat: pick changes about mod `circuit` and mod `dev`

* feat: pick rest changes of `halo2_proofs`

* fix: when `--no-default-features`

* ci: sync from upstream, and deduplicate jobs when
push to `main`, and remove always failing job `codecov`.

* fix: make `commit_zk` runnable when `--no-default-features`

* fix: put remaining multicore code behind feature flag

* style: run cargo fmt

---------

Co-authored-by: Han <[email protected]>
Velaciela pushed a commit to scroll-tech/halo2 that referenced this pull request Oct 10, 2023
* refactor: add default impl for `SyncDeps` for backward compatability

* feat: pick changes from zcash#728 and changes of flag `test-dev-graph`

* feat: pick changes from zcash#622

* feat: pick changes about mod `circuit` and mod `dev`

* feat: pick rest changes of `halo2_proofs`

* fix: when `--no-default-features`

* ci: sync from upstream, and deduplicate jobs when
push to `main`, and remove always failing job `codecov`.

* fix: make `commit_zk` runnable when `--no-default-features`
kunxian-xia added a commit to scroll-tech/halo2 that referenced this pull request Jan 12, 2024
* feat: call synthesize in `MockProver` multiple times to behave same as real prover

* modify previous commit

* Expose mod `permutation` and re-export `permutation::keygen::Assembly` (privacy-scaling-explorations#149)

* feat: expose mod ule `permutation` and re-export `permutation::keygen::Assembly`

* feat: derive `lone` for `permutation::keygen::Assembly`

* feat: bump MSRV for `inferno`

* change: Migrate workspace to pasta_curves-0.5 (privacy-scaling-explorations#157)

* change: Migrate workspace to pasta_curves-0.5

This ports the majority of the workspace to the `pasta_curves-0.5.0`
leaving some tricky edge-cases that we need to handle carefully.

Resolves: privacy-scaling-explorations#132

* fix: Complete latest trait bounds to compile halo2proofs

* change: Migrate examples & benches to pasta 0.5

* change: Migrate halo2_gadgets to pasta-0.5

* change: Update gadgets outdated code with latest upstream

* fix: Sha3 gadget circuit

* fix: doc tests

* chore: Update merged main

* fix: Apply review suggestions

* fix previous commit

* Extend Circuit trait to take parameters in config (privacy-scaling-explorations#168)

* Extend Circuit trait to take parameters in config

The Circuit trait is extended with the following:
```
pub trait Circuit<F: Field> {
    /// [...]
    type Params: Default;

    fn params(&self) -> Self::Params {
        Self::Params::default()
    }

    fn configure_with_params(meta: &mut ConstraintSystem<F>, params: &Self::Params) -> Self::Config {
        Self::configure(meta)
    }

    fn configure(meta: &mut ConstraintSystem<F>) -> Self::Config;
}
```

This allows runtime parametrization of the circuit configuration.  The extension to the Circuit trait has been designed to minimize the breaking change: existing circuits only need to define the associated `type Params`.

Unfortunately "Associated type defaults" are unstable in Rust, otherwise this would be a non-breaking change.  See rust-lang/rust#29661

* Implement circuit params under feature flag

* Don't overwrite configure method

* Fix doc test

* Allow halo2 constraint names to have non static names (privacy-scaling-explorations#156)

* static ref to String type in Gates, Constraints, VirtualCell, Argument

* 'lookup'.to_string()

* return &str for gate name and constriant_name, also run fmt

* Update halo2_gadgets/Cargo.toml

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

* upgrade rust-toochain

---------

Co-authored-by: Carlos Pérez <[email protected]>
Co-authored-by: Han <[email protected]>

* Improve halo2 query calls (privacy-scaling-explorations#154)

* return expression from cell

* add example

* selector

* recurse Expression to fill in index

* minimized changes from the original

* backword compatible meta.query_X & challange.expr()

* cargo fmt

* fixed lookup to pass all tests

* Update comments

Co-authored-by: Brecht Devos <[email protected]>

* Update comments

Co-authored-by: Brecht Devos <[email protected]>

* Update comments

Co-authored-by: Brecht Devos <[email protected]>

* Update comments

Co-authored-by: Brecht Devos <[email protected]>

* Update comments

Co-authored-by: Brecht Devos <[email protected]>

* Update comments

Co-authored-by: Brecht Devos <[email protected]>

* update

Co-authored-by: Brecht Devos <[email protected]>

* add primitives.rs back

* remove example2

* backward compatible meta.query_X & Column.cur(), next(), prev(), at(usize)

* impl Debug & make side effects only when query.index.is_none()

* change impl Debug for Expression instead & revert test in plonk_api

* upgrade rust-toolchain

* Update halo2_proofs/src/plonk/circuit.rs

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

* Update halo2_proofs/src/plonk/circuit.rs

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

* ran clippy

* Update halo2_proofs/src/plonk/circuit.rs

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

---------

Co-authored-by: Brecht Devos <[email protected]>
Co-authored-by: Han <[email protected]>

* Implement Clone trait for Hash, Absorbing, and Sponge structs (privacy-scaling-explorations#171)

* fix: Fix serialization for VerifyingKey (privacy-scaling-explorations#178)

Now the value returned when the number of selectors is a multiple of 8
is correct.

Resolves: privacy-scaling-explorations#175

* Add more getters to expose internal fields

* add a constructor (privacy-scaling-explorations#164)

* add a constructor

* add more comment

* fix as review

* remove clone

* remove

* no need to use new variable

* change comment

* fix clippy

* rename to from_parts

* remove n declaration

* feat: send sync region (privacy-scaling-explorations#180)

* feat: send / sync region

* Update layout.rs

* update

* lol

* debug

* Update keygen.rs

* Update keygen.rs

* Update keygen.rs

* Update keygen.rs

* thread-safe-region feature flag

* cleanup

* patch dev-graph

* patch non-determinism in mapping creation

* reduce mem usage for vk and pk

* mock proving examples

* swap for hashmap for insertion speed

* reduce update overhead

* replace BTree with Vec

* add benchmarks

* make the benchmarks massive

* patch clippy

* simplify lifetimes

* patch benches

* Update halo2_proofs/src/plonk/permutation/keygen.rs

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

* Update halo2_proofs/examples/vector-mul.rs

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

* rm benches

* order once

* patch lints

---------

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

* fix previous commit

* Fix `parallelize` workload imbalance (privacy-scaling-explorations#186)

* fix parallelize workload imbalance

* remove the need of unsafe

* Updates halo2_curves dependency to released package (privacy-scaling-explorations#190)

THe package release ressets the version from those inherited by the legacy
halo2curves repo's fork history.

The upstream diff is:
https://github.com/privacy-scaling-explorations/halo2curves/compare/9f5c50810bbefe779ee5cf1d852b2fe85dc35d5e..9a7f726fa74c8765bc7cdab11519cf285d169ecf

* fix: explicitly define mds diff type (privacy-scaling-explorations#196)

* fix: explicitly define mds diff type

* rm paren

* feat: expose `transcript_repr` of `VerifyingKey` and reduce the trait constraint (privacy-scaling-explorations#200)

* implement native shuffle argument and api

fix: remove nonsense comment

strictly check shuffle rows

address doc typos

move compression into product commitment

typo

add shuffle errors for `verify_at_rows_par`

dedup expression evaluation

cargo fmt

fix fields in sanity-checks feature

* feat: public cells to allow for implementations of custom `Layouter`  (privacy-scaling-explorations#192)

* feat: public cells

* Update mds.rs

* Update mds.rs

* Update single_pass.rs

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

* bump toolchain to resolve errors

* fix clippy errors for CI run

* rustfmt post clippy

* plz let it be the last lint

* patch clippy lints in gadgets

* clippy lints for sha256 bench

* patch halo2proof benches

* Update assigned.rs

* Update halo2_gadgets/src/poseidon/primitives/mds.rs

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

* Update halo2_gadgets/src/poseidon/primitives/mds.rs

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

---------

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

* Synchronize with upstream (privacy-scaling-explorations#199)

* refactor: add default impl for `SyncDeps` for backward compatability

* feat: pick changes from zcash#728 and changes of flag `test-dev-graph`

* feat: pick changes from zcash#622

* feat: pick changes about mod `circuit` and mod `dev`

* feat: pick rest changes of `halo2_proofs`

* fix: when `--no-default-features`

* ci: sync from upstream, and deduplicate jobs when
push to `main`, and remove always failing job `codecov`.

* fix: make `commit_zk` runnable when `--no-default-features`

* chore: Update rust-toolchain to 1.66 for testing  (privacy-scaling-explorations#208)

* chore: Update rust-toolchain to 1.66 for testing

Note that tests will not compile due to the silent MSRV bump in
`blake2b_simd`.

Hence, we need to use `1.66` as toolchain.

Resolves: privacy-scaling-explorations#207

* change: UIpdate MSRVs in Cargo.toml

* fix: clippy (privacy-scaling-explorations#203)

* fix: clippy

* fmt

* fix: Final clippy complains & adjustments

---------

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

* Implement Sum and Product for Expression (privacy-scaling-explorations#209)

* Make it Eq to make it easier for tests

* Implement Sum and Product for Expression

* Make it readable

* chore: update poseidon dependency

* fix: compiling bug with feautes=parallel_syn

* feat(MockProver): replace errors by asserts(privacy-scaling-explorations#150)

* boundary offset lost when resolving conflict

* disable multiphase prover

* Sync halo2 lib 0.4.0 merging (#81)

* Use thread pool for assign_regions (#57)

* feat: use rayon threadpool

* feat: add UT for many subregions

* refact: move common struct out to module level

* refact: reuse common configure code

* fix ci errors

---------

Co-authored-by: kunxian xia <[email protected]>

* Move `env_logger` dependency to dev-depdendencies (only for test). (#69)

* sync ff/group 0.13

* fix clippy

* fix clippy

* fmg

* [FEAT] Upgrading table16 for SHA256 (#73)

* upgrade sha256

* fix clippy

* Bus auto (#72)

* bus: expose global offset of regions

* bus-auto: add query_advice and query_fixed function in witness generation

* bus-auto: fix clippy

---------

Co-authored-by: Aurélien Nicolas <[email protected]>

* fix-tob-scroll-21 (#59)

* fix-tob-scroll-21

* expose param field for re-randomization

* enable accessing for table16 (#75)

* chore: update poseidon link

* merge sha256 gadget changes

* Fix the CI errors (#78)

* cargo fmt

* fix clippy error

* Feat: switch to logup scheme for lookup argument  (#71)

* Multi-input mv-lookup. (#49)

* Add mv_lookup.rs

* mv_lookup::prover, mv_lookup::verifier

* Replace lookup with mv_lookup

* replace halo2 with mv lookup

Co-authored-by: ying tong <[email protected]>

* cleanups

Co-authored-by: ying tong <[email protected]>

* ConstraintSystem: setup lookup_tracker

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

* mv_lookup::hybrid_prover

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

* WIP

* mv_multi_lookup: enable lookup caching

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

* Rename hybrid_lookup -> lookup

* Chunk lookups using user-provided minimum degree

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

* mv_lookup bench

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

* Introduce counter feature for FFTs and MSMs

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

* Fix off-by-one errors in chunk_lookup

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

* bench wip

* time evaluate_h

* KZG

* more efficient batch inversion

* extended lookup example

* Finalize mv lookup

Author: therealyingtong <[email protected]>

* Remove main/

* Fix according to the comments

* replace scan with parallel grand sum computation

* Revert Cargo.lock

* mv lookup Argument name

* parallel batch invert

---------

Co-authored-by: Andrija <[email protected]>
Co-authored-by: ying tong <[email protected]>
Co-authored-by: therealyingtong <[email protected]>

* fmt

* fix unit test

* fix clippy errors

* add todo in mv_lookup's prover

* fmt and clippy

* fix clippy

* add detailed running time of steps in logup's prover

* fmt

* add more log hooks

* more running time logs

* use par invert

* use sorted-vector to store how many times a table element occurs in input

* par the process to get inputs_inv_sum

* use par

* fix par

* add feature to skip inv sums

* add new feature flag

* fix clippy error

---------

Co-authored-by: Sphere L <[email protected]>
Co-authored-by: Andrija <[email protected]>
Co-authored-by: ying tong <[email protected]>
Co-authored-by: therealyingtong <[email protected]>

* fix some simple building errs

* upgrade pathfinder_simd to newer version as it can't compile on mac m1 pro

* resolve merge conflict

* fmt

* clippy

* more clippy fix

* more lint fix

* fmt

* minor syntax fix

* fix ipa multiopen test failure

* fix clippy warning

* fmt

* fix par scan of log_inv diff

* remove uncessary clone

---------

Co-authored-by: alannotnerd <[email protected]>
Co-authored-by: kunxian xia <[email protected]>
Co-authored-by: Steven <[email protected]>
Co-authored-by: Carlos Pérez <[email protected]>
Co-authored-by: zhenfei <[email protected]>
Co-authored-by: Ho <[email protected]>
Co-authored-by: naure <[email protected]>
Co-authored-by: Aurélien Nicolas <[email protected]>
Co-authored-by: Sphere L <[email protected]>
Co-authored-by: Andrija <[email protected]>
Co-authored-by: ying tong <[email protected]>
Co-authored-by: therealyingtong <[email protected]>

---------

Co-authored-by: han0110 <[email protected]>
Co-authored-by: Velaciela <[email protected]>
Co-authored-by: Carlos Pérez <[email protected]>
Co-authored-by: Eduard S <[email protected]>
Co-authored-by: CeciliaZ030 <[email protected]>
Co-authored-by: Brecht Devos <[email protected]>
Co-authored-by: Enrico Bottazzi <[email protected]>
Co-authored-by: Ethan-000 <[email protected]>
Co-authored-by: dante <[email protected]>
Co-authored-by: Mamy Ratsimbazafy <[email protected]>
Co-authored-by: François Garillot <[email protected]>
Co-authored-by: kilic <[email protected]>
Co-authored-by: Thor <[email protected]>
Co-authored-by: CPerezz <[email protected]>
Co-authored-by: chokermaxx <[email protected]>
Co-authored-by: Zhang Zhuo <[email protected]>
Co-authored-by: alannotnerd <[email protected]>
Co-authored-by: kunxian xia <[email protected]>
Co-authored-by: Steven <[email protected]>
Co-authored-by: Ho <[email protected]>
Co-authored-by: naure <[email protected]>
Co-authored-by: Aurélien Nicolas <[email protected]>
Co-authored-by: Sphere L <[email protected]>
Co-authored-by: Andrija <[email protected]>
Co-authored-by: ying tong <[email protected]>
Co-authored-by: therealyingtong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to disable rayon
4 participants