-
Notifications
You must be signed in to change notification settings - Fork 84
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
Branchless array-bitmap operations #232
Conversation
pairwise_and benchmarks (ran on a laptop, there might be some variability)
|
This commit implements a faster version of `Vec::retain` for array stores. Fixes RoaringBitmap#206.
9bcc178
to
5d8627c
Compare
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.
This looks very good! Thank you! It's impressive how a simple impl like that can be faster than the standard impl...
bors merge
Build succeeded: |
232: Branchless array-bitmap operations r=Kerollmops a=RaduBerinde This commit implements a faster version of `Vec::retain` for array stores. Fixes RoaringBitmap#206. Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
@@ -205,6 +205,23 @@ impl ArrayStore { | |||
pub fn as_slice(&self) -> &[u16] { | |||
&self.vec | |||
} | |||
|
|||
/// Retains only the elements specified by the predicate. | |||
pub fn retain(&mut self, mut f: impl FnMut(u16) -> bool) { |
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.
Should this be unsafe? If the supplied f
doesn't uphold the invariant
// SAFETY: pos is always at most i because
f(val) as usize
is at most 1.
I suspect this will lead to undefined behavior.
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.
f(val)
is bool
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.
Ooops didn't look at the return type in the signature.
This commit implements a faster version of
Vec::retain
for arraystores.
Fixes #206.