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

improve portability #615

Closed
Sladuca opened this issue Jul 15, 2022 · 10 comments
Closed

improve portability #615

Sladuca opened this issue Jul 15, 2022 · 10 comments

Comments

@Sladuca
Copy link
Contributor

Sladuca commented Jul 15, 2022

There are a lot of dependencies that are either 1) only used in tests/examples (e.g. rand), or 2) do not compile in rust-based smart contract runtimes (e.g. env_logger, log, std::time, rayon). If we intend this library to be used outside a standard environment, we need to be able to build the library without these dependencies.

To address this, I suggest the following:

  1. feature-gate log / TimingTree behind a feature that's enabled by default, and use conditional compilation to remove timed!, with_context!, TimingTree, and ContextTree if disabled
  2. Wrap rayon's par_iter with a macro cfg_iter that conditionally switches between iter() and par_iter like arkworks does here
  3. add a test_utils feature to deal with stuff like the gate_testing.rs in the interrim until cargo supports this functionality
@Sladuca
Copy link
Contributor Author

Sladuca commented Jul 15, 2022

Happy to open a PR for this (I'm already working on it because I need it)

@Sladuca
Copy link
Contributor Author

Sladuca commented Jul 15, 2022

I've got a branch I'm using for this at https://github.com/proxima-one/plonky2/tree/feature-gate-stuff

@dlubarov
Copy link
Contributor

For logging, shouldn't log itself be portable since it no-ops by default? I see why env_logger could be an issue though; would moving it to dev-dependencies address the concern?

For rand, I think a feature gate sounds good, like num has.

For Rayon, I think the feature would ideally be in Rayon itself, see rayon-rs/rayon#861. If we don't want to wait for that we can add a shim temporarily, but I don't really like the arkworks approach since it changes all the call sites to use macros. I'd prefer either

  • conditionally importing a rayon module with shims for ParallelIterator etc, or
  • having a maybe_rayon module, containing a MaybeParallelIterator with a maybe_par_iter(), etc

A feature gate for timed! and with_context! sounds fine, as long as it doesn't change the API; seems like the changes can be internal to those macros.

@Sladuca
Copy link
Contributor Author

Sladuca commented Jul 15, 2022

I personally need a shim for rayon now, but I can keep that part in our branch for now if others don't need it too urgently.

As for timed! and with_context!, the main thing is that all of the callsites change because the timing parameter taken by many functions won't exist if there's no TimingTree (no TimingTree because no std::time). So you still end up having conditional compilation to change the invocation each time timed! or with_context! is called, along with any time any function that takes a timing parameter is called even if you're making the change within the macro. I'm not sure if there's a cleaner way to do it.

Log might actually be fine after looking at the source - I was running into some compilation issues with it but it might have actually been something else.

@dlubarov
Copy link
Contributor

Do you have an idea for how we could handle ZK configurations if rand is disabled? It looks like there's still a rand_vec call in your branch (see oracle.rs). Since ZK is a runtime setting while rand would be a compile time setting, I guess we would need to just add an assert!(!zero_knowledge) and panic at runtime.

@Sladuca
Copy link
Contributor Author

Sladuca commented Jul 15, 2022

Oh, that's a good point. I guess you'd need to have that assertion. Then it can be a separate feature that some runtimes can enable. For instance in a browser, where there's no rayon but rand might be available.

@Sladuca
Copy link
Contributor Author

Sladuca commented Jul 21, 2022

For Rayon, I think the feature would ideally be in Rayon itself, see rayon-rs/rayon#861. If we don't want to wait for that we can add a shim temporarily, but I don't really like the arkworks approach since it changes all the call sites to use macros. I'd prefer either

  • conditionally importing a rayon module with shims for ParallelIterator etc, or
  • having a maybe_rayon module, containing a MaybeParallelIterator with a maybe_par_iter(), etc

Of these, I think I prefer the second option, as it's more explicit that's what's happening and it's less code to write. In either case, the iter() method of stuff like strings, slices, and vecs aren't part of an Iter trait, so we'll have to implement the trait for each type we want to usemaybe_par_iter() on. We'll also need to repeat the process for maybe_into_par_iter(), maybe_par_iter_mut(), maybe_par_chunks(), and maybe_par_chunks_mut().

It ends up being a lot more code than the macro approach, which can be replaced later relatively easily once rayon-rs/rayon#861 happens. You just have to go through and replace all the call sites, which doesn't take that long.

@Sladuca
Copy link
Contributor Author

Sladuca commented Jul 21, 2022

We can also just yoink similar instances of it like this: https://github.com/shssoichiro/oxipng/blob/master/src/rayon.rs. I'll look over the options and open a PR for whatever seems cleaner.

This was referenced Jul 21, 2022
@pgebheim
Copy link
Collaborator

@dlubarov @Sladuca -- Can we close this?

@dlubarov
Copy link
Contributor

I think the original inspiration for this (Plonky2 verification on Solana) went away, so it's at least not a priority; I think we can close for now and maybe reopen if the need arises

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

No branches or pull requests

3 participants