Skip to content

[bitvec] change from bit-vec to bitvec #34

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

Closed
wants to merge 3 commits into from
Closed

Conversation

ufoot
Copy link

@ufoot ufoot commented Jan 12, 2023

This is basically a dependency change, going from https://crates.io/crates/bit-vec to https://crates.io/crates/bitvec

The change was quite straightforward, a few things still:

  • since some of your funcs export as Vec<u8> I had to use BitVec<u8> instead of the default BitVec which is a BitVec<usize>. I did not investigate to see if that would speed up things, but maybe it does.
  • I did some basic perf testing, introducing benches. There are 2 branches, one with my patch here and one without the patch there (the current implementation). Both yield exactly the same results. The difference is unnoticeable.
  • the reason I would appreciate this change is that this bitvec library has count_ones which is interesting to get a feeling of whether the Bloom filter is full, or not.
  • there is one change that I found a bit strange, the clear() function seems to have different semantics in both libraries, looks like the one in the new library clears the data but keeps the capacity. I fail to see how that is practically possible on bits but it looks like it works anyway.

Happy to have your feedback on this, I was about to re-implement some Bloom filter with this bitvec, and found adapting your concise (and great!) library was really good enough.

ufoot added 3 commits January 12, 2023 22:57
Got the following warning:
warning: manifest at [...] contains `[project]` instead of `[package]`, this could become a hard error in the future
This fixes it.
@@ -1,4 +1,4 @@
[project]
[package]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cargo complains about this...

}

/// Bloom filter structure
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[cfg_attr(feature = "serde", serde(crate = "serde"))]
#[derive(Clone, Debug)]
pub struct Bloom<T: ?Sized> {
bit_vec: BitVec,
bit_vec: BitVec<u8>,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default it's usize but to keep compatibility with your api, had to use u8

siphasher = "0.3.7"
bitvec = "1.0.1"
getrandom = { version = "0.2.8", optional = true }
siphasher = "0.3.10"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the last version, thought it was OK to bump it. Works well.

@@ -234,7 +235,7 @@ impl<T: ?Sized> Bloom<T> {

/// Clear all of the bits in the filter, removing all keys from the set
pub fn clear(&mut self) {
self.bit_vec.clear()
self.bit_vec.fill(false)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you call clear() it works but... d58902a then you have to check whether it's defined or not, later.

@antonok-edm
Copy link

+1. bitvec has an optional alloc feature, so switching from bit-vec also opens up the possibility of building without an allocator. That would make this the first approximate-membership crate with that feature on https://crates.io (at least as far as I could find).

(see also #42)

@antonok-edm
Copy link

I have rebased this branch onto the latest master branch at antonok-edm/rust-bloom-filter#bitvec

@ufoot
Copy link
Author

ufoot commented Oct 27, 2024

No activity here, lack of traction maybe... Closing.

@ufoot ufoot closed this Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants