-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
Upgrade to bitvec 1.0.0 #246
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.
Thanks for the contribution! I have some comments but it looks good :)
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd much rather prefer naming the tuple field.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) |
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 here.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
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 as above
@@ -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 |
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 as above
{ | ||
// 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; |
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 as above
@@ -81,7 +81,7 @@ macro_rules! ImplDekuTraits { | |||
bits | |||
}; | |||
|
|||
let bytes: &[u8] = bits.as_raw_slice(); | |||
let bytes: &[u8] = bits.domain().region().unwrap().1; |
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 as above
@@ -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; |
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 as above
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])); |
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 as above
@@ -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()) { |
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 as above
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. 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:
|
Superseded by #281 |
What changes I made
BitSlice<Msb0, u8>
->BitSlice<u8, Msb0>
get_raw_slice
is gone, usedomain
offset_from
is unsafe and needs to be called onBitPtr
and 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