Refactor BitReader to contain explicit state (#1282) #1283
Conversation
|
|
||
| // First align bit offset to byte offset | ||
| if let BitReaderState::Unaligned(unaligned) = &mut self.state { | ||
| while values_to_read != values_read && (unaligned.bit_offset % 8) != 0 { |
There was a problem hiding this comment.
There is a slight change here - we align to the byte not u64. I think this was unintentional, as the comment said byte offset and there is no reason to align to u64.
| let in_buf = &self.buffer.data()[self.byte_offset..]; | ||
| assert!(in_buf.len() * 8 >= to_read * bit_width); | ||
|
|
||
| let mut in_ptr = in_buf as *const [u8] as *const u8 as *const u32; |
There was a problem hiding this comment.
This block is simply moved, with some slightly more verbose naming
Codecov Report
@@ Coverage Diff @@
## master #1283 +/- ##
=======================================
Coverage 83.02% 83.02%
=======================================
Files 180 180
Lines 52269 52306 +37
=======================================
+ Hits 43394 43427 +33
- Misses 8875 8879 +4
Continue to review full report at Codecov.
|
| /// | ||
| /// Returns `None` if there's not enough data available. `Some` otherwise. | ||
| pub fn get_value<T: FromBytes>(&mut self, num_bits: usize) -> Option<T> { | ||
| self.state.as_unaligned().get(num_bits) |
There was a problem hiding this comment.
Not sure whether there's any performance implication here since it now requires some extra compare & jump instructions.
There was a problem hiding this comment.
Running the benchmarks if anything I see a teeny performance improvement, this is likely because whilst there is a branch here, it isn't typically called in a loop.
Although DeltaBitPackDecoder is an exception here, but that's what #1284 is for 😄
|
Going to get #1284 in first and then revisit this |
|
Going to close this as it wasn't needed for #1284 |
3 participants
Which issue does this PR close?
Closes #1282.
Rationale for this change
I think it makes the logic easier to follow, although this may be a subjective assessment
What changes are included in this PR?
Refactors BitReader to explicitly separate the aligned and unaligned operations
Are there any user-facing changes?
No