Continued: Use Arc<[Buffer]> instead of raw Vec<Buffer> in GenericByteViewArray for faster slice #7773
Conversation
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
|
In follow-up work, I want to expose the ViewBuffers representation to the kernels in arrow-select. The |
Filter is another obvious candidate |
This comment was marked as outdated.
This comment was marked as outdated.
alamb
added
the
next-major-release
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
I am not quite sure what to make of the benchmark results so far -- some of them may be noise -- we'll have to look into that further |
|
I am starting to check this one out in more detail |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
I tried to reproduce the benchmarks for the concatenate_kernel These benchmarks seem to show 10-20% slowdown: I used this command cargo bench --bench concatenate_kernel -- "utf8_view max_str_len=20 null_density=0"I ran it on main and then on this branch and I didn't see any significant difference. Maybe some of the subsequent cleanups since when I last ran benchmarks will make sense. Lets see what the next round says Details of run on `main`
Details of run on `arc-buffers`
|
|
🤖: Benchmark completed Details
|
|
🤖 |
|
Slice certainly looks faster now 🎉 🤔 it still seems concat shows some regression However, I can't reproduce it on my Mac M3 -- maybe it is something to do with the benchmark machine (which is an x86) -- I'll try and reproduce there later today |
alamb
reviewed
Jun 27, 2025
| let len = array.len(); | ||
| array.buffers.insert(0, array.views.into_inner()); | ||
| let new_buffers = { |
There was a problem hiding this comment.
I wonder if this code is doing an extra allocation (namely it is now making a new Vec::with_capacity rather than reusing the previous buffers as it was before)
There was a problem hiding this comment.
I can reproduce the slowdown for the concat kernel on my laptop.
For this allocation, I believe we in turn save doing this allocation during clone() when using Arc<ViewBuffers>...
There was a problem hiding this comment.
Nevermind, it appears that I can't consistently reproduce the slowdown.
| @@ -609,8 +617,9 @@ impl<T: ByteViewType + ?Sized> Array for GenericByteViewArray<T> { | |||
|
|
|||
| fn shrink_to_fit(&mut self) { | |||
| self.views.shrink_to_fit(); | |||
| self.buffers.iter_mut().for_each(|b| b.shrink_to_fit()); | |||
| self.buffers.shrink_to_fit(); | |||
| if let Some(buffers) = Arc::get_mut(&mut self.buffers.0) { | |||
There was a problem hiding this comment.
I think this changes the semantics slightly -- and it only shrinks the buffers if they aren't shared
Maybe it should be using Arc::make_mut 🤔
There was a problem hiding this comment.
The underlying Buffers are reference counted too, and only shrink themselves if the reference count is 1. So while this does change the semantics slightly, I don't think it changes as much in practice: When the Arc is shared, the (current) alternative would be to store references to the same buffers in another Vec - thus incrementing the reference counts on the underlying buffers and making their shrink_to_fit a no-op.
That's also why make_mut wont lead to more shrinking
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
alamb
reviewed
Jun 27, 2025
| /// | ||
| /// Similar to `Arc<Vec<Buffer>>` or `Arc<[Buffer]>` | ||
| #[derive(Clone, Debug)] | ||
| pub struct ViewBuffers(pub(crate) Arc<[Buffer]>); |
There was a problem hiding this comment.
One idea I had was instead of Arc<[Buffer>] what if we left it as Arc<Vec<Buffer>> so converting back/forth to Vec wasn't as costly 🤔
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR is a continuation of the work by @ShiKaiWi in #6427.
Which issue does this PR close?
StringViewArray::slice()andBinaryViewArray::slice()faster / non allocating #6408.Please consult the description of #6427 for the details.
What changed since !6427
ViewBuffersabstraction (analogous toFieldsinSchema) that hides the internal representation (Arc<[Buffer]>) in the constructors ofGenericByteViewArray.Are there any user-facing changes?
Yes, these are breaking changes.