fix(buffer): avoid shrinking allocations through grow - #10030
Conversation
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | filtered_sink_i64_avx512[OneNullInEight] |
22.4 µs | 26.3 µs | -15.13% |
| ⚡ | WallTime | filtered_sink_i64_avx2[OneNullInEight] |
26.2 µs | 21.9 µs | +19.52% |
| ⚡ | WallTime | decode_avx512[8192, (Inline, OneNullInEight)] |
93.8 µs | 81.2 µs | +15.53% |
| ⚡ | WallTime | dict_canonicalize_gt_u8_neon[1000000] |
549.8 µs | 488.4 µs | +12.56% |
| ⚡ | Simulation | set_indices_vortex_buffer[128] |
2.1 µs | 1.8 µs | +12.01% |
| 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/buffer-grow-layout (b0aa3bc) with develop (2d9414a)
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. ↩
Signed-off-by: "Connor Tsui" <connor.tsui20@gmail.com>
d85c6ad to
b0aa3bc
Compare
| // The default global allocator (`is_statically_allocated`) uses allocate-and-copy. Custom | ||
| // allocators must also allocate and copy when the new layout is smaller, because | ||
| // `Allocator::grow` forbids shrinking. | ||
| let needs_new_allocation = self.allocation.allocator().is_statically_allocated() |
There was a problem hiding this comment.
I don't like that we even expose is_statically_allocated (this was me...). But we shouldn't be switching on the implementation of the allocator imo.
We already do this somewhere, so let's keep it and remove this when we experiment with new buffer allocators.
Summary
BufferMut::reserve_allocatecan pass a smaller layout toAllocator::growwhen 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.