fix(buffer): address allocator review follow-ups by gatesn · Pull Request #9745 · vortex-data/vortex

gatesn · 2026-09-03T11:32:54Z

Summary

Make BufferAllocatorRef an ArcRef<dyn BufferAllocator>. Callers can provide a static
reference or an owned Arc.
Move the 256-byte preference into StaticBufferAllocator.
Request aligned allocations directly and delegate growth to Allocator::grow.
Adopt Vec allocations with a static reference to Global.
Remove the stored pointer and capacity from BufferMut. It remains 64 bytes on 64-bit targets.
Preserve a unique sliced buffer by moving its live values to the allocation base before making
it mutable.
Give zero-sized buffers usize::MAX capacity, matching Vec.

Benchmark

cargo bench -p vortex-buffer --bench allocation -- --min-time 0.1

Medians on an Apple M5 Max. “Static” and “Arc” use the same 256-byte-aligning allocator. “Global”
uses a static Global reference and requests one-byte alignment.

Size	Allocate static	Allocate Arc	Allocate Global
64 B	22.55 ns	21.90 ns	17.98 ns
256 B	21.74 ns	21.74 ns	17.34 ns
1 KiB	25.48 ns	25.16 ns	20.64 ns
16 KiB	24.50 ns	24.18 ns	20.11 ns
64 KiB	86.36 ns	84.41 ns	81.16 ns

Size	Freeze static	Freeze Arc	Freeze Global
64 B	38.50 ns	38.83 ns	32.31 ns
256 B	38.50 ns	38.83 ns	31.99 ns
1 KiB	42.73 ns	42.41 ns	35.58 ns
16 KiB	39.81 ns	39.81 ns	34.27 ns
64 KiB	103.2 ns	104.5 ns	90.27 ns

The static-reference and Arc forms are within noise. The visible cost comes from requesting
256-byte alignment, not from ArcRef dispatch or reference counting.

Validation

cargo test -p vortex-buffer --all-features
cargo test -p vortex-array --all-features allocator
cargo test -p vortex-file --all-features test_open_path_uses_memory_session_allocator
cargo clippy -p vortex-buffer -p vortex-array -p vortex-io -p vortex-file --all-targets --all-features
cargo check --workspace --all-features

Signed-off-by: Nicholas Gates <nick@nickgates.com>

codspeed-hq · 2026-09-03T11:43:06Z

Merging this PR will degrade performance by 15.34%

⚠️

Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 8 improved benchmarks
❌ 38 regressed benchmarks
✅ 2123 untouched benchmarks
🆕 12 new benchmarks
⏩ 218 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`allocate_freeze_drop_vortex_minimal_alignment[64]`	3.5 µs	5.6 µs	-37.77%
❌	Simulation	`take_fsl_f16_force_per_index[2048, 10]`	361.1 µs	545.3 µs	-33.78%
❌	Simulation	`chunked_varbinview_canonical_into[(1000, 10)]`	365.5 µs	551 µs	-33.66%
❌	Simulation	`allocate_drop_vortex[0]`	957.4 ns	1,428 ns	-32.95%
❌	Simulation	`chunked_varbinview_into_canonical[(1000, 10)]`	405.1 µs	593.3 µs	-31.72%
❌	Simulation	`take_fsl_f16_force_per_index[1024, 10]`	209.4 µs	301.3 µs	-30.52%
❌	Simulation	`chunked_varbinview_opt_into_canonical[(1000, 10)]`	442.7 µs	625.3 µs	-29.2%
❌	Simulation	`chunked_varbinview_opt_canonical_into[(1000, 10)]`	426.4 µs	594.7 µs	-28.31%
❌	Simulation	`take_fsl_f16_force_per_index[512, 10]`	133.5 µs	179.7 µs	-25.73%
❌	Simulation	`allocate_drop_vortex_minimal_alignment[0]`	901.5 ns	1,206.1 ns	-25.25%
❌	Simulation	`chunked_varbinview_canonical_into[(100, 50)]`	272.6 µs	362.5 µs	-24.81%
❌	Simulation	`chunked_varbinview_into_canonical[(100, 50)]`	347.2 µs	438.2 µs	-20.77%
❌	Simulation	`runend_compress_u32`	389.2 µs	487.6 µs	-20.18%
❌	Simulation	`chunked_varbinview_opt_canonical_into[(100, 50)]`	362.5 µs	451.1 µs	-19.64%
❌	Simulation	`take_fsl_f16_force_per_index[256, 10]`	94.5 µs	117.6 µs	-19.64%
❌	Simulation	`compress[(10000, 4)]`	486.2 µs	604.9 µs	-19.62%
❌	Simulation	`non_nullable[32]`	252.7 µs	313.1 µs	-19.3%
❌	Simulation	`non_nullable[256]`	246.3 µs	305 µs	-19.27%
❌	Simulation	`nullable[256]`	247.7 µs	306.6 µs	-19.21%
❌	Simulation	`nullable[32]`	254.3 µs	314.7 µs	-19.21%
...	...	...	...	...	...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

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-review-followups (8861f3e) with develop (dab1684)}

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. ↩

gatesn · 2026-09-03T11:58:35Z

                let offset = ptr.addr().get() - allocation.ptr().addr().get();
-                let capacity = if allocation.size() == 0 {
+                let capacity = if size_of::<T>() == 0 {
+                    usize::MAX


This is the same as native Rust Vec

joseph-isaacs · 2026-09-03T12:11:04Z

+    /// The minimum alignment promised for `ptr` and preserved by aligned slices.
    pub(crate) alignment: Alignment,


When does this differ from size_of::<T>?

This is what vortex buffer has always done, we support arbitrary alignment.

Both develop and here in fact over-align to a min of 256 bytes

joseph-isaacs · 2026-09-03T12:44:41Z

            let allocation_size = size
                .checked_add(actual.as_usize())
                .vortex_expect("buffer capacity overflow");
            Layout::from_size_align(allocation_size, 1).unwrap_or_else(|_| {


Why do we align to one byte here?

Not we have access to a custom allocator we can get aligned memory regions directly

We likely want to over align to some well known mem pool sizes, if such a thing exists

joseph-isaacs · 2026-09-03T13:11:11Z

Signed-off-by: Nicholas Gates <nick@nickgates.com>

joseph-isaacs · 2026-09-03T14:34:40Z

are those perf changes real?

robert3005 · 2026-09-03T14:38:31Z

looks like we are allocating then we are aligning and that's majority of the time

gatesn · 2026-09-03T18:10:39Z

That's what I meant @joseph-isaacs that passing a real alignment is slower

Signed-off-by: Nicholas Gates <nick@nickgates.com>

Reject zero-sized buffer elements at construction with compile-time assertions. ZST support was previously inconsistent; rejecting these types prevents division-by-zero and pointer-distance failures without adding iterator or growth special cases. Empty buffers of non-ZST types remain supported. Six compile-fail doctests cover the independent construction paths. Remove the generic raw-owner constructor. Typed Vec adoption takes its pointer and element count directly from the Vec and retains that Vec for destruction. External byte owners continue to use `Bytes::from_owner` at the call site, without adding a buffer API. Address the allocator-stack review comments with field documentation, an explanation of the optional allocator, and tests for owner lifetime, allocator retention, empty/shared ownership, sliced-buffer growth, and alignment. The allocator representation, buffer layout, alignment policy, and allocation benchmarks are retained. Both iterator-extension implementations and the owned iterator match the merged baseline. This is a focused alternative to #9745. Validation: - `cargo nextest run -p vortex-buffer --all-features`: 874 tests passed. - `cargo test --doc -p vortex-buffer --all-features`: 8 ordinary and 6 compile-fail doctests passed. - `cargo clippy -p vortex-buffer --all-targets --all-features`: passed. - Nightly formatting and `git diff --check`: passed. - Workspace clippy was blocked locally by the CUDA nvCOMP download; full CI is running on this revision. Performance validation: - Three serial alternating local comparisons against the allocator-stack baseline place all 20 measured interleave, batched i8 filter, and multiplication cases within 2% of baseline. - The final Graviton NEON run matches develop: u64 multiplication 15.1 vs 15.3 µs; i64 and 16,384-row multiplication 17.1 vs approximately 17.3 µs. The earlier NEON slowdowns disappeared with the ZST ban. - The complete final CodSpeed report passed with no reported regressions. --------- Signed-off-by: Nicholas Gates <nick@nickgates.com>

fix(buffer): address allocator review follow-ups

2324682

Signed-off-by: Nicholas Gates <nick@nickgates.com>

gatesn commented Sep 3, 2026

View reviewed changes

gatesn marked this pull request as ready for review September 3, 2026 11:59