fix: Fix ZBufWriter implementation

[wyfo]

No description provided.

[wyfo]

@Mallets

[wyfo]

I've run throughput example, and performances after this PR are similar.

[Mallets]

I still prefer to move this ZSliceWriter to ZBuf. Here the ZSliceWriter is writing on a &'a mut Vec<u8>, which is completely fine for the goal of improving ZBufWriter.

However, a ZSlice is characterized by the fact that can contain anything that implements ZSliceBuffer trait. I would expect that a ZSliceWriter would be able to write on anything that would implement something like ZSliceBufferMut. However, this creates more problems than it solves and it is preferable to keep ZSlice a read-only type.

It's really a matter of code organization and naming.

[wyfo]

ZSliceWriter uses internal fields of ZSlice, playing with its invariant, so it should be in the same module.

I would expect that a ZSliceWriter would be able to write on anything that would implement something like ZSliceBufferMut.

That's why writer returns an Option<ZSliceWriter>, and @yellowhatter would answer you that in the future, you may also be able to write on a SHM buffer, and you may also want to change the buffer type to Vec when it's a small size array (it may help to solve the issue of small slices accumulation I've mentioned this morning).

it is preferable to keep ZSlice a read-only type.

This is obviously not the case, as ZBufWriter is supposed to write on the ZSlice. We should just assume that. And again, we should keep invariants at the module scope, that's the unofficial rule of unsafe code.

[Mallets]

ZSliceWriter uses internal fields of ZSlice, playing with its invariant, so it should be in the same module.

Ok

That's why writer returns an Option<ZSliceWriter>, and @yellowhatter would answer you that in the future, you may also be able to write on a SHM buffer, and you may also want to change the buffer type to Vec when it's a small size array (it may help to solve the issue of small slices accumulation I've mentioned this morning).

it is preferable to keep ZSlice a read-only type.

This is obviously not the case, as ZBufWriter is supposed to write on the ZSlice. We should just assume that.

No, we should not. We have already solved the problem of writing in SHM buffer via deserializing a ZBytes into a &zsliceshmmut. A lot of problems were arising while making ZSlice a writable type and mixing different levels of APIs. Moreover, why the writer should be limited to &'a mut Vec<u8> and not any supporting buffer? This is a clear assumption around how we build the infallible ZBufWriter but not how a ZSlice is supposed to operated on generic supporting buffers.

And again, we should keep invariants at the module scope, that's the unofficial rule of unsafe code.

That's ok and understood.

[Mallets]

After a deeper reading, it seems that ZSliceWriter never reference, nor constructs a ZSlice hence never fiddling with ZSlice internal fields. Also writer function performs a downcast_mut which is in the public API. Am I missing something?

EDIT

I see how you take &mut end to change the index. Apart of that, no other internal fiddling seems to be required. Wouldn't be then better to add something like:

impl ZSlice {
unsafe fn set_start(&mut self, start: usize) { .. }
unsafe fn set_end(&mut self, start: usize) { .. }
}

[wyfo]

The writing on SHM buffer idea is about extending the requested capacity to the really allocated one, but that out of the subject for the moment obviously. The only goal of this writer is still to respect the "invariant contained in a single module" rule.

[wyfo]

ZSliceWriter reference both the internal buffer, and the ZSlice.end field, so yes, it fiddles with invariant.
Also, it should not use downcast_mut, because this method has an invariant which is obviously not respected by the writer.

Yes an unsafe set_end could be exposed, but it's still a lot simpler in my opinion to keep the invariant contained in the module. ZSliceWriter implementation is pretty straightforward this way.
I don't understand what bother you so much about this ZSliceWriter. ZSlice is not immutable, we mutate it. Why is restricted to a Vec? it is only because it was our current implementation, but we could also convert every small-sized buffer to a Vec in order to avoid small-slices accumulation.

[Mallets]

It bothers me because multi-months of work have been dedicated to solve the design flaw of having ZSlice writable which was creating more problems than others and led to the design of ZBytes. And I'd like to keep it that way and to not undermine the semantics of the ZSlice for the sole need of ZBufWriter.

[wyfo]

Still, it resulted in a design flaw of unsound and bugged implementation of ZBufWriter, which I try to fix notably by following the rule of invariant contained in single module.
And this writer is as much internal as SingleOrVec, it doesn't make suddenly ZSlice more mutable it was before.

[Mallets]

Ok, then I'd would state it very clearly in a comment in the code that the usage is internal and meant to stay internal to the crate. Any attempt to make it public would require some serious rediscussion. I would also add a link to this very discussion.
I want to avoid that in the future we simply change the pub(crate) to pub without looking at this discussion. Adding a comment would be hence good enough for now and we can move on with the blocked PRs.

[wyfo]

Done