Skip to content

Use cursor copies for PiecewiseSequence slices#8832

Draft
danking wants to merge 5 commits into
codex/piecewise-sequence-varbin-takefrom
codex/piecewise-sequence-cursor-copy
Draft

Use cursor copies for PiecewiseSequence slices#8832
danking wants to merge 5 commits into
codex/piecewise-sequence-varbin-takefrom
codex/piecewise-sequence-cursor-copy

Conversation

@danking

@danking danking commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a shared checked cursor-copy helper for PiecewiseSequence slice gathering
  • replace extend_from_slice in the primitive, decimal, VarBinView, and VarBin PiecewiseSequence consumers
  • keep the safety checks explicit: each copy verifies the destination cursor range against the declared output length and capacity, and callers verify the final cursor before set_len

Validation

  • cargo +nightly fmt --all
  • cargo clippy -p vortex-array --all-targets --all-features -- -D warnings
  • cargo bench -p vortex-array --bench listview_rebuild
  • cargo bench -p vortex-array --bench cast_decimal
  • cargo test -p vortex-array piecewise_sequence

@codspeed-hq

codspeed-hq Bot commented Jul 17, 2026

Copy link
Copy Markdown

Merging this PR will regress 2 benchmarks

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 1795 untouched benchmarks
⏩ 44 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation compact[(4096, 90)] 780.3 ns 867.8 ns -10.08%
Simulation true_count_vortex_buffer[128] 522.2 ns 580.6 ns -10.05%
Simulation copy_nullable[65536] 1.4 ms 1 ms +31.91%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing codex/piecewise-sequence-cursor-copy (87f6283) with codex/piecewise-sequence-varbin-take (5d31540)

Open in CodSpeed

Footnotes

  1. 44 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.

@danking
danking force-pushed the codex/piecewise-sequential-run-take-consumers branch from 9adf56d to c4a0358 Compare July 17, 2026 19:40
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from f9a3472 to afcc04a Compare July 17, 2026 19:41
@danking
danking force-pushed the codex/piecewise-sequential-run-take-consumers branch from c4a0358 to c76531d Compare July 17, 2026 20:05
@danking danking added the changelog/performance A performance improvement label Jul 17, 2026
@danking
danking force-pushed the codex/piecewise-sequential-run-take-consumers branch from 80f7b89 to 30870aa Compare July 17, 2026 20:19
@danking
danking force-pushed the codex/piecewise-sequential-run-take-consumers branch from 30870aa to e777ab4 Compare July 17, 2026 20:25
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from afcc04a to 1a17030 Compare July 17, 2026 20:29
@danking
danking changed the base branch from codex/piecewise-sequential-run-take-consumers to codex/piecewise-sequence-varbin-take July 17, 2026 20:29
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch from 5924fe3 to 79d9e10 Compare July 17, 2026 20:38
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from 1a17030 to 534b70f Compare July 17, 2026 20:39
Comment thread vortex-array/src/arrays/decimal/compute/take.rs Outdated
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch from 79d9e10 to 39ee98e Compare July 20, 2026 16:11
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch 2 times, most recently from 3be0c74 to e60ec21 Compare July 20, 2026 16:37
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch 2 times, most recently from 83c466d to df091fb Compare July 20, 2026 16:56
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch 2 times, most recently from 423876b to 7034fd3 Compare July 20, 2026 18:10
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch 2 times, most recently from e31db3e to 9f02349 Compare July 20, 2026 19:24
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from 7034fd3 to bdfe8f7 Compare July 20, 2026 19:24
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch from 9f02349 to 43ecd40 Compare July 20, 2026 19:42
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from bdfe8f7 to 9c21b5e Compare July 20, 2026 19:42
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch from 43ecd40 to 97a2e5a Compare July 20, 2026 19:57
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from 9c21b5e to b2069a5 Compare July 20, 2026 19:58
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch from 97a2e5a to 4626cfc Compare July 20, 2026 20:09
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from 497ff39 to 5ca0608 Compare July 20, 2026 20:09
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch from 4626cfc to 9eba049 Compare July 20, 2026 20:13
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from 5ca0608 to bfd4825 Compare July 20, 2026 20:13
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch from 9eba049 to 3c5e519 Compare July 20, 2026 20:16
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from bfd4825 to 1af09ed Compare July 20, 2026 20:17
/// This is useful when the caller knows the final output length up front and wants to avoid
/// repeatedly growing the initialized length while copying contiguous source slices.
pub struct SpareBufferWriter<'a, T> {
buffer: &'a mut BufferMut<T>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you just want here &'a mut [T] unless I am missing something

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah no you need to setlen

///
/// This is useful when the caller knows the final output length up front and wants to avoid
/// repeatedly growing the initialized length while copying contiguous source slices.
pub struct SpareBufferWriter<'a, T> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think before we put this in vortex-buffer we would need to benchmark this against other approaches. We can keep it internal to the take impl and figure out a right abstraction from there. My suggestions were not fully tested

danking added 3 commits July 20, 2026 18:03
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
@danking
danking force-pushed the codex/piecewise-sequence-varbin-take branch from 3c5e519 to 5d31540 Compare July 20, 2026 22:04
@danking
danking force-pushed the codex/piecewise-sequence-cursor-copy branch from 9183265 to 182567a Compare July 20, 2026 22:04
claude added 2 commits July 20, 2026 22:53
Replace the raw-pointer cursor with a written-count cursor over
spare_capacity_mut, and remove copy_slice_unchecked along with the
unsafe blocks at the four PiecewiseSequence call sites. The bounds
check is one well-predicted branch per slice: benchmarking the gather
loop over 16M u64 elements shows checked copies within noise of
unchecked ones at every run length (212ms vs 200ms even at the
pathological run length of 1, where both beat extend_from_slice's
299ms), so the unchecked variant's extra unsafe surface isn't paying
for anything.

The writer is now #[must_use], and the remaining unsafety is contained
to the copy into the bounds-checked spare-capacity range (the
not-yet-stable <[MaybeUninit<T>]>::write_copy_of_slice) and the single
set_len in finish, justified by the writer's contiguous-prefix
invariant.

Signed-off-by: "Robert" <robert@spiraldb.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GUSY34eRKq9du5zQK1FuZq
The writer only exists to serve the PiecewiseSequence gather loops, so
keep it pub(crate) in arrays::piecewise_sequence as its own submodule
rather than growing vortex-buffer's public API. It only uses BufferMut's
public surface, so nothing in vortex-buffer changes behavior.

Signed-off-by: "Robert" <robert@spiraldb.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GUSY34eRKq9du5zQK1FuZq
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/performance A performance improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants