Differential corrected atop candidate timely 0.27#671
Differential corrected atop candidate timely 0.27#671frankmcsherry merged 6 commits intoTimelyDataflow:master-nextfrom
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.
There was a problem hiding this comment.
We could change partition to return [_; N] -- maybe. What you could do here is let [part1, part2] = parts.try_into().unwrap()
There was a problem hiding this comment.
Yes; some ideas there around making a PartitionBy trait that you implement for e.g. (Val1, Val2) or [Val: K] or various things, with an associated return type that essentially mirrors the structure.
|
A next step I'm going to try out is understanding |
frankmcsherry
changed the title
|
Retargeted at |
| let reverse = edges.map_in_place(|x| ::std::mem::swap(&mut x.0, &mut x.1)) | ||
| let as_self = edges.clone().arrange_by_self(); | ||
| let forward = edges.clone().arrange_by_key(); | ||
| let reverse = edges.clone().map_in_place(|x| ::std::mem::swap(&mut x.0, &mut x.1)) |
There was a problem hiding this comment.
The last one wouldn't necessarily need a clone, but there's no runtime cost in doing so...
There was a problem hiding this comment.
Yeah, there's a bunch of that. When do you muck up the visual patterns to optimize out a clone() that doesn't actually cost anything. Not sure. Looks weird like this, looks weird without it.
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 itsDerefimpl 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.