GH-50645: [C++][Compute] Optimize min/max_element_wise with word at a time validity handling#50647
Conversation
Reranko05
left a comment
Reranko05
left a comment
There was a problem hiding this comment.
Can you run python -m pre_commit run clang-format --files cpp/src/arrow/compute/kernels/scalar_compare.cc cpp/src/arrow/compute/kernels/scalar_compare_test.cc to fix the formatting issues?
|
I ran it and see that the C++ Format has passed and the rest were skipped. Is there something you noticed that I should reformat? |
…d at a time validity handling
e35f877 to
f1f7743
Compare
|
I re-ran it locally and realized the reported formatting issue isn't related to the changes in this PR. It looks like it's a pre-existing issue elsewhere in the file. Sorry for the confusion. Your formatting is fine. |
| bool have_seed = false; | ||
| OutValue seed{}; |
| auto left_reader = ::arrow::internal::BitmapUInt64Reader( | ||
| left_valid, left_valid ? left_offset : 0, left_valid ? length : 0); | ||
| auto right_reader = ::arrow::internal::BitmapUInt64Reader( | ||
| right_valid, right_valid ? right_offset : 0, right_valid ? length : 0); |
There was a problem hiding this comment.
This is re-implementing logic that is already exposed as e.g. VisitTwoSetBitRuns. I expect you could massively simplify this code by using existing facilities.
| const uint64_t right_word = right_valid ? right_reader.NextWord() : ~uint64_t(0); | ||
| const uint64_t out_word = | ||
| skip_nulls ? (left_word | right_word) : (left_word & right_word); | ||
| // out_valid is allocated at bit offset 0, so store the word directly |
There was a problem hiding this comment.
You could compute the output bitmap separately using either BitmapAnd or BitmapOr.
| // output bitmap is needed. Where the array is null the accumulator already | ||
| // holds the right value | ||
| if (!acc_valid && skip_nulls) { | ||
| auto reader = ::arrow::internal::BitmapUInt64Reader(arr_valid, arr.offset, length); |
There was a problem hiding this comment.
Can use VisitSetBitRuns to avoid reimplementing this.
| OutputArrayWriter<OutType> writer(&out_span); | ||
| ArrayIterator<OutType> out_it(out_span); | ||
| int64_t index = 0; | ||
| VisitArrayValuesInline<OutType>( |
There was a problem hiding this comment.
Perhaps you could simply have rewritten this using VisitTwoSetBitRuns :-)
github-actions
Bot
added
awaiting committer review
4 participants
Rationale for this change
The variadic min_element_wise / max_element_wise kernels (ScalarMinMax in scalar_compare.cc) fold arguments using a multi pass approach: an antiextreme sentinel fill, a separate validity bitmap pass (BitmapOr/BitmapAnd), and a per element value pass with a per-element validity branch.
This can be rewritten as a single word at a time pass (handling validity 64 bits at a time) that fuses validity and value computation and keeps fully-valid words branchless. That removes the redundant passes and the per-element mispredicting branch, giving a measured ~2-7x speedup on numeric types, with no behavioral change.
What changes are included in this PR?
Rewrote the numeric ScalarMinMax fold to a word at a time algorithm (CombineWordwise / CombineArrays / FoldArrayIntoOutput), with an all-valid branchless fast path and an all null skip.
Added a random, sliced/offset test (MinMaxElementWiseRandom) covering the new word loop across null densities and offsets.
Benchmarks
archery benchmark diff(this branch vsmain), 5 reps; throughput, change %:Are these changes tested?
Yes. All existing ElementWise tests pass, plus a new MinMaxElementWiseRandom typed test that cross checks the kernel against a reference across lengths (65/128/200), null densities (0–100%), offsets (0/1/3/7), both skip_nulls, and min/max, over all numeric types.
Are there any user-facing changes?
No
AI Usage
Parts of the code, tests, and this description were written with AI assistance.
Closes #50645