perf: optimize loops for primitive types binary kernels. - #10518
Draft
Anakin100100 wants to merge 2 commits into
Draft
perf: optimize loops for primitive types binary kernels. #10518Anakin100100 wants to merge 2 commits into
Anakin100100 wants to merge 2 commits into
Conversation
Contributor
|
run benchmarks arithmetic_kernels bitwise_kernel decimal_arithmetic |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing perf/arithmetic-buffer-optimization (9f4e3b3) to ed92960 (merge-base) diff Run configurationrun benchmark bitwise_kernelCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing perf/arithmetic-buffer-optimization (9f4e3b3) to ed92960 (merge-base) diff Run configurationrun benchmark decimal_arithmeticCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing perf/arithmetic-buffer-optimization (9f4e3b3) to ed92960 (merge-base) diff Run configurationrun benchmark arithmetic_kernelsCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Jefffrey
reviewed
Aug 3, 2026
|
|
||
| let buffer: Vec<_> = values.collect(); | ||
| let len = a.len(); | ||
| let byte_width = O::Native::get_byte_width(); |
Contributor
There was a problem hiding this comment.
binary: replaced iterator collect with a direct loop which LLVM can better optimize and avoids option checks. The loop overhead here is a large part of the runtime when the underlying op is simple.
this seems surprising to me since as we were iterating+zipping over values() there wouldnt be any option checks going on
| F: Fn(A::Item, B::Item) -> Result<O::Native, ArrowError>, | ||
| { | ||
| let mut buffer = MutableBuffer::new(len * O::Native::get_byte_width()); | ||
| let byte_width = O::Native::get_byte_width(); |
Contributor
There was a problem hiding this comment.
does this get better performance than simply using a vec here?
Comment on lines
+299
to
300
| let values = Buffer::from(buffer).into(); | ||
| Ok(PrimitiveArray::new(values, Some(nulls))) |
Contributor
There was a problem hiding this comment.
Suggested change
| let values = Buffer::from(buffer).into(); | |
| Ok(PrimitiveArray::new(values, Some(nulls))) | |
| Ok(PrimitiveArray::new(buffer.into(), Some(nulls))) |
alamb
marked this pull request as draft
August 4, 2026 17:43
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
4 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?
Part of #10245
Rationale for this change
Improves performance of the arithmetic kernels operating on primite types.
What changes are included in this PR?
I read the issue and focused on try_binary and discovered a few other optimizations there. All mentions of benchmarks here refer to decimal_arithmetic benchmark which covers addition and subtraction on
binary: replaced iterator collect with a direct loop which LLVM can better optimize and avoids option checks. The loop overhead here is a large part of the runtime when the underlying op is simple.
try_binary: in the null path uses Vec directly to create the buffer instead of going through the BufferBuilder. I tested it with a benchmark with 10-90% nulls and there is 5-9% improvement to the overall runtime. I don't think there's reason to include this benchmark in the library because it's very specific to this path and not general enough to be useful in the future.
try_binary_no_nulls: the typed array slice write can be vectorized more easily, Independent of that I verified that removing the no-inline annotation increases performance by additional 5% which makes sense with a more direct loop.
On average these changes increase the throughput of the kernels by 26-27% on same scale and 14-15% of different scale benchamrks for 32 and 64 bit types because the loop is a smaller fraction in the second case.
decimal32_equal_scale/sub time: [917.00 ns 923.95 ns 932.62 ns] thrpt: [1.0980 Gelem/s 1.1083 Gelem/s 1.1167 Gelem/s] change: time: [−23.032% −21.954% −20.901%] (p = 0.00 < 0.05) thrpt: [+26.424% +28.130% +29.924%] decimal64_different_scale/add time: [1.9084 µs 1.9212 µs 1.9366 µs] thrpt: [528.77 Melem/s 533.00 Melem/s 536.59 Melem/s] change: time: [−15.134% −14.025% −12.902%] (p = 0.00 < 0.05) thrpt: [+14.813% +16.313% +17.833%]On 128 and 256 bit types there is around 5% speedup because of more complex math behind the underlying op.
Are these changes tested?
Yes, already covered by tests.
Are there any user-facing changes?
No