Open
Conversation
Contributor
Author
|
run benchmark take_kernels |
|
🤖 |
Dandandan
changed the title
Contributor
Author
|
run benchmark interleave_kernels |
|
🤖: Benchmark completed Details
|
|
🤖 |
Contributor
Author
|
Hmm this looks actually worse on this machine (probably x86 vs ARM...)... |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
run benchmark take_kernels |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
run benchmark interleave_kernels |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
@comphead @andygrove FYI as you seem to be interested in sort performance (which is one of the consumers) |
Contributor
Author
|
@mbutrovich as well :) |
Contributor
|
I'm curious what a SIMD gather (AVX2, SVE) intrinsics' performance is like vs. unrolling manually on modern architectures. The former likely ends up as similar μops anyway. I don't love specializing the code like this (in either direction, SIMD intrinsics or unrolling especially with ARM increasingly prevalent in the cloud) but if today it's faster I'm okay with it. It's also a pretty well-understood technique at this point. |
comphead
reviewed
Mar 12, 2026
| let chunks = indices.chunks_exact(8); | ||
| let remainder = chunks.remainder(); | ||
| for chunk in chunks { | ||
| let v0 = arrays[chunk[0].0].value(chunk[0].1); |
Contributor
There was a problem hiding this comment.
thinking aloud, is 8 a constant? does it depend on number of cpu/threads, etc?
Contributor
Author
There was a problem hiding this comment.
It just a loop unrolling constant - the higher the more code it will generate inside the loop so it can hide more latency by running/pipelining the instructions not depending on the loads.
Contributor
There was a problem hiding this comment.
yeah, thats what I was thinking if user may want to modify this value depending on their hardware/OS setup?
Or if someone would like to benchmark take or interleave with different chunks values, they would have to rewrite the method respectively? maybe some macro could help? it can be done as follow up if needed
Contributor
Author
There was a problem hiding this comment.
Ah like that - yeah that should be possible (would require some hardware setup) although I think it would not be really to a user setting compile time constants.
On the benchmarks it looks to be beneficial for both aarch64 and x86 CPUs - getting ~23% on M1 (my laptop) and -30% on intel (the @alamb-ghbot runner).
comphead
reviewed
Mar 12, 2026
| .collect::<Vec<_>>(); | ||
| for idx in remainder { | ||
| // SAFETY: base < len == output capacity | ||
| unsafe { dst.add(base).write(arrays[idx.0].value(idx.1)) }; |
Contributor
There was a problem hiding this comment.
for unsafe calls, would it make sense to debug_assert safety requirements to catch unexpected things on CI debug builds?
Contributor
Author
Agreed. Auto-vectorization is preferred and in an ideal scenario we don't need to specialize the code and write our APIs so LLVM can generate good code automatically. Ideally I think, the I am thinking perhaps we can at least remove the bounds checks for |
Contributor
Author
|
run benchmark interleave_kernels |
This reverts commit fc1adb9.
Contributor
Author
Oh wait that is a not so useful idea as we get them for all of the rows (so it will be more expensive to check upfront than it is to do "on the go") |
|
🤖 |
|
🤖: Benchmark completed Details
|
comphead
approved these changes
Mar 13, 2026
Contributor
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks @Dandandan numbers look good to me, looking fwd to run DF TPC* benchmarks
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?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?