Upgrade to bitvec 1.0.0 #246
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wcampbell0x2a
requested changes
Jan 18, 2022
| @@ -77,7 +77,7 @@ fn emit_struct(input: &DekuData) -> Result<TokenStream, syn::Error> { | |||
| let __deku_pad = 8 * ((__deku_rest.len() + 7) / 8) - __deku_rest.len(); | |||
| let __deku_read_idx = __deku_input_bits.len() - (__deku_rest.len() + __deku_pad); | |||
|
|
|||
| Ok(((__deku_input_bits[__deku_read_idx..].as_raw_slice(), __deku_pad), __deku_value)) | |||
| Ok(((__deku_input_bits[__deku_read_idx..].domain().region().unwrap().1, __deku_pad), __deku_value)) | |||
There was a problem hiding this comment.
I'd much rather prefer naming the tuple field.
Suggested change
| Ok(((__deku_input_bits[__deku_read_idx..].domain().region().unwrap().1, __deku_pad), __deku_value)) | |
| let (_, remaining_bytes, _) = __deku_input_bits[__deku_read_idx..].domain().region().unwrap(); | |
| Ok(((remaining_bytes, __deku_pad), __deku_value)) |
There was a problem hiding this comment.
I could also just make the tuples an actual named struct here.
| @@ -308,7 +308,7 @@ fn emit_enum(input: &DekuData) -> Result<TokenStream, syn::Error> { | |||
| let __deku_pad = 8 * ((__deku_rest.len() + 7) / 8) - __deku_rest.len(); | |||
| let __deku_read_idx = __deku_input_bits.len() - (__deku_rest.len() + __deku_pad); | |||
|
|
|||
| Ok(((__deku_input_bits[__deku_read_idx..].as_raw_slice(), __deku_pad), __deku_value)) | |||
| Ok(((__deku_input_bits[__deku_read_idx..].domain().region().unwrap().1, __deku_pad), __deku_value)) | |||
| @@ -453,7 +453,7 @@ fn emit_bit_byte_offsets( | |||
| || byte_offset.is_some() | |||
| { | |||
| Some(quote! { | |||
| let __deku_bit_offset = usize::try_from(__deku_input_bits.offset_from(__deku_rest))?; | |||
| let __deku_bit_offset = usize::try_from(unsafe { __deku_rest.as_bitptr().offset_from(__deku_input_bits.as_bitptr()) } )?; | |||
There was a problem hiding this comment.
Lets add a // SAFETY for proving that this is fine, something like: https://github.com/bitvecto-rs/bitvec/blob/a7a82c67206d6e3ced67294883f6d1baaae7d74c/src/ptr/single.rs#L604
There was a problem hiding this comment.
Why are here so many unsafes
| let mut res = HashMap::with_capacity_and_hasher(capacity.unwrap_or(0), S::default()); | ||
|
|
||
| let mut rest = input; | ||
| let mut found_predicate = false; | ||
|
|
||
| while !found_predicate { | ||
| let (new_rest, kv) = <(K, V)>::read(rest, ctx)?; | ||
| found_predicate = predicate(input.offset_from(new_rest) as usize, &kv); | ||
| found_predicate = predicate(unsafe { new_rest.as_bitptr().offset_from(input.as_bitptr()) } as usize, &kv); |
| @@ -34,11 +34,11 @@ macro_rules! ImplDekuTraits { | |||
|
|
|||
| let value = if pad == 0 | |||
| && bit_slice.len() == max_type_bits | |||
| && bit_slice.as_raw_slice().len() * 8 == max_type_bits | |||
| && bit_slice.domain().region().unwrap().1.len() * 8 == max_type_bits | |||
| { | ||
| // if everything is aligned, just read the value | ||
|
|
||
| let bytes: &[u8] = bit_slice.as_raw_slice(); | ||
| let bytes: &[u8] = bit_slice.domain().region().unwrap().1; |
| @@ -81,7 +81,7 @@ macro_rules! ImplDekuTraits { | |||
| bits | |||
| }; | |||
|
|
|||
| let bytes: &[u8] = bits.as_raw_slice(); | |||
| let bytes: &[u8] = bits.domain().region().unwrap().1; | |||
| @@ -25,8 +25,8 @@ where | |||
| let (new_rest, val) = u8::read(rest, ctx)?; | |||
| rest = new_rest; | |||
|
|
|||
| let read_idx = input.offset_from(rest) as usize; | |||
| value = input[..read_idx].as_raw_slice(); | |||
| let read_idx = unsafe { rest.as_bitptr().offset_from(input.as_bitptr()) } as usize; | |||
| match limit { | ||
| // Read a given count of elements | ||
| Limit::Count(mut count) => { | ||
| // Handle the trivial case of reading an empty slice | ||
| if count == 0 { | ||
| return Ok((input, &input.as_raw_slice()[..0])); | ||
| return Ok((input, &input.domain().region().unwrap().1[..0])); |
| @@ -33,7 +33,7 @@ fn read_vec_with_predicate< | |||
|
|
|||
| // This unwrap is safe as we are pushing to the vec immediately before it, | |||
| // so there will always be a last element | |||
| if predicate(input.offset_from(rest) as usize, res.last().unwrap()) { | |||
| if predicate(unsafe { rest.as_bitptr().offset_from(input.as_bitptr()) } as usize, res.last().unwrap()) { | |||
|
Running the benchmarks (which aren't a full holistic picture of this library), gives me some worrying numbers. 😬 These are ran on my cheap laptop: origin/master..JuanPotato/master origin/master for reference: |
|
Running benchmarks on my adsb library after switching deku to this branch: |
|
Thank you for the feedback! Yeah that's not great performance, and I'm also thinking my code might have some bugs in it in very specific cases, Will add some new tests and see. Also not sure if bitvec itself regressed or if our usage of it is not ideal. |
There is this issue, it might not be related to our performance regression however. ferrilab/bitvec#158 |
|
Thanks for this work! Let's see if we can figure out the performance issue before merging. |
|
I ran the benchmark executables with perf. master: JuanPotato:master: Just by refactoring https://github.com/JuanPotato/deku/blob/09427b8eb5e39402c4dec49070e9ae0a5ecbe20d/src/impls/primitive.rs#L35 with a match and thus reusing But even this is of course still much slower than the master branch. |
|
At some point I began working on a replacement to bitvec that would be much more performant for deku's usage. I got it looking pretty good but it required a lot of refactoring and is definitely a breaking change. Can't remember how the performance compares to master though |
|
@myrrlyn I see you updated bitvec w.r.t. performance with the ferrilab/bitvec@4de6277 commit. When updating the diff --git a/Cargo.toml b/Cargo.toml
index 14d1a0a..ebfb0c0 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -26,7 +26,7 @@ const_generics = []
[dependencies]
deku_derive = { version = "^0.12.0", path = "deku-derive", default-features = false}
-bitvec = { version = "1.0.0", default-features = false }
+bitvec = { git = "https://github.com/bitvecto-rs/bitvec", default-features = false }I get performance boosts vs the v1 version of bitvec: However, still not the kind of performance that current deku provides: |
|
By any chance, does the faster old version have |
I'm not at my computer, but I think this branch is the code that's used for our code. Looks like everything possible is inlined? |
|
as of bitvec v1.0.1, compared to old bitvec: |
|
Better https://github.com/wcampbell0x2a/deku/tree/update-bitvec-1.x.x: |
Merged
|
Superseded by #281 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes I made
BitSlice<Msb0, u8>->BitSlice<u8, Msb0>get_raw_sliceis gone, usedomainoffset_fromis unsafe and needs to be called onBitPtrand is reversedNot related to the upgrade but it made my tests pass. DekuData.vis was never read and that emitted a warning. Cargo test didn't like this extra warning so I renamed it to _vis for now.
closes #243