From 595c373521d1d0c4ceaee28fe4d1f145dfe65b58 Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 29 Mar 2023 11:38:36 +0800 Subject: [PATCH 1/8] make bitmap bench more accurate --- src/common/benches/bitmap.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/common/benches/bitmap.rs b/src/common/benches/bitmap.rs index 943c394f93d48..9edf4dc3f284f 100644 --- a/src/common/benches/bitmap.rs +++ b/src/common/benches/bitmap.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use criterion::{criterion_group, criterion_main, BatchSize, Criterion}; +use criterion::{criterion_group, criterion_main, BatchSize, Criterion, black_box}; use itertools::Itertools; use risingwave_common::buffer::{Bitmap, BitmapIter}; @@ -39,14 +39,13 @@ fn bench_bitmap_iter(c: &mut Criterion) { fn bench_bitmap_iter(bench_id: &str, bitmap: Bitmap, c: &mut Criterion) { let bitmaps = vec![bitmap; N_CHUNKS]; let make_iters = || make_iterators(&bitmaps); - let mut result = vec![true; CHUNK_SIZE]; c.bench_function(bench_id, |b| { b.iter_batched( make_iters, |iters| { for iter in iters { - for (i, bit_flag) in iter.enumerate() { - result[i] = bit_flag; + for bit_flag in iter { + black_box(bit_flag); } } }, From 1c0047bb28418c802036bb977e48bfba2809baa1 Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 29 Mar 2023 11:42:45 +0800 Subject: [PATCH 2/8] avoid name shadowing --- src/common/benches/bitmap.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/common/benches/bitmap.rs b/src/common/benches/bitmap.rs index 9edf4dc3f284f..d4ba50df9e017 100644 --- a/src/common/benches/bitmap.rs +++ b/src/common/benches/bitmap.rs @@ -36,7 +36,7 @@ fn bench_bitmap_iter(c: &mut Criterion) { fn make_iterators(bitmaps: &[Bitmap]) -> Vec> { bitmaps.iter().map(|bitmap| bitmap.iter()).collect_vec() } - fn bench_bitmap_iter(bench_id: &str, bitmap: Bitmap, c: &mut Criterion) { + fn bench_bitmap_iter_inner(bench_id: &str, bitmap: Bitmap, c: &mut Criterion) { let bitmaps = vec![bitmap; N_CHUNKS]; let make_iters = || make_iterators(&bitmaps); c.bench_function(bench_id, |b| { @@ -54,9 +54,9 @@ fn bench_bitmap_iter(c: &mut Criterion) { }); } let zeros = Bitmap::zeros(CHUNK_SIZE); - bench_bitmap_iter("zeros_iter", zeros, c); + bench_bitmap_iter_inner("zeros_iter", zeros, c); let ones = Bitmap::ones(CHUNK_SIZE); - bench_bitmap_iter("ones_iter", ones, c); + bench_bitmap_iter_inner("ones_iter", ones, c); } criterion_group!(benches, bench_bitmap, bench_bitmap_iter); From 9c13c1f9466ef051427e7d7647da72d657bcf762 Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 29 Mar 2023 11:55:48 +0800 Subject: [PATCH 3/8] cleanup bitmap iter representation, no performance regression --- src/common/src/buffer/bitmap.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/common/src/buffer/bitmap.rs b/src/common/src/buffer/bitmap.rs index d75a150e2c31a..e8340208e3df4 100644 --- a/src/common/src/buffer/bitmap.rs +++ b/src/common/src/buffer/bitmap.rs @@ -180,6 +180,7 @@ pub struct Bitmap { // The number of high bits in the bitmap. count_ones: usize, + /// Bits are stored in a compact form via usize. bits: Box<[usize]>, } @@ -565,9 +566,15 @@ impl<'a> iter::Iterator for BitmapIter<'a> { if self.idx >= self.num_bits { return None; } - let b = unsafe { self.bits.get_unchecked(self.idx / BITS) } & (1 << (self.idx % BITS)) != 0; + // Get the index of usize which the bit is located in + let usize_index = self.idx / BITS; + // Offset of the bit within the usize. + let usize_offset = self.idx % BITS; + let bit_mask = 1 << usize_offset; + let usize_containing_bit = unsafe { self.bits.get_unchecked(usize_index) }; + let bit_flag = usize_containing_bit & bit_mask != 0; self.idx += 1; - Some(b) + Some(bit_flag) } fn size_hint(&self) -> (usize, Option) { From 9c7f1570142be4a91dfde394bcf2b09b683ed6c6 Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 29 Mar 2023 12:07:04 +0800 Subject: [PATCH 4/8] improve docs --- src/common/src/buffer/bitmap.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/common/src/buffer/bitmap.rs b/src/common/src/buffer/bitmap.rs index e8340208e3df4..05285989f7d77 100644 --- a/src/common/src/buffer/bitmap.rs +++ b/src/common/src/buffer/bitmap.rs @@ -173,14 +173,15 @@ impl BitmapBuilder { /// An immutable bitmap. Use [`BitmapBuilder`] to build it. #[derive(Clone, PartialEq, Eq)] pub struct Bitmap { - // The useful bits in the bitmap. The total number of bits will usually - // be larger than the useful bits due to byte-padding. + /// The useful bits in the bitmap. The total number of bits will usually + /// be larger than the useful bits due to byte-padding. num_bits: usize, - // The number of high bits in the bitmap. + /// The number of high bits in the bitmap. count_ones: usize, - /// Bits are stored in a compact form via usize. + /// Bits are stored in a compact form. + /// They are packed into `usize`s. bits: Box<[usize]>, } From a468aa7b7981619746e6fe92b16ec93fde1c12a6 Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 29 Mar 2023 12:16:15 +0800 Subject: [PATCH 5/8] reduce calls to get_unchecked by factor of usize --- src/common/src/buffer/bitmap.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/common/src/buffer/bitmap.rs b/src/common/src/buffer/bitmap.rs index 05285989f7d77..90102619ebe96 100644 --- a/src/common/src/buffer/bitmap.rs +++ b/src/common/src/buffer/bitmap.rs @@ -312,6 +312,7 @@ impl Bitmap { bits: &self.bits, idx: 0, num_bits: self.num_bits, + current_usize: 0, } } @@ -558,6 +559,7 @@ pub struct BitmapIter<'a> { bits: &'a [usize], idx: usize, num_bits: usize, + current_usize: usize, } impl<'a> iter::Iterator for BitmapIter<'a> { @@ -567,13 +569,19 @@ impl<'a> iter::Iterator for BitmapIter<'a> { if self.idx >= self.num_bits { return None; } - // Get the index of usize which the bit is located in - let usize_index = self.idx / BITS; + // Offset of the bit within the usize. let usize_offset = self.idx % BITS; + + if usize_offset == 0 { + // Get the index of usize which the bit is located in + let usize_index = self.idx / BITS; + self.current_usize = unsafe { *self.bits.get_unchecked(usize_index) }; + } + let bit_mask = 1 << usize_offset; - let usize_containing_bit = unsafe { self.bits.get_unchecked(usize_index) }; - let bit_flag = usize_containing_bit & bit_mask != 0; + + let bit_flag = self.current_usize & bit_mask != 0; self.idx += 1; Some(bit_flag) } From ddab3ad68344295034da122f8a71d3fd80ce126a Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 29 Mar 2023 12:45:50 +0800 Subject: [PATCH 6/8] add more bench for bitmap::get * Add various granularity: 1, 1000, 1000_000 --- src/common/benches/bitmap.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/common/benches/bitmap.rs b/src/common/benches/bitmap.rs index d4ba50df9e017..c3f84e2053842 100644 --- a/src/common/benches/bitmap.rs +++ b/src/common/benches/bitmap.rs @@ -23,7 +23,9 @@ fn bench_bitmap(c: &mut Criterion) { let i = 0x123; c.bench_function("zeros", |b| b.iter(|| Bitmap::zeros(CHUNK_SIZE))); c.bench_function("ones", |b| b.iter(|| Bitmap::ones(CHUNK_SIZE))); - c.bench_function("get", |b| b.iter(|| x.is_set(i))); + c.bench_function("get", |b| b.iter(|| for _ in 0..1000 { black_box(x.is_set(i)); })); + c.bench_function("get_1000", |b| b.iter(|| for _ in 0..1000 { black_box(x.is_set(i)); })); + c.bench_function("get_1000_000", |b| b.iter(|| for _ in 0..1000_000 { black_box(x.is_set(i)); })); c.bench_function("and", |b| b.iter(|| &x & &y)); c.bench_function("or", |b| b.iter(|| &x | &y)); c.bench_function("not", |b| b.iter(|| !&x)); From c40d6fc02658165cc67b0493a694f64fc0d32cdd Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 29 Mar 2023 13:11:35 +0800 Subject: [PATCH 7/8] fmt --- src/common/benches/bitmap.rs | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/common/benches/bitmap.rs b/src/common/benches/bitmap.rs index c3f84e2053842..64796d42944d4 100644 --- a/src/common/benches/bitmap.rs +++ b/src/common/benches/bitmap.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use criterion::{criterion_group, criterion_main, BatchSize, Criterion, black_box}; +use criterion::{black_box, criterion_group, criterion_main, BatchSize, Criterion}; use itertools::Itertools; use risingwave_common::buffer::{Bitmap, BitmapIter}; @@ -23,9 +23,27 @@ fn bench_bitmap(c: &mut Criterion) { let i = 0x123; c.bench_function("zeros", |b| b.iter(|| Bitmap::zeros(CHUNK_SIZE))); c.bench_function("ones", |b| b.iter(|| Bitmap::ones(CHUNK_SIZE))); - c.bench_function("get", |b| b.iter(|| for _ in 0..1000 { black_box(x.is_set(i)); })); - c.bench_function("get_1000", |b| b.iter(|| for _ in 0..1000 { black_box(x.is_set(i)); })); - c.bench_function("get_1000_000", |b| b.iter(|| for _ in 0..1000_000 { black_box(x.is_set(i)); })); + c.bench_function("get", |b| { + b.iter(|| { + for _ in 0..1000 { + black_box(x.is_set(i)); + } + }) + }); + c.bench_function("get_1000", |b| { + b.iter(|| { + for _ in 0..1000 { + black_box(x.is_set(i)); + } + }) + }); + c.bench_function("get_1000_000", |b| { + b.iter(|| { + for _ in 0..1_000_000 { + black_box(x.is_set(i)); + } + }) + }); c.bench_function("and", |b| b.iter(|| &x & &y)); c.bench_function("or", |b| b.iter(|| &x | &y)); c.bench_function("not", |b| b.iter(|| !&x)); From c1bf0a1a7542becd6281d0ac3c84cc8aa2634c1c Mon Sep 17 00:00:00 2001 From: Noel Kwan Date: Wed, 29 Mar 2023 13:54:35 +0800 Subject: [PATCH 8/8] fix nth --- src/common/src/buffer/bitmap.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/common/src/buffer/bitmap.rs b/src/common/src/buffer/bitmap.rs index 90102619ebe96..630baafef1729 100644 --- a/src/common/src/buffer/bitmap.rs +++ b/src/common/src/buffer/bitmap.rs @@ -562,6 +562,26 @@ pub struct BitmapIter<'a> { current_usize: usize, } +impl<'a> BitmapIter<'a> { + fn next_always_load_usize(&mut self) -> Option { + if self.idx >= self.num_bits { + return None; + } + + // Offset of the bit within the usize. + let usize_offset = self.idx % BITS; + + // Get the index of usize which the bit is located in + let usize_index = self.idx / BITS; + self.current_usize = unsafe { *self.bits.get_unchecked(usize_index) }; + + let bit_mask = 1 << usize_offset; + let bit_flag = self.current_usize & bit_mask != 0; + self.idx += 1; + Some(bit_flag) + } +} + impl<'a> iter::Iterator for BitmapIter<'a> { type Item = bool; @@ -593,7 +613,7 @@ impl<'a> iter::Iterator for BitmapIter<'a> { fn nth(&mut self, n: usize) -> Option { self.idx += n; - self.next() + self.next_always_load_usize() } }