-
Notifications
You must be signed in to change notification settings - Fork 194
perf(mask): fuse true_count into boolean operators #8425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you also read the benchmarking guidelines |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // SPDX-FileCopyrightText: Copyright the Vortex contributors | ||
|
|
||
| //! Benchmarks comparing strategies for computing a bitwise-op result *and* its `true_count`: | ||
| //! | ||
| //! * `*_two_pass`: current behaviour, materialise the result buffer then run the (vectorised) | ||
| //! `count_ones` kernel as a separate pass (`&a & &b` followed by `.true_count()`). | ||
| //! * `*_fused`: single pass that combines the words and accumulates `count_ones` together | ||
| //! (`BitBuffer::bitand_with_true_count`). | ||
|
|
||
| #![allow(clippy::unwrap_used, clippy::cast_possible_truncation)] | ||
|
|
||
| use divan::Bencher; | ||
| use divan::black_box; | ||
| use vortex_buffer::BitBuffer; | ||
|
|
||
| fn main() { | ||
| divan::main(); | ||
| } | ||
|
|
||
| // Lengths in bits, from L1-resident up to L2/L3. | ||
| const SIZES: &[usize] = &[ | ||
| 1_024, // 128 B | ||
| 8_192, // 1 KB | ||
| 65_536, // 8 KB | ||
| 524_288, // 64 KB | ||
| ]; | ||
|
|
||
| fn inputs(len: usize) -> (BitBuffer, BitBuffer) { | ||
| let lhs = | ||
| BitBuffer::from_iter((0..len).map(|i| (i.wrapping_mul(2_654_435_761) >> 13) & 1 == 0)); | ||
| let rhs = BitBuffer::from_iter((0..len).map(|i| (i.wrapping_mul(40_503) >> 7) & 1 == 0)); | ||
| (lhs, rhs) | ||
| } | ||
|
|
||
| #[divan::bench(args = SIZES)] | ||
| fn and_two_pass(bencher: Bencher, len: usize) { | ||
| let (lhs, rhs) = inputs(len); | ||
| bencher.bench_local(|| { | ||
| let result = black_box(&lhs) & black_box(&rhs); | ||
| let count = result.true_count(); | ||
| (result, count) | ||
| }); | ||
| } | ||
|
|
||
| #[divan::bench(args = SIZES)] | ||
| fn and_fused(bencher: Bencher, len: usize) { | ||
| let (lhs, rhs) = inputs(len); | ||
| bencher.bench_local(|| black_box(&lhs).bitand_with_true_count(black_box(&rhs))); | ||
| } | ||
|
|
||
| // Owned-LHS scenario: the current code can combine in-place (no allocation) then count | ||
| // separately; the fused path always allocates but does a single pass. A fresh owned, uniquely | ||
| // owned LHS is cloned per iteration so the in-place fast path is actually taken. | ||
| #[divan::bench(args = SIZES)] | ||
| fn and_owned_two_pass(bencher: Bencher, len: usize) { | ||
| let (lhs, rhs) = inputs(len); | ||
| bencher | ||
| .with_inputs(|| lhs.clone()) | ||
| .bench_local_values(|lhs| { | ||
| let result = lhs & black_box(&rhs); | ||
| let count = result.true_count(); | ||
| (result, count) | ||
| }); | ||
| } | ||
|
|
||
| #[divan::bench(args = SIZES)] | ||
| fn and_owned_fused(bencher: Bencher, len: usize) { | ||
| let (lhs, rhs) = inputs(len); | ||
| bencher | ||
| .with_inputs(|| lhs.clone()) | ||
| .bench_local_values(|lhs| lhs.bitand_with_true_count(black_box(&rhs))); | ||
| } |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a great, insight. Then use |_| {} for the original and |c| {count += c} for the counted one. The compile will drop the original unit comb function, then we can share the copy of the op.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I learned a lot here, thank you. I will merge to one
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a feeling part of your speed up is a better binary_op impl for BitBuffer would be nice to see that. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -235,6 +235,113 @@ pub(super) fn bitwise_binary_op<F: FnMut(u64, u64) -> u64>( | |
| BitBuffer::new(bytes.freeze(), len) | ||
| } | ||
|
|
||
| /// Combine `left` and `right` word-by-word with `op`, calling `comb` on each result word so a | ||
| /// caller can fold a reduction (e.g. `count_ones`) into the same pass. The word given to `comb` | ||
| /// has bits past `len` cleared; the buffer keeps `op`'s raw output in those positions. | ||
| pub(super) fn bitwise_binary_op_with<F, C>( | ||
| left: &BitBuffer, | ||
| right: &BitBuffer, | ||
| op: F, | ||
| comb: C, | ||
| ) -> BitBuffer | ||
| where | ||
| F: FnMut(u64, u64) -> u64, | ||
| C: FnMut(u64), | ||
| { | ||
| assert_eq!(left.len(), right.len()); | ||
| let len = left.len(); | ||
| if len == 0 { | ||
| return BitBuffer::empty(); | ||
| } | ||
|
|
||
| // Byte-aligned operands: logical bits map onto physical `u64` words, so we can read the backing | ||
| // bytes straight as words instead of shifting through `iter_padded`. | ||
| if left.offset().is_multiple_of(8) && right.offset().is_multiple_of(8) { | ||
| let n_bytes = len.div_ceil(8); | ||
| let l_start = left.offset() / 8; | ||
| let r_start = right.offset() / 8; | ||
| let lhs = &left.inner().as_slice()[l_start..l_start + n_bytes]; | ||
| let rhs = &right.inner().as_slice()[r_start..r_start + n_bytes]; | ||
|
|
||
| let (lhs_words, lhs_tail) = lhs.as_chunks::<8>(); | ||
| let (rhs_words, rhs_tail) = rhs.as_chunks::<8>(); | ||
|
|
||
| let words = lhs_words | ||
| .iter() | ||
| .zip(rhs_words) | ||
| .map(|(l, r)| (u64::from_le_bytes(*l), u64::from_le_bytes(*r))) | ||
| .chain((!lhs_tail.is_empty()).then(|| (read_u64_le(lhs_tail), read_u64_le(rhs_tail)))); | ||
| return combine_words(words, len, op, comb); | ||
|
Comment on lines
259
to
+274
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you find
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for the sizes that actually hit this, yeah. the buffer kernel only runs when both sides are mixed masks, all-true and all-false short-circuit. those are chunk-sized, and as_chunks is the fastest of the three there. it also matches the other bit kernels here, count_ones and select both read via as_chunks::<8> not BitChunkIterator.
as_chunks vs bare .iter() is close to a wash and bare edges ahead once it goes memory-bound. chunk size is not capped so a mask can get large, but as_chunks is never much worse and stays consistent with count_ones and select. bench codeuse divan::Bencher;
use divan::black_box;
use vortex_buffer::BitBuffer;
fn main() {
divan::main();
}
const SIZES: &[usize] = &[1_024, 8_192, 65_536, 524_288, 4_194_304, 33_554_432];
fn inputs(len: usize) -> (BitBuffer, BitBuffer) {
let a = BitBuffer::from_iter((0..len).map(|i| (i.wrapping_mul(2_654_435_761) >> 13) & 1 == 0));
let b = BitBuffer::from_iter((0..len).map(|i| (i.wrapping_mul(40_503) >> 7) & 1 == 0));
(a, b)
}
fn read_u64_le(bytes: &[u8]) -> u64 {
let mut buf = [0u8; 8];
buf[..bytes.len()].copy_from_slice(bytes);
u64::from_le_bytes(buf)
}
fn combine_as_chunks(a: &BitBuffer, b: &BitBuffer, len: usize) -> Vec<u64> {
let nbytes = len.div_ceil(8);
let la = &a.inner().as_slice()[..nbytes];
let lb = &b.inner().as_slice()[..nbytes];
let (aw, at) = la.as_chunks::<8>();
let (bw, bt) = lb.as_chunks::<8>();
let mut out = Vec::with_capacity(len.div_ceil(64));
for (x, y) in aw.iter().zip(bw) {
out.push(u64::from_le_bytes(*x) & u64::from_le_bytes(*y));
}
if !at.is_empty() {
out.push(read_u64_le(at) & read_u64_le(bt));
}
out
}
fn combine_bare_iter(a: &BitBuffer, b: &BitBuffer, len: usize) -> Vec<u64> {
let mut out = Vec::with_capacity(len.div_ceil(64));
for (x, y) in a.chunks().iter().zip(b.chunks().iter()) {
out.push(x & y);
}
if a.chunks().remainder_len() != 0 {
out.push(a.chunks().remainder_bits() & b.chunks().remainder_bits());
}
out
}
fn combine_bitchunks(a: &BitBuffer, b: &BitBuffer, len: usize) -> Vec<u64> {
let n_words = len.div_ceil(64);
let mut out = Vec::with_capacity(n_words);
for (x, y) in a
.chunks()
.iter_padded()
.zip(b.chunks().iter_padded())
.take(n_words)
{
out.push(x & y);
}
out
}
#[divan::bench(args = SIZES)]
fn aligned_as_chunks(bencher: Bencher, len: usize) {
let (a, b) = inputs(len);
bencher.bench_local(|| combine_as_chunks(black_box(&a), black_box(&b), len));
}
#[divan::bench(args = SIZES)]
fn aligned_bare_iter(bencher: Bencher, len: usize) {
let (a, b) = inputs(len);
bencher.bench_local(|| combine_bare_iter(black_box(&a), black_box(&b), len));
}
#[divan::bench(args = SIZES)]
fn aligned_bitchunks(bencher: Bencher, len: usize) {
let (a, b) = inputs(len);
bencher.bench_local(|| combine_bitchunks(black_box(&a), black_box(&b), len));
} |
||
| } | ||
|
|
||
| // Sub-byte offset: `iter_padded` realigns the bits and zero-pads the final word. | ||
| let words = left | ||
| .chunks() | ||
| .iter_padded() | ||
| .zip(right.chunks().iter_padded()); | ||
| combine_words(words, len, op, comb) | ||
| } | ||
|
|
||
| /// Combine an iterator of `(left, right)` words via `op` into a `len`-bit [`BitBuffer`], folding | ||
| /// `comb` over each in-range result word. Consumes exactly `ceil(len / 64)` words (any extra the | ||
| /// producer yields, e.g. `iter_padded`'s trailing pad word, is dropped). Only the final word can | ||
| /// hold bits past `len`, so it alone is masked for `comb`; the buffer keeps `op`'s raw output. | ||
| #[inline] | ||
| fn combine_words<I, F, C>(words: I, len: usize, mut op: F, mut comb: C) -> BitBuffer | ||
| where | ||
| I: Iterator<Item = (u64, u64)>, | ||
| F: FnMut(u64, u64) -> u64, | ||
| C: FnMut(u64), | ||
| { | ||
| let n_words = len.div_ceil(64); | ||
| let mut out: BufferMut<u64> = BufferMut::with_capacity(n_words); | ||
| let mut words = words.take(n_words); | ||
|
|
||
| // Words fully within `len` need no masking; handling the final word separately keeps this loop | ||
| // uniform, with no per-word branch. | ||
| for (l, r) in words.by_ref().take(n_words - 1) { | ||
| let word = op(l, r); | ||
| comb(word); | ||
| out.push(word); | ||
| } | ||
| // Final word: clear bits past `len` for `comb` only. | ||
| if let Some((l, r)) = words.next() { | ||
| let word = op(l, r); | ||
| comb(word & last_word_mask(len)); | ||
| out.push(word); | ||
| } | ||
|
|
||
| let mut bytes = out.into_byte_buffer(); | ||
| bytes.truncate(len.div_ceil(8)); | ||
| BitBuffer::new(bytes.freeze(), len) | ||
| } | ||
|
|
||
| /// Mask of the in-range bits in the final word: the low `len % 64` bits, or all 64 when `len` is | ||
| /// a multiple of 64. | ||
| #[inline] | ||
| fn last_word_mask(len: usize) -> u64 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like it should only be used once for the tail value?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's the final word not just the tail, same as the other thread. it ends up a single call once i merge the last word and tail into one step. |
||
| let rem = len % 64; | ||
| if rem == 0 { | ||
| u64::MAX | ||
| } else { | ||
| (1u64 << rem) - 1 | ||
| } | ||
| } | ||
|
|
||
| /// Like [`bitwise_binary_op`], but also returns the result's set-bit count, folded into the same | ||
| /// pass instead of re-scanning the buffer. | ||
| pub(super) fn bitwise_binary_op_counted<F: FnMut(u64, u64) -> u64>( | ||
| left: &BitBuffer, | ||
| right: &BitBuffer, | ||
| op: F, | ||
| ) -> (BitBuffer, usize) { | ||
| let mut count = 0usize; | ||
| let buffer = bitwise_binary_op_with(left, right, op, |word| { | ||
| count += word.count_ones() as usize; | ||
| }); | ||
| (buffer, count) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use std::ops::Not; | ||
|
|
@@ -367,6 +474,44 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| /// The fused path must yield the same buffer and true count as the reference | ||
| /// (`bitwise_binary_op` + a separate `true_count`), across all ops, offsets, and lengths, | ||
| /// including lengths like 127 whose final word holds out-of-range bits. | ||
| #[rstest] | ||
| #[case::aligned(0, 0)] | ||
| #[case::equal_nonzero(3, 3)] | ||
| #[case::byte_aligned(8, 16)] | ||
| #[case::mismatch(0, 5)] | ||
| fn counted_matches_reference(#[case] left_offset: usize, #[case] right_offset: usize) { | ||
| #[allow(clippy::cast_possible_truncation)] | ||
| let make = |offset: usize, len: usize, salt: u8| -> BitBuffer { | ||
| let bytes: ByteBufferMut = (0..(offset + len).div_ceil(8).max(1)) | ||
| .map(|i| (i as u8).wrapping_mul(31).wrapping_add(salt)) | ||
| .collect(); | ||
| BitBufferMut::from_buffer(bytes, offset, len).freeze() | ||
| }; | ||
| let ops: [fn(u64, u64) -> u64; 4] = | ||
| [|a, b| a & b, |a, b| a | b, |a, b| a ^ b, |a, b| a & !b]; | ||
|
|
||
| for len in [1usize, 5, 8, 63, 64, 65, 127, 128, 129, 200, 256] { | ||
| let left = make(left_offset, len, 0xC3); | ||
| let right = make(right_offset, len, 0x5A); | ||
| for op in ops { | ||
| let (got_buf, got_count) = bitwise_binary_op_counted(&left, &right, op); | ||
| let expected_buf = bitwise_binary_op(&left, &right, op); | ||
| assert_eq!( | ||
| got_buf, expected_buf, | ||
| "buffer mismatch loff={left_offset} roff={right_offset} len={len}" | ||
| ); | ||
| assert_eq!( | ||
| got_count, | ||
| expected_buf.true_count(), | ||
| "count mismatch loff={left_offset} roff={right_offset} len={len}" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Regression test for a bug where [`bitwise_unary_op`] produced corrupt results when | ||
| /// the [`BitBuffer`]'s underlying byte pointer was not u64-aligned. Slicing a buffer by | ||
| /// a non-multiple-of-8 number of bytes can cause this misalignment. The bug only | ||
|
|
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you reduce the number of benchmarks we add, also all must be under 1ms
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep will do, trim it under 1ms.