-
Notifications
You must be signed in to change notification settings - Fork 171
[parallel] Introduce data parallelism abstraction #2621
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
Conversation
Deploying monorepo with
|
| Latest commit: |
92d9540
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8246b305.monorepo-eu0.pages.dev |
| Branch Preview URL: | https://cl-parallel-crate.monorepo-eu0.pages.dev |
f82cc6b to
9ec19df
Compare
math/src/algebra.rs
Outdated
| /// | ||
| /// For empty slices, the result should be [`Additive::zero`]; | ||
| fn msm(points: &[Self], scalars: &[R], _concurrency: usize) -> Self { | ||
| fn msm<S: ParStrategy>(points: &[Self], scalars: &[R], _strategy: &S) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we just use Strategy in places like this? I tend to only rename if there is an import conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an import conflict, w/ proptest's Strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_: &impl Parallel would be nice here, eh?
5e5fafb to
dca457f
Compare
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
commonware-mcp | 92d9540 | Jan 06 2026, 08:47 PM |
dc44339 to
ddbcf2c
Compare
cronokirby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main RC is to move from a trait with open ended impls, to a concrete struct which dynamically knows how to parallelize things.
I think we should also eschew providing something as low level as join, in favor of focusing on "fold_init" as the source of the difference in behavior.
parallel/src/lib.rs
Outdated
| /// assert_eq!(sum, 55); | ||
| /// assert_eq!(product, 3628800); | ||
| /// ``` | ||
| fn join<L, LO, R, RO>(&self, left: L, right: R) -> (LO, RO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there places, already, that need this granular level of parallelism?
I think exposing this could be fine, but if so, we should use it to base other methods off of, to avoid burdening the trait too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, no. The primary reason I added this was for future flexibility in areas where we may want to use parallel divide-and-conquer, for example in the bisection-style verification for BLS.
Glad to remove this for now given that no one uses it, if you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this for now.
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea to simplify the test suite is to enforce that:
- parallel fold_init does the same thing as sequential fold_init,
- sequential "other method" does the same thing as the expression in terms of fold_init
I think you could probably streamline the test suite a lot with that.
I think we could do that in a follow up PR though, I think the existing tests provide enough coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated these while removing IntoStrategyIterator 👍
parallel/src/lib.rs
Outdated
| /// let range = 0..100; | ||
| /// let _iter = range.into_iter(); | ||
| /// ``` | ||
| pub trait IntoStrategyIterator: IntoIterator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having a cfg_if defined body of the trait is really hacky.
Also, having rayon leak into the public interface of the module seems less than ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a concrete struct strategy, one approach would be to have this trait represent the concept of being able to call fold on itself (or whatever the ur-method ends up being) using the strategy as an argument. Then, for feature gating, we would just create a nice impl which hooks into a concrete method on strategy which has parallel iterator as a bound. This would avoid having to know about Rayon to implement this for types that don't need parallelism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With having rayon leak into the public interface, what are the issues you foresee there? IntoStrategyIterator intentionally bridges regular iterators with rayon's parallel iterators (when in an std context.) Hiding rayon would allow us to use other backends for data parallelism, but I couldn't think of another that we may want to use.
This would avoid having to know about Rayon to implement this for types that don't need parallelism.
The way I was thinking about it is that you would only use Strategy for situations where you want to allow for parallelism via rayon specifically. If you don't need parallelism, I was assuming you likely wouldn't use Strategy for the type at all. (This was also the intention behind the IntoStrategyIterator bridge requiring that T: IntoParallelIterator outside of no_std contexts.)
Is "use Strategy when you may or may not want to parallelize T's operations with rayon" too narrow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re point 2: you need to use Strategy if you want consumers of your code to be able to parallelize your code. It's correct that there aren't situations where you'd use Strategy if you never want parallelism. However, there are many situations where you need to use Strategy, while also wanting your code to be run Sequentially in, e.g. WASM, or other constrained environments. In that case, you need a public API that's easy to satisfy regardless of what capabilities you have, since that kind of code will have to assume the least possible. You want to avoid the virality induced by having a public API required just to call a function on strategy be feature flagged. Instead, the feature flagging should really only affect what means you have to construct a strategy. Once you have one, it should be usable even if no_std environments.
Re point 1: the main issue is that it makes using the crate more annoying and require reading more documentation needlessly. The user shouldn't have to understand both this crate and read up docs on rayon on how to construct things or what types implement the trait we're linking them to, it should just be a complete interface. It would also be nice to have the option to use other backends, especially if the limitation there comes from exposing implementation details, rather than a higher level conceptual blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, there are many situations where you need to use Strategy, while also wanting your code to be run Sequentially in, e.g. WASM, or other constrained environments. In that case, you need a public API that's easy to satisfy regardless of what capabilities you have, since that kind of code will have to assume the least possible. You want to avoid the virality induced by having a public API required just to call a function on strategy be feature flagged. Instead, the feature flagging should really only affect what means you have to construct a strategy. Once you have one, it should be usable even if no_std environments.
This makes sense. I think the existing API is relatively friendly to deal with here personally, but def open to disagreement. Users should never manually implement IntoStrategyIterator for themselves, their code would look something like:
struct Thing([u8; 16]);
impl IntoIterator for Thing { /* ... */ }
#[cfg(feature = "std")]
impl IntoParallelIterator for Thing { /* ... */ }[dependencies]
rayon = { workspace = true, optional = true }
[features]
std = ["dep:rayon"]where feature flagging would be necessary anyways due to including rayon in the std feature. Thing here would automatically implement commonware_parallel::IntoStrategyIterator, whether std was enabled or not.
the main issue is that it makes using the crate more annoying and require reading more documentation needlessly. The user shouldn't have to understand both this crate and read up docs on rayon on how to construct things or what types implement the trait we're linking them to, it should just be a complete interface.
I mainly didn't want to prematurely abstract here, didn't know of any other backends we may want to use. Are there any that we're thinking of adding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that whatever trait bounds we require for a collection, they should be automatically implemented on any type a user will realistically provide, and it can be more complex to implement it manually.
It's less that we would consider using something other than rayon, but more so that we don't want to commit to a public API based on rayon, because then you're stuck with its way of modelling things forever.
Even more importantly, you want to avoid a situation in which to understand and use this crate, you also need to read a bunch of rayon docs first to figure out how to construct the things you need to pass in here, or what types this crate is asking for, etc. In other words, we want to abstract over rayon, not provide a slightly different interface in addition to it.
3c2b385 to
9611aec
Compare
patrick-ogrady
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should create_pool in the runtime return Parallel (rather than its own type)?
In any case, we should probably have Arc<rayon::ThreadPool> live in parallel now?
coding/src/benches/bench.rs
Outdated
| minimum_shards: min as u16, | ||
| extra_shards: (chunks - min) as u16, | ||
| }; | ||
| let pool = Arc::new(ThreadPoolBuilder::new().num_threads(conc).build().unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create a helper for this in parallel?
coding/src/benches/bench.rs
Outdated
| &Parallel::new(pool.clone()), | ||
| ) | ||
| } else { | ||
| S::encode(&config, data.as_slice(), &Sequential) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A shame that there isn't a single variable we could use to house Sequential/Parallel to avoid rewriting the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could have an enum of sorts that we just provide conc to but maybe that defeats the purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use Box<dyn Strategy>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the discussion around using a concrete type (or Box<dyn Strategy>) vs. the trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what we spoke about on slack - Strategy (now Parallel) cannot be (usefully) dyn-compatible because each generic method requires Self: Sized and therefore cannot be included in the trait object's vtable.
parallel/README.md
Outdated
| [](https://crates.io/crates/commonware-parallel) | ||
| [](https://docs.rs/commonware-parallel) | ||
|
|
||
| Abstract data parallelism over iterators and tasks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We should make the first word a verb
(maybe Parallelize...)
runtime/src/utils/mod.rs
Outdated
| } | ||
|
|
||
| #[test_traced] | ||
| fn test_create_pool_deterministic() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
cronokirby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments:
I have a naming suggestion, to make the use of Strategy clearer.
I think we should also consistently use &impl Strategy rather than an explicit generic ; consistency matters more, and I think this creates much less noise.
We also want to avoid storing a thread pool and creating the strategy on demand, in favor of storing the strategy.
math/src/algebra.rs
Outdated
| /// | ||
| /// For empty slices, the result should be [`Additive::zero`]; | ||
| fn msm(points: &[Self], scalars: &[R], _concurrency: usize) -> Self { | ||
| fn msm<S: ParStrategy>(points: &[Self], scalars: &[R], _strategy: &S) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_: &impl Parallel would be nice here, eh?
coding/src/benches/bench.rs
Outdated
| &Parallel::new(pool.clone()), | ||
| ) | ||
| } else { | ||
| S::encode(&config, data.as_slice(), &Sequential) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use Box<dyn Strategy>
a1d9872 to
753268c
Compare
Uses `ParallelBridge` from the `rayon` crate to convert iterators into parallel iterators, simplifying the codebase by removing the custom `IntoStrategyIterator` trait. There's a slight performance hit, but the API is much more ergonomic and doesn't bind to `rayon` specifics.
…ential` -> `ParallelNone`
d85f023 to
92d9540
Compare
cronokirby
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2621 +/- ##
==========================================
+ Coverage 92.82% 92.87% +0.04%
==========================================
Files 361 362 +1
Lines 107102 107278 +176
==========================================
+ Hits 99416 99631 +215
+ Misses 7686 7647 -39
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Overview
Inspired by #2187, introduces
commonware-parallel, which offers a thin abstraction over aStrategythat can perform afoldorjoinoperation either sequentially (withIterator) or in parallel (withrayon'sParallelIterator.) These strategies are objects, rather than marker traits, so that types can be given aStrategyto be used in any operations attached to it. TheParallelstrategy owns acommonware_runtime::ThreadPool(Arc<rayon::ThreadPool>).In addition, fixes
commonware-runtime'screate_poolfunction to work with thedeterministicruntime by usingrayon'sThreadPoolBuilder::use_current_threadoption. When set, this enables the current thread to steal queued work when yielded to.TODO:
codingmathcryptographyStrategyacross the board? There's quite a few places where the concurrency argument was not exposed, and this PR tries to honor that. It may be beneficial, though, to allow passing aStrategythrough every function / type that can make use of it? -> Exposed it in the areas I think make sense.closes #1540
closes #2187