-
Notifications
You must be signed in to change notification settings - Fork 271
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
Remove all Pusher/Puller boxing using GATs #460
base: master
Are you sure you want to change the base?
Conversation
timely/src/dataflow/channels/pact.rs
Outdated
|
||
/// A `ParallelizationContractCore` allocates paired `Push` and `Pull` implementors. | ||
pub trait ParallelizationContractCore<T, D> { | ||
pub trait ParallelizationContractCore<G: ScopeParent, D> { |
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 looks like a regression. Can we leave it as T
?
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.
Yes, just fixed that
@@ -43,8 +43,8 @@ impl<T:Timestamp+Send> Progcaster<T> { | |||
let addr = path.clone(); | |||
Progcaster { | |||
to_push: None, | |||
pushers, | |||
puller, | |||
pushers: pushers.into_iter().map(|p| Box::new(p) as Box<dyn Push<_>>).collect(), |
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.
@frankmcsherry This is the only place I kept the trait objects because otherwise Progcaster
would have to become generic over the pusher which would then add a generic parameter to Subgraph
. I didn't know if that was ok but if it is I can eliminate this last bit of trait object if that sounds good
e602f8e
to
2046265
Compare
This PR now compiles on stable! |
Signed-off-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
Currently the
Allocate::allocate
method is hardcoded to return a(Vec<Box<dyn Push<Message<T>>>>, Box<dyn Pull<Message<T>>>)
pair. Boxing was necessary because the genericT
parameter was on the method.Generic associated types is the missing piece to allow the concrete type to be spelled out and since they are right around the corner for stable Rust I went ahead and implemented it.
This PR removes every single use of a
Box<dyn Puller/Pusher>
instance in the codebase. It is not mergeable yet because it requires the nightly feature flag but we could discuss it and merge it once GATs are released.