Updating how we create BloomFilter from rdb loads and upgrading bloomfilter dependency to version 3.0 #23

zackcam · 2024-11-19T18:45:59Z

Before we were initializing a vector of bloomfilters with Vec::New() which could cause the size to be higher when we pushed an item into the vector during RDB Load and Copy. This is due to the raw alloc vector growth. From testing we saw that it could be four times as large. Which makes sense from this. In order to make this size what we expect we can just set the capacity to the number of filters we know we will be adding

This PR also upgrades the dependency of bloomfilter to 3.0.
Breaking changes included:

from_existing no longer exists, it is now from_slice which has different arguments
We no longer have an exposed sip_keys but have the seed. Updated tests to check for this.
Bitmap is no longer exposed we now have to use the as_slice method to get it
len() is now being used instead of number_of_bits()
Updated serialize and deserialize to now use the module from bloom

Other changes with this upgrade include removing unneeded fields from rdb load and save as they are now included in bitmap. Due to this we also changed the size validation in rdb load to now calcualte this off of fprate and capacity. This included creating a new helper method in BloomFilterType to claculate fp_rate based on number of filters

…w has capacity of filter we are loading from Signed-off-by: zackcam <[email protected]>

… as well Signed-off-by: zackcam <[email protected]>

zackcam · 2024-12-02T22:42:49Z

Breaking changes included:

from_existing no longer exists, it is now from_slice which has different arguments
We no longer have an exposed sip_keys but have the seed. Updated tests to check for this.
Bitmap is no longer exposed we now have to use the as_slice method to get it
len() is now being used instead of number_of_bits()
Updated serialize and deserialize to now use the module from bloom

YueTang-Vanessa · 2024-12-02T23:01:30Z

Cargo.toml

@@ -15,7 +15,7 @@ homepage = "https://github.com/valkey-io/valkey-bloom"
 valkey-module = "0.1.2"
 valkey-module-macros = "0"
 linkme = "0"
-bloomfilter = { version = "1.0.13", features = ["serde"] }
+bloomfilter = { version = "3", features = ["serde"] }


Please change the version format 3 -> 3.0.0

YueTang-Vanessa · 2024-12-03T03:21:49Z

src/bloom/utils.rs

@@ -104,7 +105,7 @@ impl BloomFilterType {

    /// Create a new BloomFilterType object from an existing one.
    pub fn create_copy_from(from_bf: &BloomFilterType) -> BloomFilterType {
-        let mut filters = Vec::new();
+        let mut filters: Vec<BloomFilter> = Vec::with_capacity(from_bf.filters.len());


Should we worry about over-allocate? ( when filters.len() = 0)

filters.len() will never / should never be 0, so this seems OK

KarthikSubbarao · 2024-12-03T20:10:22Z

src/bloom/data_type.rs

@@ -75,6 +74,8 @@ impl ValkeyDataType for BloomFilterType {
        let Ok(fp_rate) = raw::load_double(rdb) else {
            return None;
        };
+        let mut filters: Vec<BloomFilter> = Vec::with_capacity(num_filters as usize);
+


Few comments on RDB Save and Load.

Number of hash functions are included in the bitmap dump as a result of to_bytes. So we can remove this from the save/load logic. Right?

Do we need number of bits for any purpose? Is it needed for bloom filter restoration (external bloom)? If not, can we remove it? If it helps with any purpose (or if it makes size validation impossible), feel free to retain it

We don't need both fields. I removed them both and instead moved the logic of checking the size until we have converted the bloom object into a type where we can get the number of bits

KarthikSubbarao · 2024-12-03T23:35:20Z

src/bloom/data_type.rs

-                dig.add_long_long(key1 as i64);
-                dig.add_long_long(key2 as i64);
-            }
+            dig.add_string_buffer(filter.bloom.as_slice());


Just Confirming. Does the slice contain the seed as well?

Yes it is contained in the header portion of the bitmap. BitMap::set_seed(header, &seed); it is set like this in sync

KarthikSubbarao · 2024-12-03T23:50:53Z

src/bloom/data_type.rs

-            // Reject RDB Load if any bloom filter within a bloom object of a size greater than what is allowed.
-            if !BloomFilter::validate_size_with_bits(number_of_bits) {
+            let new_fp_rate =
+                match Self::calculate_fp_rate(fp_rate, num_filters.try_into().unwrap()) {


Let's not unwrap

KarthikSubbarao · 2024-12-03T23:50:59Z

src/bloom/data_type.rs

+                        return None;
+                    }
+                };
+            if !BloomFilter::validate_size(capacity.try_into().unwrap(), new_fp_rate) {


Let's not unwrap

You can cast the u64 to a u32 using as

KarthikSubbarao · 2024-12-03T23:51:07Z

src/bloom/utils.rs

@@ -330,7 +337,8 @@ impl BloomFilter {
            capacity as usize,
            fp_rate,
            &configs::FIXED_SEED,
-        );
+        )
+        .unwrap();


Let's not unwrap

KarthikSubbarao · 2024-12-03T23:51:14Z

src/bloom/utils.rs

-            sip_keys,
-        );
+    pub fn from_existing(bitmap: &[u8], num_items: u32, capacity: u32) -> BloomFilter {
+        let bloom = bloomfilter::Bloom::from_slice(bitmap).unwrap();


Let's not unwrap

…ll as removing unnecesary fields saved in rdb Signed-off-by: zackcam <[email protected]>

src/bloom/data_type.rs

Signed-off-by: KarthikSubbarao <[email protected]>

src/bloom/utils.rs

Signed-off-by: KarthikSubbarao <[email protected]>

src/bloom/data_type.rs

Signed-off-by: KarthikSubbarao <[email protected]>

KarthikSubbarao · 2024-12-04T05:50:14Z

src/bloom/data_type.rs

@@ -75,24 +71,28 @@ impl ValkeyDataType for BloomFilterType {
        let Ok(fp_rate) = raw::load_double(rdb) else {
            return None;
        };
+        let mut filters: Vec<BloomFilter> = Vec::with_capacity(num_filters as usize);


Note: This will be fixed in a follow up PR. We want to make the vector expansion consistent for (1) bloom object creation and scaling (2) COPY (3) RDB Load (4) BF.LOAD.

We can also add the vector capacity into the DEBUG DIGEST. This will be very useful for correctness testing

You can keep this in mind when working on the fix @zackcam

…filter dependency to version 3.0 (valkey-io#23) * Updating how we create BloomFilter from rdb loads. BloomFilter vec now has capacity of filter we are loading from Signed-off-by: zackcam <[email protected]> * Updating bloomfilter dependency to version 3, fixing breaking changes as well Signed-off-by: zackcam <[email protected]> * Updating the digest changes to follow updated version of bloom. As well as removing unnecesary fields saved in rdb Signed-off-by: zackcam <[email protected]> * Update log in src/bloom/data_type.rs Signed-off-by: KarthikSubbarao <[email protected]> * Update comment in src/bloom/utils.rs Signed-off-by: KarthikSubbarao <[email protected]> * Clippy error in src/bloom/data_type.rs Signed-off-by: KarthikSubbarao <[email protected]> --------- Signed-off-by: zackcam <[email protected]> Signed-off-by: KarthikSubbarao <[email protected]> Co-authored-by: KarthikSubbarao <[email protected]>

KarthikSubbarao approved these changes Nov 19, 2024

View reviewed changes

KarthikSubbarao force-pushed the unstable branch 3 times, most recently from 188835b to a33e0e3 Compare November 21, 2024 21:52

zackcam force-pushed the unstable branch from 9182dec to cbbefa2 Compare November 22, 2024 06:08

zackcam closed this Nov 22, 2024

zackcam force-pushed the unstable branch from cbbefa2 to a33e0e3 Compare November 22, 2024 06:09

Updating how we create BloomFilter from rdb loads. BloomFilter vec no…

8628648

…w has capacity of filter we are loading from Signed-off-by: zackcam <[email protected]>

zackcam reopened this Nov 22, 2024

zackcam force-pushed the unstable branch from 12a76fb to 8628648 Compare November 22, 2024 06:18

Updating bloomfilter dependency to version 3, fixing breaking changes…

8cbec6d

… as well Signed-off-by: zackcam <[email protected]>

zackcam force-pushed the unstable branch from aa6136f to 8cbec6d Compare December 2, 2024 22:32

Merge branch 'valkey-io:unstable' into unstable

669fd9f

YueTang-Vanessa reviewed Dec 2, 2024

View reviewed changes

zackcam force-pushed the unstable branch from 52f1095 to 10f318f Compare December 2, 2024 23:04

YueTang-Vanessa reviewed Dec 3, 2024

View reviewed changes

KarthikSubbarao reviewed Dec 3, 2024

View reviewed changes

zackcam force-pushed the unstable branch 3 times, most recently from 3da4c19 to 0484bbb Compare December 3, 2024 23:18

zackcam changed the title ~~Updating how we create BloomFilter from rdb loads. BloomFilter vec now has capacity of filter we are loading from~~ Updating how we create BloomFilter from rdb loads and upgrading bloomfilter dependency to version 3.0 Dec 3, 2024

KarthikSubbarao reviewed Dec 3, 2024

View reviewed changes

Updating the digest changes to follow updated version of bloom. As we…

7d09c3d

…ll as removing unnecesary fields saved in rdb Signed-off-by: zackcam <[email protected]>

zackcam force-pushed the unstable branch from 0484bbb to 7d09c3d Compare December 4, 2024 00:03

YueTang-Vanessa approved these changes Dec 4, 2024

View reviewed changes

KarthikSubbarao reviewed Dec 4, 2024

View reviewed changes

src/bloom/data_type.rs Outdated Show resolved Hide resolved

Update log in src/bloom/data_type.rs

5f364cf

Signed-off-by: KarthikSubbarao <[email protected]>

KarthikSubbarao reviewed Dec 4, 2024

View reviewed changes

src/bloom/utils.rs Show resolved Hide resolved

Update comment in src/bloom/utils.rs

7cb7098

Signed-off-by: KarthikSubbarao <[email protected]>

KarthikSubbarao reviewed Dec 4, 2024

View reviewed changes

src/bloom/data_type.rs Outdated Show resolved Hide resolved

Clippy error in src/bloom/data_type.rs

976b082

Signed-off-by: KarthikSubbarao <[email protected]>

KarthikSubbarao reviewed Dec 4, 2024

View reviewed changes

KarthikSubbarao approved these changes Dec 4, 2024

View reviewed changes

KarthikSubbarao merged commit 34d6fb0 into valkey-io:unstable Dec 4, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating how we create BloomFilter from rdb loads and upgrading bloomfilter dependency to version 3.0 #23

Updating how we create BloomFilter from rdb loads and upgrading bloomfilter dependency to version 3.0 #23

zackcam commented Nov 19, 2024 •

edited

Loading

zackcam commented Dec 2, 2024

YueTang-Vanessa Dec 2, 2024

YueTang-Vanessa Dec 3, 2024

KarthikSubbarao Dec 3, 2024

KarthikSubbarao Dec 3, 2024

zackcam Dec 3, 2024

KarthikSubbarao Dec 3, 2024

zackcam Dec 3, 2024

KarthikSubbarao Dec 3, 2024

KarthikSubbarao Dec 3, 2024

KarthikSubbarao Dec 3, 2024

KarthikSubbarao Dec 3, 2024

KarthikSubbarao Dec 3, 2024

KarthikSubbarao Dec 4, 2024 •

edited

Loading

Updating how we create BloomFilter from rdb loads and upgrading bloomfilter dependency to version 3.0 #23

Updating how we create BloomFilter from rdb loads and upgrading bloomfilter dependency to version 3.0 #23

Conversation

zackcam commented Nov 19, 2024 • edited Loading

zackcam commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarthikSubbarao Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

zackcam commented Nov 19, 2024 •

edited

Loading

KarthikSubbarao Dec 4, 2024 •

edited

Loading