perf(mask): fuse true_count into boolean operators#8425
Conversation
There was a problem hiding this comment.
can you reduce the number of benchmarks we add, also all must be under 1ms
There was a problem hiding this comment.
yep will do, trim it under 1ms.
| if left.offset() != 0 || right.offset() != 0 { | ||
| let buffer = bitwise_binary_op(left, right, op); | ||
| let true_count = buffer.true_count(); | ||
| return (buffer, true_count); | ||
| } |
There was a problem hiding this comment.
why do you fail here?
There was a problem hiding this comment.
yeah you are right, fallback is still two pass. will fold into the combiner loop and drop it.
There was a problem hiding this comment.
can you also read the benchmarking guidelines
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decompress_rd[f64, (100000, 0.0)] |
845.7 µs | 1,023.8 µs | -17.4% |
| ❌ | Simulation | decompress_rd[f32, (100000, 0.0)] |
499.2 µs | 586.2 µs | -14.85% |
| ❌ | Simulation | chunked_varbinview_canonical_into[(100, 100)] |
273.4 µs | 308.4 µs | -11.34% |
| ❌ | Simulation | encode_varbin[(1000, 2)] |
157.9 µs | 177.4 µs | -10.99% |
| ⚡ | Simulation | chunked_varbinview_canonical_into[(1000, 10)] |
198 µs | 161.7 µs | +22.48% |
| ⚡ | Simulation | decompress_rd[f64, (100000, 0.01)] |
981.5 µs | 845.2 µs | +16.12% |
| ⚡ | Simulation | decompress_rd[f64, (100000, 0.1)] |
981.4 µs | 845.2 µs | +16.11% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[128] |
215.3 ns | 186.1 ns | +15.67% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[1024] |
275.6 ns | 246.4 ns | +11.84% |
| 🆕 | Simulation | and_fused[1024] |
N/A | 3.7 µs | N/A |
| 🆕 | Simulation | and_fused[524288] |
N/A | 237.6 µs | N/A |
| 🆕 | Simulation | and_fused[65536] |
N/A | 33 µs | N/A |
| 🆕 | Simulation | and_fused[8192] |
N/A | 6.3 µs | N/A |
| 🆕 | Simulation | and_owned_fused[1024] |
N/A | 4.2 µs | N/A |
| 🆕 | Simulation | and_owned_fused[524288] |
N/A | 237.9 µs | N/A |
| 🆕 | Simulation | and_owned_fused[65536] |
N/A | 33.3 µs | N/A |
| 🆕 | Simulation | and_owned_fused[8192] |
N/A | 6.6 µs | N/A |
| 🆕 | Simulation | and_owned_two_pass[1024] |
N/A | 5.2 µs | N/A |
| 🆕 | Simulation | and_owned_two_pass[524288] |
N/A | 107.1 µs | N/A |
| 🆕 | Simulation | and_owned_two_pass[65536] |
N/A | 17.5 µs | N/A |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing miniex:perf/mask-fused-true-count (e15657e) with develop (9b5447a)
Footnotes
-
10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
There was a problem hiding this comment.
this is a great, insight.
pub(super) fn bitwise_binary_op_counted<F: FnMut(u64, u64) -> u64, C: u64 -> ()>(
left: &BitBuffer,
right: &BitBuffer,
mut op: F,
mut comb: C
) -> (BitBuffer) {
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.
There was a problem hiding this comment.
Ah, I learned a lot here, thank you. I will merge to one binary_op with a comb: FnMut(u64) - |_| {} for plain op (compiler drop it) and |w| count += w.count_ones() for counted, so the offset case is also gone. And you are right, some of the speedup is just a better kernel not the fused count, so I move it into the shared binary_op and bench again.
There was a problem hiding this comment.
I have a feeling part of your speed up is a better binary_op impl for BitBuffer would be nice to see that.
|
pushed the combiner version and dropped the fallback. can you check if i got your point right? also i ran the bench on a different machine than before, so the numbers are not 1:1 with the old table. |
| let observed = if lhs_tail.is_empty() && i + 1 == full { | ||
| word & last_word_mask(len) | ||
| } else { | ||
| word | ||
| }; |
There was a problem hiding this comment.
move this out of the hot loop
| }; | ||
| comb(observed); | ||
| // SAFETY: capacity reserved for `n_words` words, each written exactly once. | ||
| unsafe { out.push_unchecked(word) }; |
There was a problem hiding this comment.
use either a mut iterator
| let word = op(u64::from_le_bytes(*l), u64::from_le_bytes(*r)); | ||
| // Only the last word can hold bits past `len`; clear them for `comb`. | ||
| let observed = if lhs_tail.is_empty() && i + 1 == full { | ||
| word & last_word_mask(len) |
There was a problem hiding this comment.
does this actually get hit, seeing as you have a tail below to handle this too?
There was a problem hiding this comment.
yeah it's hit when n_bytes is a multiple of 8 but len isn't a multiple of 64, so no tail runs but the last word still has bits past len
joseph-isaacs
left a comment
There was a problem hiding this comment.
Why are there two loops?
| /// 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 { |
There was a problem hiding this comment.
This seems like it should only be used once for the tail value?
There was a problem hiding this comment.
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.
byte-aligned reads the backing bytes directly as u64 for the fast kernel, sub-byte offsets straddle byte boundaries so they need iter_padded to realign. |
|
pushed the updates for your comments. mask moved out of the hot loop, only the final word is masked now, and the unsafe push is gone. can you check if i got it right? also miri caught an out-of-bounds push in the sub-byte path where iter_padded appends one extra word, so i capped it to ceil(len/64). |
| if left.offset().is_multiple_of(8) && right.offset().is_multiple_of(8) { | ||
| let left_chunks = left.unaligned_chunks(); | ||
| let right_chunks = right.unaligned_chunks(); | ||
| if left_chunks.lead_padding() == 0 | ||
| && left_chunks.trailing_padding() == 0 | ||
| && right_chunks.lead_padding() == 0 | ||
| && right_chunks.trailing_padding() == 0 | ||
| { | ||
| let iter = left_chunks | ||
| .iter() | ||
| .zip(right_chunks.iter()) | ||
| .map(|(l, r)| op(l, r)); | ||
| let iter = unsafe { iter.trusted_len() }; | ||
| let result = Buffer::<u64>::from_trusted_len_iter(iter).into_byte_buffer(); | ||
| return BitBuffer::new(result, left.len()); | ||
| } | ||
| 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); |
There was a problem hiding this comment.
Do you find lhs.as_chunks::<8>() is more performant to BitChunkIterator
There was a problem hiding this comment.
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.
& at offset 0, as_chunks vs bare BitChunkIterator vs iter_padded:
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 code
use 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));
}
joseph-isaacs
left a comment
There was a problem hiding this comment.
This is a really great result.
Do you think we could break out the changes to the bit collect into its own pr and keep the collect+count in this pr.
then we can examine each perf change in isolation
thanks! makes sense. will split the bit collect into its own pr and keep collect+count here |
|
done, split into #8436 |
744ae4e to
e9c59e0
Compare
|
done, rebased onto #8436. |
e9c59e0 to
ca220b8
Compare
## Summary Split out of #8425 so each perf change can be looked at on its own, as suggested by @joseph-isaacs. This is the "bit collect" half, no count fusion. `bitwise_binary_op` only took the direct word path when both operands were 64-bit aligned, otherwise falling back to `iter_padded`. It now reads the backing bytes as `u64` via `as_chunks::<8>` for any byte-aligned offset, leaving `iter_padded` for sub-byte offsets only. This also drops the `from_trusted_len_iter` unsafe for a safe `BufferMut` push. --------- Signed-off-by: Han Damin <miniex@daminstudio.net> Co-authored-by: Joe Isaacs <joe.isaacs@live.co.uk>
`&`, `|`, `bitand_not` and `!` re-scanned the result with `true_count()` after building it. they now combine and count in a single pass; `!` reuses the cached count. Closes vortex-data#1508 Signed-off-by: Han Damin <miniex@daminstudio.net>
ca220b8 to
e15657e
Compare
|
Do we see a perf with when fusing count and pack? |
no, right now the fused seems slower than two-pass.. after making the kernel fast with the iterator, the vectorized two-pass beats the fused loop.. so fusion doesn't really seem to give a win.. i think it'd be better to close this PR and keep it as the two-pass approach and flesh it out more..! my head was all over the place so i didn't think of it.. thanks! |
|
I would expect a small win for large bit buffers. I am happy with the other commit |
Summary
Closes: #1508
Stacked on #8436, the bit collect half. the kernel change is reviewed there, so the diff here includes it until #8436 merges.
Mask's boolean operators (&,|,bitand_not,!) built the result bufferthen re-scanned it with
true_count(), walking the bits twice. They now foldcount_onesinto the combine pass through a sharedbitwise_binary_op_with, soit is single-pass for every offset;
!maskreuses the cached count, no popcount.API Changes
Adds three non-breaking
BitBuffermethods:bitand_with_true_count,bitor_with_true_count,bitand_not_with_true_count.Benchmark
vortex-buffer/benches/bitbuffer_count.rscompares two-pass vs fused. The gainis ~neutral for small borrowed inputs and grows with size, largest on owned-LHS.
Testing
cargo test -p vortex-buffer -p vortex-maskpasses, plus a per-bit referencetest across offsets/lengths and a
Mask-level regression test for #1508. Italso passes under
miri(strict provenance).fmt+clippy --all-targets --all-featuresclean.I'm Korean, so sorry if any wording reads a little awkward.