Zslice kind #1290
Zslice kind #1290
Conversation
|
I've run throughput example, and performances after this PR are better, as expected. |
| @@ -211,65 +201,9 @@ impl AsRef<[u8]> for ZSlice { | |||
| } | |||
| } | |||
|
|
|||
| impl Index<usize> for ZSlice { | |||
There was a problem hiding this comment.
(This change is brought by #1289 and should be discussed there)
Because they are not needed. Rust indexing syntax already includes an auto-deref. I know that some standard collections like Vec reimplement Index (by just manually doing the deref), but I suspect it may comes from a time where there was no auto-deref.
On the other hand, bytes::Bytes doesn't implement Index.
| T: Any, | ||
| { | ||
| Arc::get_mut(&mut self.buf).and_then(|val| val.as_any_mut().downcast_mut::<T>()) | ||
| pub unsafe fn downcast_mut<T: Any>(&mut self) -> Option<&mut T> { |
There was a problem hiding this comment.
Why marking this function as unsafe? Arc::get_mut should already guarantee that reference count is 1 and it is safe to mutate.
There was a problem hiding this comment.
(This change is brought by #1289 and should be discussed there)
The answer is in the safety comment. It may be safe to mutate the inner buffer inside the Arc, but ZSlice still have an invariant to maintain. If the function was safe, you would be able to truncate the vector, and the end would then be invalid. Thus, we need to ensure that the invariant is maintained, i.e. start/end remain valid for the modified slice. The simple precondition for that is to have the slice size unmodified, as it is the case for SHM buffers which are the only use case of this method.
| let mut zslice: ZSlice = self.codec.read(&mut *reader)?; | ||
| zslice.kind = ZSliceKind::ShmPtr; | ||
| zbuf.push_zslice(zslice); | ||
| let vec: Vec<u8> = self.codec.read(&mut *reader)?; |
There was a problem hiding this comment.
We should read a ZSlice here and not a Vec<u8> which always causes an allocation and a copy.
This is something that should be avoided as much as possible.
In short, while this PR reduces the size on the stack of the ZSlice, it introduces an allocation for every ShmBufInfoBuffer.
There was a problem hiding this comment.
Indeed, forget about that again... I'm not sure there is a simple way to handle that. In fact the issue is our two-phase deserialization, first into a ZSlice containing a ShmBufInfo, then into a ZSlice containing a SHM buffer.
Why don't we do both at once?
There was a problem hiding this comment.
We can definitely define a specific codec (e.g. ZenohZSliceSHM) to directly decode a ZSlice containing a ShmBufInfoBuffer. The code would look like something like:
{
...
SHM_PTR => {
let shm_codec = ZenohZSliceSHM::new();
let zslice: ZSlice = shm_codec.read(&mut *reader)?;
zbuf.push_zslice(zslice);
}
}
struct ZenohZSliceSHM;
impl<R> RCodec<ZSlice, &mut R> for ZenohZSliceSHM
where
R: Reader,
{
// Do everything all-in-one
}
| @@ -228,3 +228,20 @@ impl ZSliceBuffer for ShmBufInner { | |||
| self | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug)] | |||
| pub struct ShmBufInfoBuffer(pub Vec<u8>); | |||
There was a problem hiding this comment.
I believe ShmBufInfoBuffer can simply be a transparent wrapper around a ZSlice so as to potentially avoid an allocation during deserialization? I'm not sure this will work out but we need to find a way to not have any allocation in deserialization, especially for SHM.
There was a problem hiding this comment.
You cannot use a wrapper around ZSlice, otherwise you would not be able to use it in ZBuf.
|
As this PR is neither urgent nor important, it can be postponed or closed because my implementation isn't good indeed. |
Requires #1289 before being merged