Use interleave for fragmented zip masks#10368
Conversation
Count true runs in large array/array masks and dispatch fragmented masks to the interleave kernel while retaining MutableArrayData for scalar, short, and contiguous inputs. Extend zip benchmarks with direct interleave and long-run comparisons.
|
run benchmark zip_kernels |
| zip_impl(mask, &truthy, truthy_is_scalar, &falsy, falsy_is_scalar) | ||
| } | ||
|
|
||
| fn count_true_runs(mask: &BooleanBuffer) -> usize { |
There was a problem hiding this comment.
we should probably have some unit tests for this specific function
There was a problem hiding this comment.
Added direct count_true_runs coverage in 8c47191, including empty/all-false/all-true masks, multiple runs, 64-bit chunk boundaries with trailing padding, and sliced masks with non-zero bit offsets. cargo test -p arrow-select --all-features passes (394 unit tests and 17 doctests); formatting and clippy checks also pass.
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/zip-interleave (c4c42dc) to 305bf26 (merge-base) diff File an issue against this benchmark runner |
|
Benchmark for this request hit the 7200s job deadline before finishing. Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
The full |
|
Hi @Jo2234, thanks for the request (#10368 (comment)). Only whitelisted users can trigger benchmarks. Allowed users: Dandandan, Fokko, Jefffrey, Omega359, Rachelint, adriangb, alamb, asubiotto, brunal, buraksenn, cetra3, codephage2020, coderfender, comphead, erenavsarogullari, etseidl, friendlymatthew, gabotechs, geoffreyclaude, grtlr, haohuaijin, jonathanc-n, kevinjqliu, klion26, kosiew, kumarUjjawal, kunalsinghdadhwal, liamzwbao, mbutrovich, mkleen, mzabaluev, neilconway, rluvaton, sdf-jkl, timsaucer, xudong963, zhuqi-lucas. File an issue against this benchmark runner |
Which issue does this PR close?
Rationale for this change
zipcurrently usesMutableArrayData, which is efficient when a mask contains long runs but performs one extension per run. Fragmented array/array masks therefore pay substantial per-run overhead, while the specialized interleave kernels are faster for this shape.What changes are included in this PR?
interleaveMutableArrayDatapath for inputs shorter than 1,024 rows, scalar inputs, and masks with fewer runszipsemanticstrue_then_falsecaseThe run-count threshold is conservative: interleave is selected only above one true run per eight rows. Counting the complete bitmap also avoids sampling errors for masks whose fragmentation is unevenly distributed.
Are these changes tested?
Yes. The new tests cover dispatch decisions for short, fragmented, sparse/dense, contiguous, unevenly fragmented, and offset masks, plus end-to-end fragmented array selection with null mask and input values.
Validation:
CARGO_BUILD_JOBS=2 cargo test -p arrow-select --all-features(394 unit tests and 17 doctests passed)CARGO_BUILD_JOBS=2 cargo clippy -p arrow-select --all-targets --all-features -- -D warningsCARGO_BUILD_JOBS=2 cargo clippy -p arrow --bench zip_kernels --features=async,test_utils -- -D warningscargo +stable fmt --all -- --checkCARGO_BUILD_JOBS=2 RUSTDOCFLAGS='-D warnings' cargo doc -p arrow-select --no-depsCriterion used 30 samples, a 2-second warm-up, and a 5-second measurement. On 8,192-row array/array inputs with random 50%-true masks, the existing run path was forced by temporarily raising the local dispatch threshold; the benchmark-only control was then restored:
At the 1,024-row boundary, i32 measured 14.350 us on the forced run path versus 4.6005 us on the adaptive path. The contiguous
true_then_falsecase remains on the run-oriented path.AI assistance was used to draft parts of the adaptive dispatch, tests, and benchmark changes. The resulting diff was reviewed against the
zip,MutableArrayData,BooleanBuffer, andinterleaveimplementations; an edge-sampling flaw found during review was replaced with a complete run count and covered by a regression test. The checks and benchmark results above were rerun on the final implementation.Are there any user-facing changes?
No API or semantic changes. Fragmented array/array
zipcalls use a faster internal implementation; scalar, short, and contiguous-mask calls retain the existing path.