perf(filter): replace bit-at-a-time null bitmap filtering - #11055
Rich-T-kid wants to merge 5 commits into
Conversation
…evel PEXT The IndexIterator/Indices paths in filter_bits walked the precomputed indices Vec (up to 256 KB at 50% selectivity) to read the null bitmap one byte at a time via get_bit_raw. This caused severe cache pressure against the values buffer being filtered simultaneously. Replace with gather_bits: zip 64-bit chunks from the filter and source bitmaps, apply software PEXT to extract selected bits per chunk. This works directly on the two 8 KB bitmaps, eliminating the indices Vec traversal and reducing source reads from O(count) byte loads to O(filter_len/64) u64 loads. A threshold (count * 64 < filter_len) keeps the original indices path for very sparse selections where the tiny precomputed Vec is cheaper to walk than scanning all filter chunks. Benchmark delta on existing NULLs benchmarks (65536 elements): i32 w NULLs kept 1/2: 149.9 µs -> 38.1 µs (-74%) i32 w NULLs high selectivity: 17.9 µs -> 6.6 µs (-63%) i32 w NULLs low selectivity: 499 ns -> 276 ns (-60%) u8 w NULLs kept 1/2: 159.0 µs -> 63.4 µs (-75%) u8 w NULLs high selectivity: -- -> 2.8 µs (-58%) u8 w NULLs low selectivity: -- -> 234 ns (-35%)
|
run benchmark filter_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-t-kid/filter-bits-gather-optimization (75281be) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench filter_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing rich-t-kid/filter-bits-gather-optimization (75281be) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
run benchmark filter_kernels |
1 similar comment
|
run benchmark filter_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-t-kid/filter-bits-gather-optimization (200598b) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench filter_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-t-kid/filter-bits-gather-optimization (200598b) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench filter_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing rich-t-kid/filter-bits-gather-optimization (200598b) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing rich-t-kid/filter-bits-gather-optimization (200598b) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
c275451 to
4727e6a
Compare
|
run benchmark filter_kernels |
1 similar comment
|
run benchmark filter_kernels |
|
@sdf-jkl could you take a look 🚀 |
Rich-T-kid
changed the title
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-t-kid/filter-bits-gather-optimization (4727e6a) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench filter_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-t-kid/filter-bits-gather-optimization (4727e6a) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench filter_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing rich-t-kid/filter-bits-gather-optimization (4727e6a) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing rich-t-kid/filter-bits-gather-optimization (4727e6a) to 2078680 (merge-base) diff Run configurationrun benchmark filter_kernels
env:
BENCH_FILTER: "NULL"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
| // SAFETY: indices were derived from the filter predicate | ||
| let bits = indices.iter().map(|src_idx| unsafe { | ||
| bit_util::get_bit_raw(buffer.values().as_ptr(), *src_idx + offset) | ||
| }); | ||
| // SAFETY: `Vec::iter()` reports its size correctly |
There was a problem hiding this comment.
going to add these safety comments back in
There was a problem hiding this comment.
yeah defiitely important to add back in
|
I'll take a look this weekend |
|
|
||
| /// Collects the bits of `val` wherever `mask` is 1, packed into the low bits of the result. | ||
| #[inline(always)] | ||
| fn pext64(val: u64, mut mask: u64) -> u64 { |
There was a problem hiding this comment.
There already is an implementation of PEXT in the codebase --
arrow-rs/parquet/src/util/bit_util.rs
Lines 959 to 984 in 4727e6a
Yours seems to perform better though (on my machine 🤓 ) If anything we can drop drop the parquet one and make it import your implementation.
Rust has a nightly impl-- doc.rust-lang.org/std/primitive.u64.html#method.extract_bits
The issue here - rust-lang/rust#149069 - explains that the implementation is waiting on supporting the new LLVM intrinsics that support automatically using the PEXT/PDEP machine instructions if machine supports them. When the instructions are available the perf is pretty epic.
There was a problem hiding this comment.
yea I think moving this out so it can be re-used is a good idea. Would be nice to see how much of a speed up parquet gets from this as well
There was a problem hiding this comment.
arrow-buffer/src/util/bit_util.rs is where @devanbenz put it
| } | ||
| }; | ||
|
|
||
| for (filter_word, src_word) in filter_chunks.iter().zip(src_chunks.iter()) { |
There was a problem hiding this comment.
Processing each word is independent from each other so this could be paralellizable.
I killed some time on it today. You can take a look here -- #11086
There was a problem hiding this comment.
a great follow on task perhaps
There was a problem hiding this comment.
I agree, can open up a follow on PR
|
@sdf-jkl your PR seems to be faster than this one, should I close this in favor of that one? |
|
Mine was just pathfinding and only better when SIMD enabled, not on scalar. You can port smth / take inspiration from my PR here. |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This is very neat -- thank you @Rich-T-kid @sdf-jkl and @devanbenz
It will be pretty amazing to get 75% faster on some filter kernels 🤯
I'll keep an eye on this one
FYI @jhorstmann and @hhhizzz you may be interested too
| } | ||
| }; | ||
|
|
||
| for (filter_word, src_word) in filter_chunks.iter().zip(src_chunks.iter()) { |
There was a problem hiding this comment.
a great follow on task perhaps
|
|
||
| /// Collects the bits of `val` wherever `mask` is 1, packed into the low bits of the result. | ||
| #[inline(always)] | ||
| fn pext64(val: u64, mut mask: u64) -> u64 { |
There was a problem hiding this comment.
arrow-buffer/src/util/bit_util.rs is where @devanbenz put it
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
it might also make sense to ensure the benchmarks cover this case as well (clearly some do but maybe not all)
| // SAFETY: indices were derived from the filter predicate | ||
| let bits = indices.iter().map(|src_idx| unsafe { | ||
| bit_util::get_bit_raw(buffer.values().as_ptr(), *src_idx + offset) | ||
| }); | ||
| // SAFETY: `Vec::iter()` reports its size correctly |
There was a problem hiding this comment.
yeah defiitely important to add back in
|
@alamb @sdf-jkl @devanbenz could you take another look |
f83ffde to
fa5badb
Compare
fa5badb to
9f60687
Compare
|
Hi @Rich-T-kid and @alamb this seems very similar to the PR I have here: https://github.com/apache/arrow-rs/pull/10136/changes 🤔 If we are planning on going with your implementation here it may be a good idea to pull over some of the benchmarking I did in mine and verify that sparse and dense bitmaps are accounted for -- specifically using PEXT, I had to run the benchmarks on my local x86 machine since the GHA CI running CPU is ARM based. I was finding performance issues using pext when a bitmap was either of the two. It wasn't much of a performance hit just a few percentage points, but still. |
|
@devanbenz our PR's are very similar but I think the focus of both a separate. this PR was meant to be a more narrowed optimization. |
5 participants
Which issue does this PR close?
Rationale for this change
When filtering arrays with a null bitmap, the current code walks one bit at a time
What changes are included in this PR?
Replaces the bit-at-a-time null bitmap loop with a word-level approach: instead of reading one bit per selected row, we load 64 filter bits and 64 source bits at a time, extract only the bits where the filter is set, and pack them directly into the output buffer.
Are these changes tested?
yes, existing test
Are there any user-facing changes?
no