perf : avoided repeated bounds checks in take kernel - #10441
Conversation
6b46564 to
1e0e5a4
Compare
|
I may have mis-understood #8879 then. |
take kernel
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - works towards #8879. # Rationale for this change see #8879 (comment) the point of these benchmarks are to measure the change of #10441 <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> # What changes are included in this PR? adds benchmarks for `take_record_batch()` . <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> # Are these changes tested? n/a <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> # Are there any user-facing changes? no <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
|
Update to get #10442 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as outdated.
This comment was marked as outdated.
yup like @Jefffrey mentioned this actually adds an extra validation step so there isn't much performance gains. maybe what #8879 was referring to was actually the bounds checks in each of the |
5656bd2 to
c27953d
Compare
c27953d to
1a626c5
Compare
| // SAFETY: idx is a valid index in indices.nulls() --> idx<indices.len() | ||
| if values.value(unsafe { indices.value_unchecked(idx).as_usize() }) { | ||
| // SAFETY: caller guarantees all valid indices are in-bounds | ||
| if unsafe { values.value_unchecked(indices.value_unchecked(idx).as_usize()) } { |
There was a problem hiding this comment.
im not sure we can do this, since callers can call take without an unsafe block and with check bounds off and pass in an invalid index; this would cause UB via safe interfaces i believe?
There was a problem hiding this comment.
I agree -- though it might be faster to validate the indices once at the start of take_impl and then use the unchecked accessors
We could also potentially add a take_unchecked variant or something to skip the acces check
aah, I think ive come full circle to understand what @Dandandan meant in
I can update the PR to introduce a new unsafe method that uses unsafe array access methods. The main issue with this would be the code bloat. |
|
out of curiosity could you run the benchmarks @Jefffrey. if the perf difference is < +10% This may not even be worth it |
|
run benchmark take_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
alamb
left a comment
There was a problem hiding this comment.
Thanks for pushing on this @Rich-T-kid
|
10% performance on |
i'll continue to look into this 🚀 |
|
I noticed this benchmarks run #10441 (comment) doesn't include the new benchmarks from #10442 for some reason 🤔 |
|
run benchmark take_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
6dc9944 to
6181df3
Compare
|
2947869 compared to main we should also run adriabot again. This PR (2947869) threads a generic parameter through a couple of functions. It's just a simple flag that determines whether value_unchecked() is used instead of array[index]. This was the most minimal change I could think of without duplicating code. The approach has zero overhead for the current safe code path due to Rust's monomorphization, each const gets replaced with its true/false branch, and dead code analysis removes the alternative branch, meaning there won't be any actual repeated comparisons within the functions. |
|
run benchmark take_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
run benchmark take_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
seems to be quite some real regressions? |
|
yea the benchmarks do not look good. even the targeted my understanding of rust's monomorphization may be incorrect. I'll keep looking into it |


Which issue does this PR close?
takekernels #8879.Rationale for this change
see #8879 (comment) and #8879 (comment)
What changes are included in this PR?
instead of checking length bound for each array we only do this for the first array and then remaining arrays avoid validation. The rational behind this is that record batches are all of the same size.
Are these changes tested?
logically nothing has changed
Are there any user-facing changes?
no