fix(array): honor the execution allocator for RowFn outputs - #10014
Conversation
Merging this PR will degrade performance by 1.39%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | filtered_owned_i64_avx2[OneNullInEight] |
21.8 µs | 26 µs | -16.07% |
| ❌ | WallTime | decode_avx512[8192, (Inline, OneNullInEight)] |
80.9 µs | 95.1 µs | -14.97% |
| ❌ | WallTime | decode_avx2[8192, (External, OneNullInEight)] |
152.3 µs | 174.3 µs | -12.63% |
| ❌ | WallTime | dict_canonicalize_gt_u8_neon[1000000] |
492.3 µs | 548 µs | -10.16% |
| ❌ | Simulation | non_nullable[256] |
196.3 µs | 218.2 µs | -10.06% |
| ⚡ | Simulation | decode_primitives[f32, (1000, 32)] |
35.9 µs | 22 µs | +63.58% |
| ⚡ | WallTime | decode_avx512[8192, (Inline, AllValid)] |
112.8 µs | 102.5 µs | +10.03% |
| Simulation | fixed_16_advancing_ptr_safe[100] |
< 1 ns | < 1 ns | N/A | |
| Simulation | preverify_advancing_ptr_unchecked[1000] |
< 1 ns | < 1 ns | N/A | |
| Simulation | preverify_advancing_ptr_unchecked[10000] |
< 1 ns | < 1 ns | N/A |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ct/row-fn-output-allocator (8e3b4c0) with develop (eff2b1e)
Footnotes
-
385 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
7042d42 to
573b85f
Compare
573b85f to
4aef610
Compare
6bf049e to
a200209
Compare
a200209 to
7da447a
Compare
7da447a to
e1ca00c
Compare
|
(blocked on #10030) |
| let mut values: Vec<Out> = std::iter::repeat_with(Out::default) | ||
| .take(valid_rows.len()) | ||
| .collect(); | ||
| let mut values = Out::allocate(valid_rows.len(), ctx.allocator()); |
There was a problem hiding this comment.
Nit: rename allocate to with_capacity to align with Vec etc?
## Summary `BufferMut::reserve_allocate` can pass a smaller layout to `Allocator::grow` when alignment padding or slicing leaves the backing allocation larger than the new capacity requires. This violates the allocator's safety contract and can panic in debug builds. The bug dates to #9668 and was exposed by the custom allocator test in #10014. ## Changes Use the existing allocate-and-copy path when the new layout is smaller, preserving the configured allocator and initialized values. Add a deterministic regression that grows a short slice of a larger allocation. Signed-off-by: "Connor Tsui" <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
e1ca00c to
8e3b4c0
Compare
Depends on #10016.
RowFn output payloads bypass the execution allocator. This change allocates them directly through
ctx.allocator()across owned, deferred-retry, selected, filtered, constant, and sink execution, while preserving zero-copy primitive publication and empty-output paths.OutputElementchooses its collection storage through an associated buffer type and an allocation hook. The executor writes throughOutputBufferslots, and the buffer implementation constructs the array. Vortex primitive and Boolean implementations useBufferMut, while scalar and fixed-size-list sinks use the same storage contract. UTF-8 descriptors, external bytes, and polygon payloads also use the execution allocator. Physical sink parameters remain separate from allocation resources.Regressions check ownership of returned payloads using canonical inputs prepared before allocation tracking, including a context override, constant UTF-8 output, retry execution, and zero-copy reuse. A zero-sized output with
Vecstorage exercises the owned execution paths without requiringBufferMut. Boolean collector selection is preserved.The Boolean dense-retry path from merged #9986 also uses the execution allocator, with coverage in the existing packed-output allocator test.
Validation before the rebase: 168 focused comparison, mask, and RowFn tests passed on the combined stack through #9979, along with
cargo clippy -p vortex-array --all-targets --all-features -- -D warnings. Tests, formatting, and benchmarks were not rerun locally after the rebase.