Fuse Map/Filter/Project operators on top of a Get #4463
Conversation
src/dataflow/src/render/mod.rs
Outdated
| ) -> bool { | ||
| if let Some((mfp, input)) = self.extract_map_filter_project(relation_expr) { | ||
| self.ensure_rendered(&input, scope, worker_index); | ||
| let (ok_collection, mut err_collection) = self.collection(&input).unwrap(); |
There was a problem hiding this comment.
I think this is the moment where we could integrate this more tightly, using the flat_map_ref method instead of collection. It has the potential to avoid pulling the contents of the arrangement first, and then flat_map_fallible'ing them afterwards, and instead doing all the work at once.
|
The set-up looks great. I think the thing to check out next are some of the ways to integrate a bit more tightly, if we are worried about performance before landing. For example, a very restrictive filter should demonstrate a performance difference when push in through the |
This is part 1 of the work outlined in MaterializeInc#4346, and builds off of MaterializeInc#3258, where we detect map/filter/project chains already present in plans but do not attempt to synthesize them. I did some quick and dirty unscientific benchmarking and noticed some slight slowdown rather than any performance improvement, not sure if there are any thoughts on what might need to be done to see some improvement.
|
That change showed perf improvements! My (very unscientific) benchmark with the following query with 1000000 rows: took 1.5s with the new impl and 3.5s with the old one. |
| let mut row_packer = repr::RowPacker::new(); | ||
| let temp_storage = RowArena::new(); | ||
| move |row| { | ||
| mfp.evaluate(&mut row.unpack(), &temp_storage, &mut row_packer) |
There was a problem hiding this comment.
Very minor, and not actionable, but this is the other moment at which a potentially optional allocation happens. row.unpack() will produce a Vec<Datum> that we could try and wrangle in to re-using an existing allocation. It's a bit awkward with lifetimes, but inside flat_map_ref() we could mess around a bit there and re-use one vector for each operator invocation.
There was a problem hiding this comment.
Just read the code, and it would at least involve copy/paste of differential's flat_map_ref, as that method does not expose the 'storage lifetime, which is what we would want to use for the lifetime of the Vec<Datum>.
|
Cool, TFTR! Thanks for the notes. |
In MaterializeInc#4463 we introduced MapFilterProjects on top of Gets to reduce the amount of data that we had to pack and unpack across operator boundaries. When doing this, I made one RowArena that was used across each invocation of MFP, which resulted in memory buildup over time. This change changes this to occur per-row so that we can throw away intermediate data.
This is part 1 of the work outlined in #4346, and builds off of #3258,
where we detect map/filter/project chains already present in plans but
do not attempt to synthesize them.
I did some quick and dirty unscientific benchmarking and noticed some
slight slowdown rather than any performance improvement, not sure if
there are any thoughts on what might need to be done to see some
improvement. I'm not sure if you had in mind some kind of tighter fusing
on top of a Get, or not?
I triggered the behaviour with this kind of query:
This change is