viirya commented on PR #5778: URL: https://github.com/apache/datafusion-comet/pull/5778#issuecomment-5595412077
You are right that the previous revision only handled a lone row at the *start* of the loop, and that `[1, 1, 8]` still gathered eight times. That was a real regression, not just a missed opportunity. Two shapes on the current benchmark, base vs. that revision: | shape | base | previous revision | |---|---|---| | 2 short rows + an 8-element tail | 1.56 µs | 5.06 µs (+224%) | | 2 short rows + a 1024-element tail | 210 µs | 514 µs (+145%) | The check now runs at the top of every pass, so the tail after the shorter rows finish takes the direct path. The comment's "one row left" wording now describes what the code does. While confirming that I also found a `!uniform` guard I had put on the check, which left a single-row uniform batch on the gather path at 2.27 ms; without the guard it is 510 µs. The adverse shapes are now checked into `benches/hash.rs` rather than reported, so the comparison reproduces. Matched base/head, 8192 rows unless noted, Apple M4 Max: | case | base | head | |---|---|---| | `list_of_struct_x10` | 9884 µs | 720 µs | | `list_of_struct_1kb_string_x4` (2048 rows) | 3550 µs | 2538 µs | | `list_of_struct_half_null_x10` | 20407 µs | 733 µs | | `list_of_struct_long_tail_x1024` (2 rows) | 124 µs | 129 µs | | `list_of_int32_x10` | 147 µs | 147 µs | The long-tail case is the transition you asked about, and it comes out at parity rather than ahead: repeated runs give 123-129 µs against a base of 124 µs, so I would not claim a win there — the point is that it is no longer 514 µs. `list_of_int32_x10` is the untouched control. Adding those shapes surfaced a bug in the benchmark harness itself: it seeded `vec![42u32; NUM_ROWS]` while indexing by the array's length, so any shape that is not 8192 rows panicked (`the len is 2049 but the index is 8192`). It now sizes the buffer from the array. That is a fix to the existing benchmark, independent of this change. On the allocation side, the four per-pass vectors are now hoisted out of the loop and cleared per pass, so the steady-state allocation is the `take` output plus those buffers once. I kept `rows_at_position` as its own vector: the uniform and non-uniform branches fill `positions` from different row sets, and folding it into `active` would tie the survivor compaction to the gather order for no measurable gain. What I have not done is measure peak RSS or total bytes allocated. criterion does not report it, so it needs a counting allocator or dhat, and I would rather report a number I trust than an estimate. The `take` output for `1kb_string_x4` is bounded by one gathered element per surviving row per pass, which is why I sized that case at 1 KB payloads, and it is the shape where the win is smallest (-29%) precisely because the copy dominates. If you want the allocation figures pinned before this merges, say so and I will wire up a counting allocator in the benchmark; I did not want to hold the timing data for it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
