Consolidate on references instead of owned data #27157
Conversation
The current implementation converts a `TupleRef` iterator into a vector and consolidates that. This might cause extra work if the data consolidates well because it needs to convert the whole input into owned data. With this change, we first collect the references into a vector, consolidate this, and then convert into an owned allocation. Signed-off-by: Moritz Hoffmann <mh@materialize.com>
|
I'd have a hard time how much this specific change will help - in many cases the inputs to this function should have few duplicates, and in that case this is a slight pessimization. (We're consolidating mostly to get the data in sorted order.) We could almost certainly get away with allocating & sorting a Vec of indices into the data instead of the data itself, though. That's a more invasive change but would eliminate all these small allocations entirely. It's a bit unusual to see so much data going through this function, though I know of a couple cases where it's possible. (Load generators and source snapshots, mostly.) Can you say a bit more about the workload you were running that generated this trace? |
It was a load generator someone else was running! |
|
Sketched out what I was talking about above: #27168 The idea is that you only allocate a collection of indices into the original collection, not a vec-of-vecs. Should be less allocation overall but also less fragmentation, since we're not allocating a ton of small vecs. Also haven't benched it yet however... |
|
Cool, I'll close this one in favor of your change! Thanks very much! |
The current implementation converts a
TupleRefiterator into a vector and consolidates that. This might cause extra work if the data consolidates well because it needs to convert the whole input into owned data.With this change, we first collect the references into a vector, consolidate this, and then convert into an owned allocation.
I didn't measure the performance impact, but I saw this as a source of memory allocations in https://pprof.me/c3516597e535d8c64a2184c143c408db
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.