Fix chained .slice to not produce nested Slice wrappers #163
Conversation
The module's code is no longer in use.
b"hello".to_vec().slice(1..).slice(..3) produces a Slice<Slice<Vec<u8>>>. The API makes it easy to inadvertendly end up with nested Slice wrappers, which adds unnecessary overhead to the buffer accessor methods and makes the results of .begin() and .end() difficult to interpret. Introduce trait IntoSlice to uniformly convert all buffer parameters to a Slice<T>, with the impl of InfoSlice for Slice<T> closing onto Self. The buffer parameter type in the generic I/O operation methods is bound with IntoSlice to keep the polymorphism good, while reducing code bloat in monomorphisation when the application passes both buffers and their slices to tokio-uring operations. Consequently, all buffers returned from the operations are wrapped into Slice.
Judging by the impls and the tests, it's clearly meant to work as a high water mark for initialized portion of the buffer, there is no setting the position back. The documentation on Slice::set_init has already been made correct, so it makes sense to fix this in the same PR.
The doc tests need update after the original IntoSlice refactoring.
There's no need to expose raw buffer access methods of Slice in the public API.
|
Nice, the synergies of multiple sets of eyes. I'm going to hold off reviewing until tomorrow. I am very interested in comparing this set against what I had reviewed in #53. I might pull both branches and run git vimdiff against the files in the two tips unless there is an easier way I can be pointed to. |
|
@mzabaluev Thanks for the confirmation. I've started taking a look. |
|
Random musings: rust 1.65 is released today. Does the GAT support make any of this crate's IoBuf/Slice support even cleaner? Should some of the crate's unit tests work with the bytes type when the bytes feature is enabled? |
|
I still think we could go further, although maybe these items are for a different PR IoBuf and IoBufMut are used for several purposes:
w.r.t w.r.t w.r.t |
|
@ollie-etl Interesting. Yes, let me finish my review. @Noah-Kennedy and @mzabaluev can then decide what to do from there. |
|
@ollie-etl @FrankReh I went over this on #161, but IoBuf is needed, at the very least as a marker type. |
|
Unpin just means "I can move this", it does not guarantee a stable pointer from a deref. We need our own trait to signal this. |
|
I should be able to take a look at this tonight. |
Clarify the behavior of Slice::slice, mention the From<T: IoBuf> conversion for Slice.
The impl Into<Slice<T> buffer arguments and their corresponding Slice<T> returned values do require some more explaining.
Reduce generic parameters on the implementation of write_all to just the buffer type, to cut back on code bloat and improve branch prediction.
|
@mzabaluev Another PR was merged today that added two functions, write_all_at and read_exact_at. Do you want to update your branch and handle those two, they come with a unit test each I think. Or would you like me to? |
|
@FrankReh I will update the branch. |
Post-merge fix, this method was recently added.
Will no longer work after slice reform.
Remove checks for ErrorKind::Interrupted from: - File::read_exact_at - File::write_all_at As mentioned in the implementation of write_all for sockets, this error code should never be returned with the way the runtime drives io-uring. Also update the similar comments in net that needed a bit more editing since the commented-out match arms were removed.
Another post-merge fix for a recently added method.
Describe the semantics of the returned Slice.
| /// | ||
| /// If the method returns [`Ok(())`], then the read was successful. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// If this function encounters an "end of file" before completely filling | ||
| /// the buffer, it returns an error of the kind [`ErrorKind::UnexpectedEof`]. | ||
| /// The buffer is returned on error. | ||
| /// The buffer is returned on error as described above. |
There was a problem hiding this comment.
Maybe add, "but the data that could be read will have been written to the buffer.
So it's clear that when there's an error, the buffer may have been written to somewhat. These write_all type functions are just helpers and can't be expected to be magic. If the caller cares, they should make their own copy beforehand.
| } | ||
|
|
||
| impl<T: IoBufMut> Op<Read<T>> { | ||
| pub(crate) fn read_at(fd: &SharedFd, buf: T, offset: u64) -> io::Result<Op<Read<T>>> { | ||
| pub(crate) fn read_at(fd: &SharedFd, buf: Slice<T>, offset: u64) -> io::Result<Op<Read<T>>> { |
There was a problem hiding this comment.
I'm not sure how I feel about changing the APIs like this. Why is this necessary?
There was a problem hiding this comment.
I also feel unhappy about Slice popping up everywhere even when not needed, and perhaps it's not justified by fixing .slice(..).slice(..).
I've got another idea which I'm going to try first.
|
Superseded by #172. |
Introduce new traits, `BoundedBuf` and `BoundedBufMut`, taking the `slice` method and related infrastructure off `IoBuf`. `Slice` no longer implements `IoBuf`/`IoBufMut`, instead it does `BoundedBuf` and `BoundedBufMut`. These traits are used as bounds in most generic I/O methods in place of `IoBuf`/`IoBufMut`. The purpose of this is to get rid of `Slice<Slice<T>>` arising from chained slice calls. * Any buffer the user has can be defined to be an IoBuf (subject to the safety constraints of IoBuf) * An IoBuf is by definition a BoundedBuf * BoundedBuf<IoBuf>.slice(range) -> Slice<IoBuf> (i.e. slicing an IoBuf BoundedBuf produces an IoBuf Slice) * A Slice<IoBuf> is also a BoundedBuf by definition * And to void the problem of a Slice<Slice<>> hierarchy building up * A BoundedBuf<Slice<IoBuf>>.slice(range) -> Slice<IoBuf> (keeping the slice flat) All calls that take a BoundedBuf input type can take an IoBuf and a Slice<IoBuf>. And a brief explanation for the few functions that were split into two layers, a generic public layer and a slice specific private layer: The purpose of factoring out the bulk of the public method's body into the private method working over Slice is twofold: * Unify the code path recovering the buffer passed in for the BufResult, rather than have it replicated in multiple return expressions as in #163; * Reduce monomorphization bloat, so that most of the machine code to work with buffers passed in as, say, Vec<u8> and Slice<Vec<u8>> is emitted in one instance, rather than replicated instances of exactly the same code.
A rework of #53 without introducing a utility trait, based in part on suggestions in #53 (comment)
Sliceno longer implementsIoBuf, but gains an inherentslicemethod that returns aSelfsubslice.There is
impl<T: IoBuf> From<T> for Slice<T>, and the generic buffers in the API methods are bound asimpl Into<Slice<T>>.Like in #53, the buffers are returned back as
Slice<T>.