-
Notifications
You must be signed in to change notification settings - Fork 82
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
Feature Request: RoaringBitmap::from_bitmap_bytes
#288
base: main
Are you sure you want to change the base?
Conversation
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.
Hey @lemolatoon 👋
Thank you very much for these changes. The results look very good, indeed. However, could you:
- Write a better explanation of what
offset
means. I understand, but it needs to be clearer. Maybe talk about internal containers that are aligned around 64k values integer groups? - Explain what kind of input is expected in plain text in the function description (endianness, size, alignment).
- Move this function and the test into the serialization module.
Thank you very much for the work!
83af352
to
adbc00b
Compare
Hi @Kerollmops 👋 Thank you for your quick reply. I have just made changes based on your review. Basically I did:
|
adbc00b
to
af5ea01
Compare
af5ea01
to
03a61ad
Compare
roaring/src/bitmap/serialization.rs
Outdated
/// - `offset: u32` - The starting position in the bitmap where the byte slice will be applied, specified in bits. | ||
/// This means that if `offset` is `n`, the first byte in the slice will correspond to the `n`th bit(0-indexed) in the bitmap. | ||
/// Must be a multiple of 8. | ||
/// - `bytes: &[u8]` - The byte slice containing the bitmap data. The bytes are interpreted in little-endian order. |
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.
endianness
doesn't apply within a byte, endianness refers to the order of bytes within a word.
The bitvec
crate refers to this order as "Least-Significant-First" bit order, and I think that's a more accurate description.
roaring/src/bitmap/serialization.rs
Outdated
debug_assert!(first_container_bytes.len() <= number_of_bytes_in_first_container); | ||
// Safety: | ||
// * `first_container_bytes` is a slice of `bytes` and is guaranteed to be smaller than or equal to `number_of_bytes_in_first_container` | ||
unsafe { | ||
core::ptr::copy_nonoverlapping( | ||
first_container_bytes.as_ptr(), | ||
bits_ptr, | ||
first_container_bytes.len(), | ||
) | ||
}; |
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.
Same below, but I don't think this is correct on Big Endian systems. E.g. I've got the bytes [0x01, 0, 0, 0, 0, 0, 0, 0]
. If I plop those down into the u64
s of the bitmap, I've set the least significant byte to 1 on a little endian system, but I've set the most significant byte to 1 on a big endian system. So the least significant bit of the u64 is set for a little endian system, but unset for a big endian system?
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.
I will add tests using miri, which is an interpreter of Rust's intermediate representation. That can even behave like big-endian system. I found the usage in miri's README.
I'll push the additional tests for big endian system (by modifiying CI) and some patch (if needed) later (maybe 2 weeks later).
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.
I am not sure miri will detect algorithm correctness issues.
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.
At least, miri
can emulate a big-endian system, allowing us to ensure that tests also pass on big-endian systems.
@Dr-Emann I've just fixed the documentation and made the implementation endian-aware. |
* Directly create an array/bitmap store based on the count of bits * Use u64 words to count bits and enumerate bits * Store in little endian, then just fixup any touched words * Only use unsafe to reinterpret a whole bitmap container as bytes, everything else is safe * Allow adding bytes up to the last possible offset
we can setting an initial value in that case
Speed up from_bitmap_bytes, and use less unsafe
/// assert!(rb.contains(17)); | ||
/// assert!(rb.contains(39)); | ||
/// ``` | ||
pub fn from_bitmap_bytes(offset: u32, mut bytes: &[u8]) -> RoaringBitmap { |
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.
I'm not sure if I'd rather the name from_lsb0_bytes
be used here, or use from_bitmap_bytes
everywhere instead, but it would be nice to keep them consistent.
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.
IMO, from_lsb0_bytes
can be more concise. from_bitmap_bytes
is named after what I think is the fact that bytes in Bitmap
containers are ordered as Lest-Significant-Bits, regardless of its endian, but actually it is not (in big-endian system we have to reorder internally).
The Feature Explanation
RoaringBitmap
Function Behavior
RoaringBitmap
offset
can be used to offset the passing bitmap's indexoffset
is not aligned to # of bits ofContainer
'sStore::Bitmap
(# of bits ofBox<[u64; 1024]>
), this function panicsMotivation
Sometimes bitmap is calculated by SIMD instructions. The result of SIMD instruction is likely to be already bitmask, not the series of bitmap indicies.
Under current implementation, when you intend use
RoaringBitmap
with bitmask produced by SIMD instruction, you have to useRoaringBitmap::sorted_iter
or just insert one by one.To solve this problem, I implemented
RoaringBitmap::from_bitmap_bytes
, which can be used to construct directly from bitmask.Example of Production of Bitmask by SIMD instructions
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=40ddd13554c171be31fe53893401d40f
Benchmark Result
On my laptop (Apple M3 MacBook Air Sonoma14.3 Memory 16 GB), in most cases
from_bitmap_bytes
is much faster thanfrom_sorted_iter
.Part of Results