Improve take_bytes perf in the null cases between 10-25%#9625
Draft
AdamGS wants to merge 3 commits intoapache:mainfrom
Draft
Improve take_bytes perf in the null cases between 10-25%#9625AdamGS wants to merge 3 commits intoapache:mainfrom
take_bytes perf in the null cases between 10-25%#9625AdamGS wants to merge 3 commits intoapache:mainfrom
Conversation
AdamGS
force-pushed
the
adamg/faster-take-bytes
branch
from
a41e896 to
57cacc4
Compare
Contributor
Author
|
Running the |
Contributor
|
run benchmark take_kernels |
Dandandan
reviewed
Mar 30, 2026
arrow-select/src/take.rs
Outdated
| T::Offset::from_usize(capacity) | ||
| .ok_or_else(|| ArrowError::OffsetOverflowError(capacity))?, | ||
| ); | ||
| ranges.push((start, end)); |
Contributor
There was a problem hiding this comment.
Won't for this case be faster to just keep the previous implementation?
Contributor
There was a problem hiding this comment.
we can use the unsafe if it helps
Contributor
Author
There was a problem hiding this comment.
yeah it seems to be more consistently ~5% faster locally, I've used unsafe in what I think is a sound way
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/faster-take-bytes (57cacc4) to aa9432c (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
AdamGS
force-pushed
the
adamg/faster-take-bytes
branch
from
b6143ff to
089c9a9
Compare
Contributor
|
run benchmark take_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/faster-take-bytes (089c9a9) to aa9432c (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
vs 089c9a9 Earlier one looks slightly better, I'll run once more |
Contributor
|
run benchmark take_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/faster-take-bytes (089c9a9) to aa9432c (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
jhorstmann
reviewed
Mar 31, 2026
arrow-select/src/take.rs
Outdated
| capacity: usize, | ||
| values: &mut Vec<u8>, | ||
| ) { | ||
| values.reserve(capacity); |
Contributor
There was a problem hiding this comment.
What is the expected behavior if the vec is non-empty? Currently its a bit inconsistent in reserving additional capacity, but then overwriting any existing content. I think appending might make sense, which means the assert and dst pointer below should be based on Vec::spare_capacity_mut.
Contributor
Author
There was a problem hiding this comment.
yeah this might be a bit confusing, this being a function is a remanent of a previous iteration but I might just inline it which I think makes it clearer.
Signed-off-by: Adam Gutglick <adam@spiraldb.com>
Contributor
Author
|
I've made both branches use |
Contributor
|
run benchmark take_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing adamg/faster-take-bytes (1b3d1fa) to aa9432c (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
regressions in "take stringview" might be significant, I'll look into it. |
Contributor
|
Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look |
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
5 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?
Rationale for this change
Just improves performance, I was profiling some things downstream and got curious about how it works.
What changes are included in this PR?
The main idea is to use a two-pass approach:
copy_byte_ranges)This PR also reduces the branching from 4 (one for each nullability combination) to only two.
Are these changes tested?
Existing tests
Are there any user-facing changes?
None