feat!: improve serialization efficiency #1467
Conversation
Fixed-size objects are now written directly on the writer, instead of being wrapped first into a ZBytes. Slice like objects are also written on the writer, but obviously prefixed by their size, like ZBytes.
|
PR missing one of the required labels: {'dependencies', 'enhancement', 'new feature', 'bug', 'documentation', 'breaking-change', 'internal'} |
| } | ||
| impl_num!(i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize, f32, f64); |
There was a problem hiding this comment.
I'm not sure implementation for isize/usize makes sense anymore since their sizes are platform dependent (we could always use vle, but the issue is that in some languages they are not independent types and are defined in terms of other integers).
| @@ -38,7 +38,7 @@ async fn main() { | |||
| sample.key_expr().as_str(), | |||
| sample | |||
| .payload() | |||
| .deserialize::<String>() | |||
| .try_deserialize::<String>() | |||
There was a problem hiding this comment.
Why changing the name? There is already quite a bit of material out there, not mentioning all bindings, that uses deserialize. I don't think this change is needed.
There was a problem hiding this comment.
everything has been changed in the last iteration with @DenisBiryukov91 and @kydos, see the coming PR. This one will be closed.
| fn from(t: &Cow<'_, [u8]>) -> Self { | ||
| ZSerde.serialize(t) | ||
| } | ||
| impl SerializedSize for bool { |
There was a problem hiding this comment.
Are we confident that the size of bool will be the same for each platform ?
There was a problem hiding this comment.
I don't thing we care about the size of bool on any platform, if you want to write a compatible serialization, you have to (de)serialize it as a u8, not as an arbitrary integer.
| /// assert_eq!(buf.as_slice(), deser.as_slice()); | ||
| /// ``` | ||
| /// | ||
| /// If you want to be sure that no copy is performed at all, then you should use [`ZBytes::slices`]. | ||
| /// Please note that in this case data may not be contiguous in memory and it is the responsibility of the user to properly parse the raw slices. | ||
| pub fn into<'a, T>(&'a self) -> T |
There was a problem hiding this comment.
I don't think we need to introduce this API change.
See my previous comment.
| ZBytes::new(t) | ||
| impl Deserialize<'_> for Vec<u8> { | ||
| fn read(_reader: &mut ZBytesReader) -> Result<Self, ZDeserializeError> { | ||
| unimplemented!() |
There was a problem hiding this comment.
It should be implemented at some point :)
There was a problem hiding this comment.
No it doesn't, because it should never be called, as the the semantic was:
- call
readif the size is fixed (not the case forVec<T>) - call
deserialize/try_from_zbytesif the size is variable
| pub trait Deserialize<'a>: Sized + SerializedSize { | ||
| fn read(reader: &mut ZBytesReader) -> Result<Self, ZDeserializeError>; | ||
| fn deserialize(zbytes: &'a ZBytes) -> Result<Self, ZDeserializeError>; | ||
| fn try_from_zbytes(zbytes: ZBytes) -> Result<Self, ZDeserializeError>; |
There was a problem hiding this comment.
What's the purpose of try_from_zbytes as part of the trait? It seems not needed to me.
There was a problem hiding this comment.
Even if everything will be dropped so we don't care anymore, here was the reason:
deserialize couldn't be call in the context of an iterator when you deserialize first a ZBytes, which would then have an anonymous lifetime. So deserializing an iterator of Cow<'a, [u8]> was not possible as 'a would be anonymous in the implementation.
That's why two different methods were needed, one for ZBytes::deserialize, and the other one for the iterator.
Fixed-size objects are now written directly on the writer, instead of being wrapped first into a ZBytes.
Slice like objects are also written on the writer, but obviously prefixed by their size, like ZBytes.