refactor(array): propagate allocators through builders - #9670
Conversation
9f76879 to
cb223eb
Compare
cb223eb to
dd4f747
Compare
dd4f747 to
fd018db
Compare
fd018db to
737f66c
Compare
737f66c to
08a209a
Compare
56d0be5 to
4af7bf3
Compare
01b019a to
de08d68
Compare
de08d68 to
b773833
Compare
b773833 to
4b92e40
Compare
62670ea to
f494e5a
Compare
Merging this PR will degrade performance by 8.22%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decompress[u64, (4000, 1024)] |
70.8 µs | 86 µs | -17.63% |
| ❌ | WallTime | filtered_owned_i64_avx512[OneNullInEight] |
22.4 µs | 26.1 µs | -14.24% |
| ❌ | Simulation | chunked_bool_canonical_into[(1000, 10)] |
27.1 µs | 30.6 µs | -11.47% |
| ⚡ | Simulation | allocate_drop_arrow[0] |
456.9 ns | 402.7 ns | +13.45% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ngates/buffer-allocator-builders (542aed9) with develop (bffdca1)
Footnotes
-
218 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. ↩
d5d1c39 to
fd3a728
Compare
fd3a728 to
cfcc653
Compare
cfcc653 to
f497774
Compare
f497774 to
a8608ae
Compare
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
a8608ae to
f627f7c
Compare
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
|
No breaks please, can you add back old api and mark it deprecated. Users don't want the churn just because you need a new feature. |
|
I dunno… most callers will have an ExecutionCtx in scope and not pass it. In fact, shouldn't ArrayBuilders hold ExecutionCtx anyway so that you can extend_from_array? |
|
I am purely arguing from the churn POV, it would be ideal if we can avoid forcing people to update their builder usage when they upgrade vortex. I think we can remove it in 1-2 releases |
Signed-off-by: Nicholas Gates <nick@nickgates.com>
| /// | ||
| /// Prefer [`builder_with_capacity_in_ref`] when borrowing an allocator already in scope. | ||
| #[deprecated(note = "use `builder_with_capacity_in_ref` with a borrowed allocator")] | ||
| pub fn builder_with_capacity_in( |
There was a problem hiding this comment.
I think since this method wasn't widely used before we can break this ONE method so that we don't have builder_with_capacity_in_ref
Signed-off-by: Nicholas Gates <nick@nickgates.com>
`VarBinBuilder::with_capacity` was deprecated when allocators were propagated through builders (#9670), after the btrblocks binary scheme (#9576) was written against the old API. CI denies warnings, so pass the execution context's allocator instead. Signed-off-by: "Claude" <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd
Summary
Require array builder call sites to choose a buffer allocator explicitly while keeping the old APIs as deprecated compatibility shims.
Changes
_inAPIs and deprecate the allocator-free builder constructors.&BufferAllocatorRefat API boundaries; clone only when a buffer or builder retains it.ctx.allocator()in execution paths; use the static allocator only where no execution context exists.