Repository navigation
Reuse buffers when interleaving byte views - #11281
Merged
Merged
Conversation
Contributor
Author
|
run benchmark interleave_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing reuse_interleave_view_buffers (e712522) to 74b630b (merge-base) diff Run configurationrun benchmark interleave_kernelsBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench interleave_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing reuse_interleave_view_buffers (e712522) to 74b630b (merge-base) diff Run configurationrun benchmark interleave_kernelsCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
Summary of benchmark results across both architectures:
No regressions in the rest of |
Jefffrey
approved these changes
Sep 29, 2026
|
|
||
| // contains the mapping from old buffer index to new buffer index | ||
| let mut buffer_to_new_index = vec![None; total_buffers]; | ||
| let mut buffer_to_new_index = vec![UNASSIGNED; total_buffers]; |
Contributor
There was a problem hiding this comment.
is switching from option to a sentinel value a performance optimization or ergonomic change?
Contributor
Author
There was a problem hiding this comment.
This was left over from a bit of churn. I tried about 5 different approaches to get this shape.
However, this does add a speed bump, since an Option<u32> is 8 bytes vs 4
| /// buffer, so this checks by identity to ensure it is only emitted once | ||
| #[inline(never)] | ||
| fn push_buffer( | ||
| seen: &mut BTreeMap<(*const u8, usize), u32>, |
Contributor
There was a problem hiding this comment.
a minor optimization could be to hash only on pointer then always accept the buffer with larger len (which means retroactively changing the buffer in buffers), but i suppose thats more trouble than its worth for presumably a rare case 🤔
- and probably less performant
Jefffrey
changed the title
Contributor
|
thanks @cetra3 |
cetra3
added a commit
to pydantic/arrow-rs
that referenced
this pull request
Oct 2, 2026
# Which issue does this PR close? Fixes apache#11280 And also helps with: apache/datafusion#23565 # Rationale for this change See the original issue, but, basically the interleave kernel can make sub-optimal string views especially when buffers are shared. # What changes are included in this PR? We use a btreemap on the seen buffers (not individual values) and use that to prevent us from cloning the same buffer multiple times. # Are these changes tested? Yes tested, and locally benchmarks have shown a moderate speed increase in some of the interleave benchmarks, on my dev machine which has an **AMD Ryzen 9 9950X3D** processor: | benchmark | main | this PR | change | |-----------------------------------------------------------------------|----------|----------|--------| | interleave str_view(0.0) 100 [0..100, 100..230, 450..1000] | 284.5 ns | 268.6 ns | -5.6% | | interleave str_view(0.0) 400 [0..100, 100..230, 450..1000] | 706.9 ns | 563.9 ns | -20.2% | | interleave str_view(0.0) 1024 [0..100, 100..230, 450..1000] | 1.717 µs | 1.250 µs | -27.2% | | interleave str_view(0.0) 1024 [0..100, 100..230, 450..1000, 0..1000] | 1.783 µs | 1.274 µs | -28.6% | # Are there any user-facing changes? No user facing changes, just an optimization. (cherry picked from commit 007894e)
alamb
added a commit
to alamb/datafusion
that referenced
this pull request
Oct 16, 2026
* `interleave` no longer repeats a shared data buffer for byte view arrays (apache/arrow-rs#11281), so the spill `compact_view_array` tests build the repeated-buffer array by hand * Casting a naive timestamp to a named time zone now resolves DST-ambiguous and nonexistent local times like PostgreSQL instead of erroring (apache/arrow-rs#11038): update `timestamps_timezone.slt` Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Fixes #11280
And also helps with: apache/datafusion#23565
Rationale for this change
See the original issue, but, basically the interleave kernel can make sub-optimal string views especially when buffers are shared.
What changes are included in this PR?
We use a btreemap on the seen buffers (not individual values) and use that to prevent us from cloning the same buffer multiple times.
Are these changes tested?
Yes tested, and locally benchmarks have shown a moderate speed increase in some of the interleave benchmarks, on my dev machine which has an AMD Ryzen 9 9950X3D processor:
Are there any user-facing changes?
No user facing changes, just an optimization.