-
Notifications
You must be signed in to change notification settings - Fork 7
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
Implement Fracpack definition_will_not_change in rust and fixed arrays #37
Conversation
return Err(Error::ReadPastEnd); | ||
} | ||
|
||
let mut items: Vec<T> = Vec::with_capacity(N); |
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.
Very unfortunate. I hope they fix the need to use Vec someday.
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.
Agreed. 😢
This was the closest that I could try: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initializing-an-array-element-by-element
But it does not work....
let data = {
let mut data: [MaybeUninit<T>; N] = unsafe { MaybeUninit::uninit().assume_init() };
for elem in &mut data[..] {
*elem = MaybeUninit::new(T::embedded_unpack(src, pos, &mut heap_pos)?);
}
unsafe { mem::transmute::<_, [T; N]>(data) }
};
error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
--> fracpack/src/fracpack.rs:675:22
|
675 | unsafe { mem::transmute::<_, [T; N]>(data) }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: source type: `[MaybeUninit<T>; N]` (this type does not have a fixed size)
= note: target type: `[T; N]` (this type does not have a fixed size)
Issue: rust-lang/rust#61956
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.
The llvm backend has an optimization pass which sometimes eliminates heap usage. It's really tough to predict when it does it though.
let name = &input.ident; | ||
let generics = &input.generics; | ||
let fields = struct_fields(data); | ||
let fixed_size = fields | ||
.iter() | ||
.map(|field| { | ||
let ty = &field.ty; | ||
quote! {<#ty as fracpack::Packable>::FIXED_SIZE} | ||
quote! {if <#ty as fracpack::Packable>::USE_HEAP { |
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 change will be unnecessary after fix to [T;N]'s FIXED_SIZE
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.
nice catch. I leaked the responsibility of the nested type, thanks for bringing me back here!
fd4b1cf
to
5c7733b
Compare
@@ -64,6 +81,13 @@ fn process_struct(input: &DeriveInput, data: &DataStruct) -> TokenStream { | |||
quote! {<#ty as fracpack::Packable>::FIXED_SIZE} | |||
}) | |||
.fold(quote! {0}, |acc, new| quote! {#acc + #new}); | |||
let use_heap = fields |
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 always be true when !definition_will_not_change
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.
Maybe the name of this variable is misleading but the Fracpackable::USE_HEAP actually reflects this exact function of cpp: https://github.com/gofractally/psibase/blob/sparkplug0025/fracpack-arrays/libraries/psio/include/psio/fracpack.hpp#L317-L329
What you are referring to is to the following blocks, where we dont write to (or unpack from) the heap:
let pack_heap = if !opts.definition_will_not_change {
quote! { <u16 as fracpack::Packable>::pack(&(heap as u16), dest); }
} else {
quote! {}
};
let unpack_heap_size = if !opts.definition_will_not_change {
quote! { let heap_size = <u16 as fracpack::Packable>::unpack(src, pos)?; }
} else {
quote! { let heap_size = #fixed_size; }
};
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.
Look at line 309
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.
oh, nice catch! fixed!
685ca46
to
69b23e8
Compare
69b23e8
to
66cf4c9
Compare
No description provided.