WIP: Differential corrected atop candidate timely 0.27#671
WIP: Differential corrected atop candidate timely 0.27#671frankmcsherry wants to merge 3 commits intoTimelyDataflow:masterfrom
Conversation
| /// }); | ||
| /// ``` | ||
| pub fn concatenate<I>(&self, sources: I) -> Self | ||
| pub fn concatenate<I>(self, sources: I) -> Self |
There was a problem hiding this comment.
Consider removing the concatenate function as we did in Timely, or make it an associated function.
There was a problem hiding this comment.
Hrm. I'm looking at the implementation and it feels easy enough. Is the suggestion just to bring it to parity with timely? We could also add it back to timely, which lost it for an unclear to me reason (I agree I did it, but perhaps it shouldn't have).
There was a problem hiding this comment.
I think the reason TD lost it is because the argument would need to change to self, which is totally possible (scopes can be cloned), but that wasn't as obvious at the moment.
| .count(); | ||
|
|
||
| let odds = counts.filter(|x| x.1 % 2 == 1); | ||
| let odds = counts.clone().filter(|x| x.1 % 2 == 1); |
There was a problem hiding this comment.
This change, and a few others like it, call out the benefit of partition.
| let propose0 = index_xz.propose(&parts[0].as_collection()); | ||
| let propose1 = index_yz.propose(&parts[1].as_collection()); | ||
| let propose0 = index_xz.propose(parts[0].clone().as_collection()); | ||
| let propose1 = index_yz.propose(parts[1].clone().as_collection()); |
There was a problem hiding this comment.
We used partition but end up preparing a clone anyhow. It won't actually clone, but the additional vcall will happen. Worth watching to see about improving the ergonomics / patterns.
|
A next step I'm going to try out is understanding |
2 participants
Many things to discuss, but thought I'd put this up so we can see the horror. Only builds locally against a local timely check-out, so the red is not surprising.
The changes here are mainly in response to
The first one was the larger problem, as far as I could tell. There are a lot of new explicit
.clone()calls, that were 100% implicit before. But they are here now and a bit in the way, often. There are a few painful moments that happened a lot:.scope()later on, but the stream has already been consumed.Variableand it'sDerefimpl gave us access to the stream; now we call.clone()as a hack.Arrangedis generally consumed as well, though there are moments where we don't need all of it, but can't deconstruct and share the parts around.