Simplify API

[mgeier]

This is a different potential way to go (compared to #52), which reduces the API surface instead of extending it.

This is a breaking change (mostly just removing .split()).

Auto-generated docs as ZIP file: https://github.com/mgeier/rtrb/suites/3125739092/artifacts/71657611.

[PieterPenninckx]

What happens if commit, commit_iterated() or commit_all is not called? My guess is that the consumer cannot see the items that have been written and that the items written can be overwritten by calling "push()" (or similar) again, which would cause them to be leaked. Is that correct?

[mgeier]

Exactly.

I've just added a commit (7bb6e5d) that hopefully clarifies that.

[PieterPenninckx]

I like the removal of split since this makes the most common use case simpler :-)

There's some asymmetry between the "normal" API were items are moved into and out of the ring buffer and the "chunked" API where the user of the API is given access (via &mut and & references) to the data in the buffer. This makes the "chunked" API less suitable for items that implement Drop. The "chunked" API seems to be geared towards data types that implement Copy + Default. For instance, you can use "chunked" API to transfer Vec<T>'s, but you need to clone the whole Vec and the Vec you have just sent will be dropped. If ReadChunk's as_slice method would give &mut slices, you could use the mem::replace trick to avoid needless memory allocation and de-allocation, but currently the slices are as immutable.

Thinking about it this way, I would suggest to consider replacing the "chunked" API by some other methods:

• on the Producer
  • a method to push all items from an iterator "in one go" (chunked) onto the ring buffer
  • a method to copy all elements from a slice into the ring buffer
• on the Consumer
  • a method that gives an iterator to iterate over all items in the ring buffer that moves them "in one go"
  • a method to copy move all elements from the ring buffer into a slice

Just my two cents, this might actually not be a good idea after all. I'm happy that somebody is actually maintaining this crate in his spare time, so I really can't complain.

Edit: I meant moving elements from ring buffer into slice.

[mgeier]

Thanks for your suggestions @PieterPenninckx, I very much appreciate this!

[...] This makes the "chunked" API less suitable for items that implement Drop. The "chunked" API seems to be geared towards data types that implement Copy + Default.

Well, yes, for me the main usage is with chunks of f32.

But items don't actually have to be Copy (I have removed the trait bound in this PR: 148cf65) and they only have to implement Default for safe chunk-writing.

For instance, you can use "chunked" API to transfer Vec<T>'s, but you need to clone the whole Vec and the Vec you have just sent will be dropped.

Yes indeed, in this case multiple push() calls would be more appropriate (for now).

An extension for batch-moving has already been requested in #56.

If ReadChunk's as_slice method would give &mut slices, you could use the mem::replace trick to avoid needless memory allocation and de-allocation, but currently the slices are as immutable.

Yes, but wouldn't it be strange to return a writable slice for a read operation?
Granted, read_chunk() already takes a &mut self, which also might seem strange.

And the mem::replace() trick would require a way to cheaply construct a "dummy" T, which might not be feasible for some types.

I think it would be better to provide a separate way for batch-moving.

Thinking about it this way, I would suggest to consider replacing the "chunked" API by some other methods:

I wouldn't want to replace the slice functions, because they allow very efficient copy_from_slice() and similar.

The iterator API could probably be replaced, though.

• on the Producer
  • a method to push all items from an iterator "in one go" (chunked) onto the ring buffer

Yeah, that would be possible, let's discuss this in #56.

• a method to copy all elements from a slice into the ring buffer

I'm not sure whether this is "fundamental" enough to deserve its own method.

I've provided an example for how to implement this as a user in: https://docs.rs/rtrb/0.1.4/rtrb/#common-access-patterns

This also shows that there are multiple possible behaviors. Which one would we choose for an "official" method?

[PieterPenninckx]

I wouldn't want to replace the slice functions, because they allow very efficient copy_from_slice() and similar.

I proposed a method to copy all elements from a slice into the ring buffer on the producer side and a method to copy all elements from the ring buffer into a slice on the consumer side. That would be do exactly the efficient copy_from_slice(). You mention some that there are other, similar techniques that are enabled by exposing slices. What are these?

I'm not sure whether this is "fundamental" enough to deserve its own method.

I've provided an example for how to implement this as a user in: https://docs.rs/rtrb/0.1.4/rtrb/#common-access-patterns

This also shows that there are multiple possible behaviors. Which one would we choose for an "official" method?

I would go for the push_partial_slice, but without the Default bound if that's possible. For the consumer side, I was thinking about moving the items from the ring buffer into the slice. I used the word "copy" in my comment where I meant "move" (sorry about that, I've edited the comment). With the moving, it's something that cannot be implemented on top of the API currently proposed in this pull request, so it's fundamental in some sense.

Reading your comments, I'm not so sure anymore about the design that I proposed. The reason is that, when it comes to slices, it only allows for the slice -> ring buffer copying and ring buffer -> slice moving, but maybe there are other things you can do with slices and that are not possible with the design that I proposed.

[mgeier]

copy_from_slice() [...] You mention some that there are other, similar techniques that are enabled by exposing slices. What are these?

I thought there would be also copy_to_slice() or something similar, but I was wrong. I think I was confusing this with the kinda similar pointer operations like e.g. pointer::copy_to().
Sorry for the confusion!

Anyway, I have another use case for the 2-slices API: using fixed-size chunks (which led me to create #50 and #52 and finally this very PR). Since I'm not planning to introduce additional API for fixed-size chunks anymore, I'll have to use the existing chunks-and-slices API, and for that I need direct access to the first of the two slices.

And apart from that, I just have the feeling that the two-slices API is the "classic" low-level representation of SPSC ring buffers. I would like to go as low-level as possible (without exposing raw pointers, though).

I would go for the push_partial_slice, but without the Default bound if that's possible.

Yes, that's certainly possible, I just didn't want to show too much unsafe code in the documentation.

I'm using the unsafe version in the implementation of std::io::Write:

rtrb/src/lib.rs

Lines 1471 to 1489 in 272f9d7

	fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
	use ChunkError::TooFewSlots;
	let mut chunk = match self.write_chunk_uninit(buf.len()) {
	Ok(chunk) => chunk,
	Err(TooFewSlots(0)) => return Err(std::io::ErrorKind::WouldBlock.into()),
	Err(TooFewSlots(n)) => self.write_chunk_uninit(n).unwrap(),
	};
	let end = chunk.len();
	let (first, second) = chunk.as_mut_slices();
	let mid = first.len();
	// NB: If buf.is_empty(), chunk will be empty as well and the following are no-ops:
	buf[..mid].copy_to_uninit(first);
	buf[mid..end].copy_to_uninit(second);
	// Safety: All slots have been initialized
	unsafe {
	chunk.commit_all();
	}
	Ok(end)
	}

I haven't needed it myself so far, but I'm starting to realize that exposing this function would be indeed helpful, because then users don't need to use unsafe to get the most efficient slice-copying.

And then I could use that new function to implement std::io::Write.

For the consumer side, I was thinking about moving the items from the ring buffer into the slice.

I guess you mean using the mem::replace() trick?

I think this would be less efficient for the T: Copy case, right?

With the moving, it's something that cannot be implemented on top of the API currently proposed in this pull request, so it's fundamental in some sense.

That's true, it's not possible with the current API, but I think this could be added as separate API as discussed in #56.

The problem with "moving non-Copy items into a mutable slice" is that the user still has to initially fill the slice with valid items, which might be non-trivial.

The ringbuf crate provides a method where the user passes a mutable slice of MaybeUninit<T> (https://docs.rs/ringbuf/0.2.5/ringbuf/struct.Consumer.html#method.pop_copy), but I don't think this is nice to use. But it avoids the redundant initialization part. Note that this uses the "pointer copy" functions, but in Rust semantics this actually does a "move", so the name pop_copy() might be misleading (it doesn't require T: Copy).

maybe there are other things you can do with slices and that are not possible with the design that I proposed.

As I mentioned above, there's my fixed-chunks use case.

Apart from that I don't know any concrete and real use cases, but there is at least one theoretical use case: You want to "peek" at several items and only later decide whether you actually want to consume them or not.

[mgeier]

I've merged this in order to make progress on #56.

However, I'm still very much interested in continuing the discussion here.

I've also created a new issue as a reminder for the planned slice-copy API: #59. Feel free to add comments either here or over there.

[mgeier]

I've just created a possible implementation of the discussed slice API: #66.