Avoid a temporary allocation when reporting frontiers#20904
Conversation
Signed-off-by: Moritz Hoffmann <mh@materialize.com>
| } | ||
|
|
||
| new_uppers.push((id, new_frontier)); | ||
| new_uppers.push((id, new_frontier.clone())); |
There was a problem hiding this comment.
Now we avoid the allocation above but need to .clone() here. How is this different in terms of memory footprint?
There was a problem hiding this comment.
Most of the iterations don't even get here because we only report changes.
| let mut new_uppers = Vec::new(); | ||
|
|
||
| // Maintain a single allocation for `new_frontier` to avoid allocating on every iteration. | ||
| let mut new_frontier = Antichain::new(); |
There was a problem hiding this comment.
No strong feelings, but I'd like to call out that this reverts a suggestion you made here: #20090 (comment) :D
There was a problem hiding this comment.
Yes, I know! Sorry for that, and it's also why I added a comment so we don't forget!
|
|
||
| for (&id, collection) in self.compute_state.collections.iter_mut() { | ||
| let mut new_frontier = Antichain::new(); | ||
| new_frontier.clear(); |
There was a problem hiding this comment.
I don't think clearing is necessary here, based on that before #20090 it also didn't happen. Presumably both read_upper and clone_from do their own clears.
While writing this I have convinced myself that this is still a good thing to do, as a matter of defensive programming. So feel free to disregard this :)
There was a problem hiding this comment.
Yup, you're correct, but I added it to not confuse myself again that there is any important state maintained across loop iterations.
3 participants
Motivation
Fixes an unreported issue. We're allocating a new
Antichainfor every maintained collection when reporting frontiers. With this PR, we re-use the allocation, avoiding a significant number of temporary allocations.It's hard to say what performance impact this has, but it's good habit to avoid allocations if it's easy to do.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.