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

change: Move from maybe_rayon to rayon-1.8 #122

Merged
merged 7 commits into from
Jan 5, 2024
Merged

Conversation

CPerezz
Copy link
Member

@CPerezz CPerezz commented Jan 5, 2024

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: Add a fallback when threading is unsupported rayon-rs/rayon#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.
  • The CI has also been modified to keep accounting for WASM compatibility by forcing the build to wasmi and wasm32 targets on each CI run. (It does so by targeting test build such that we can get getrandom as a dev-dependency which will not get pulled on consumer's machines.
  • Finally, updates the README to provide further info on how this lib should be used by a consumer if they want to use it within WASM.

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: rayon-rs/rayon#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.
@CPerezz CPerezz requested review from han0110 and kilic January 5, 2024 09:14
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@CPerezz CPerezz force-pushed the change/rayon_dep branch 2 times, most recently from cb55f40 to d560c95 Compare January 5, 2024 11:14
Copy link
Contributor

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM! Just some final nits that can be ignored.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@CPerezz CPerezz added this pull request to the merge queue Jan 5, 2024
Merged via the queue into main with commit 94dd63e Jan 5, 2024
11 checks passed

[features]
default = ["bits", "multicore"]
multicore = ["maybe-rayon/threads"]
Copy link
Collaborator

@kilic kilic Jan 10, 2024

Choose a reason for hiding this comment

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

[[bench]]
name = "msm"
harness = false
required-features = ["multicore"]

Seems like we had a leftover feature for msm benchs

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohh dammit!!
Will update. Hopefully this will not affect the released crate.

jonathanpwang pushed a commit to axiom-crypto/halo2curves that referenced this pull request Apr 19, 2024
…rations#122)

* 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: rayon-rs/rayon#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.

* change: Update CI to account for WASM compat

* chore: Add paralellism section to README

* chore: Fix CI missing "

* chore:  Split WASM build & add targets

* chore: Test all features for regular-target  builds

* chore: Address review nits
jonathanpwang pushed a commit to axiom-crypto/halo2curves that referenced this pull request Apr 24, 2024
…rations#122)

* 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: rayon-rs/rayon#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.

* change: Update CI to account for WASM compat

* chore: Add paralellism section to README

* chore: Fix CI missing "

* chore:  Split WASM build & add targets

* chore: Test all features for regular-target  builds

* chore: Address review nits
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.

3 participants