Improve the ergonomics of .slice with IntoSlice trait #53

mzabaluev · 2021-09-07T21:59:16Z

The owned slice API has a flaw: b"hello".to_vec().slice(1..).slice(..3) produces a
Slice<Slice<Vec<u8>>>.
It is easy to inadvertently 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.

Move the slice method to a new trait IntoSlice, which is used to uniformly convert buffer parameters to a Slice<T>, with the impl of InfoSlice for Slice<T> closing onto Slice<T>.

Instead of IoBuf which can no longer be implemented for Slice, the buffer parameter type in the generic I/O operation methods is bound with IntoSlice to keep the convenient polymorphism, while reducing code bloat in monomorphization when the application passes both buffers and their slices to tokio-uring operations. Consequently, all buffers returned from the operations are wrapped into Slice.

This PR incorporates #52 in order to get tests to pass.

mzabaluev · 2021-09-07T22:25:01Z

src/buf/slice.rs

+    ///
+    /// This method is to be used by the `tokio-uring` runtime and it is not
+    /// expected for users to call it directly.
+    pub fn stable_ptr(&self) -> *const u8 {


I'm not sure it's a good idea to combine inherent methods with the Deref impl, as per the API guidelines C-DEREF, C-SMART-PTR.

As these methods are not meant to be called by the API user, perhaps they should be turned into associated functions and/or made private.

On the other hand, the Deref / DerefMut impls for Slice could be considered for removal in favor of explicit accessor methods like .as_bytes() / .as_mut_bytes(). Especially if we'd want to equip Slice with more non-slicey methods for appending to buffers, like .put_bytes, .put_u32_le, etc.

explicit accessor methods like .as_bytes() / .as_mut_bytes()

... or a std::ops::Index impl, as this is what the users would need the most from a buffer, and [..] is a concise adapter syntax for anything else where buffer contents would be used as a byte slice.

I have realized that the IoBuf/IoBufMut-cum-inherent-Slice methods do not need to be public, so I've made them pub(crate). But the above comments still apply to the other inherent methods of Slice.

Noah-Kennedy · 2022-03-09T02:32:58Z

@mzabaluev any interest in completing this?

mzabaluev · 2022-03-17T16:06:05Z

@Noah-Kennedy Yes, I should update the PR at least, will get to it soon.

mzabaluev · 2022-03-21T03:19:56Z

@Noah-Kennedy I have rebased the branch against master and updated the newly added socket ops SendTo and RecvFrom along with their user-facing methods to the IntoSlice API.

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.

FrankReh · 2022-10-26T10:58:49Z

@mzabaluev I know you were asked this before. Any interest in merging with master and pushing up changes again?

This looks like it was going to be important and @Noah-Kennedy was happy with the direction but was left requiring another review. I would like to help if I can.

FrankReh · 2022-10-28T03:44:20Z

@Noah-Kennedy Should this go in? I can work on rebasing.

mzabaluev · 2022-10-28T09:05:39Z

I will try to update this over the weekend.

FrankReh · 2022-10-31T11:04:55Z

@mzabaluev will you have a look at these CI errors? I suspect they come from documentation examples that were added since you started this PR. If you don't have time, I can take a look today.

FrankReh · 2022-10-31T17:42:59Z

@mzabaluev I have fixed up the CI issues locally. Let me know if its okay with you that I push them up to this branch.

ollie-etl · 2022-10-31T20:36:44Z

All, I'll add I'm also looking at this, although my aims are a little broader. I think struct Slice + Pincan completely replace the existing IoBuf and IoMuttraits, giving both the very nice simplification shown in this PR, but also generalizing existing methods over any owned buffer which derefs to bytes. I will push as a PR for comment

mzabaluev · 2022-10-31T20:40:20Z

@mzabaluev I have fixed up the CI issues locally. Let me know if its okay with you that I push them up to this branch.

@FrankReh Go ahead. I suppose the newly added methods like write_all need to be converted to the new API.

FrankReh · 2022-10-31T21:22:30Z

@mzabaluev Would you mind reviewing the changes I made to the write_all example and write_all method for tcp, and perhaps the doc examples that use write_all too? Please check that the signatures look right, to be conforming with the others you had changed, that my logic for making progress in the loop inside write_all looks right to you and perhaps also that the progress method seems idiomatic, and finally, my use of in_begin and in_end to track what the original slice looked like coming in so it could be returned.

FrankReh · 2022-10-31T21:22:59Z

We expect the test to fail today for reasons not related to this commit.

mzabaluev · 2022-10-31T22:10:55Z

@FrankReh The write_all changes look fine, thank you.

FrankReh · 2022-10-31T23:10:12Z

@mzabaluev ~~Then this is ready. When master is ready for such a change, @Noah-Kennedy will likely merge.~~ Thank you. It's surprising the old Slice of Slice ... lived here for the last year. When @Noah-Kennedy has time, he should look at the methods that have become pub. I thought they should be pub(crate) but then I found some unit tests failing and decided to leave well-enough alone. Maybe Noah is already on board with that.

There's no need to expose raw buffer access methods of Slice in the public API.

mzabaluev · 2022-11-01T04:21:17Z

@FrankReh

I thought they should be pub(crate) but then I found some unit tests failing and decided to leave well-enough alone.

You are right, I've now fixed that.

FrankReh · 2022-11-01T15:54:31Z

Okay. Will wait on @Noah-Kennedy as he had something he wanted to get in first. This still looks good to me.

ollie-etl · 2022-11-02T11:42:50Z

src/buf/slice.rs

+pub trait IntoSlice: Sized {
+    /// The buffer type underlying the produced slices.
+    type Buf: IoBuf;
+
+    /// Returns a view of the buffer with the specified range.
+    /// The effective range bounds are computed against the potentially offset
+    /// buffer view that this method is called on, which may itself be a slice.
+    ///
+    /// This method is similar to Rust's slicing (`&buf[..]`), but takes
+    /// ownership of the buffer.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use tokio_uring::buf::IntoSlice;
+    ///
+    /// let buf = b"hello world".to_vec();
+    /// buf.slice(5..10);
+    /// ```
+    fn slice(self, range: impl ops::RangeBounds<usize>) -> Slice<Self::Buf>;
+
+    /// Converts the buffer into a [`Slice`] covering its full extent.
+    ///
+    /// This method is used internally by the `io-uring` operations and the
+    /// end user does not need to call it directly.
+    /// The implementation should be equivalent to `self.slice(..)`, but
+    /// can be optimized by skipping range checks.
+    fn into_full_slice(self) -> Slice<Self::Buf> {
+        self.slice(..)
+    }
+}


Could slice become a member function of impl Slice, and this trait be replaced with an instance of From<T: IoBuf> for Slice. I prefer that because in the absence of specialization, we can then repeatedly chain calls to slice, and they would then operate without creating nested slice instances.

let buf = vec![0u8;4096]; // Creates a slice over the full range let mut my_slice: Slice<_> = buf.into(); // Slice over 32 bytes offset at 16, and also demonstrate that post slice, the indeces are into the visible region of the slice my_slice.slice(16..48).slice(0..);

With this approach, we leave IoBuf alone, and the API functions still take T: IoBuf, On return, if a slice is needed by the API (like for read) .into() will produce a Slice, and if we started with Slice, it'll be a no op rather than Slice<Slice>

And also because slice is implments IoBuf, inputs to API calls don't need to be slice, but returned values on read-like function will be.

I did start implmenting this, but got a bit carried away replacing IoBuf with Pin. I think that is a more involved change

It's an interesting idea; the API methods always returning Slice<T> is a bit of a bump when you actually need T.

Would IoBuf lose its slice method then? If not, <Slice<T> as IoBuf>::slice remains a wart that could still get called inadvertently in some generic code.

I've started exploring this and came up with #163. I did not expect type inference to work as well as it does there.

<Slice<T> as IoBuf>::slice remains a wart

I think this can be fixed by adding an associated type to IoBuf. So I'm going to make yet another PR to explore that.

I think this can be fixed by adding an associated type to IoBuf.

I could not make that work. However, I favour #163 over this PR now.

mzabaluev · 2022-11-03T21:12:57Z

Superseded by #163.

Add infrastructure to manage pre-registered buffers, and operation methods `File::read_fixed_at` and `File::write_fixed_at` to make use of them. Exclusive access to buffer data between the application and in-flight ops is controlled at runtime. This is initial API to enable fixed buffers. Future developments may include: - ✅ Improved parameter polymorphism with ~#53~ #172; - An internal linked list of free buffers, to be able to check out the next available buffer from `FixedBufRegistry` without enumerating or keeping track of indices; - Support `IORING_REGISTER_BUFFERS2`/`IORING_REGISTER_BUFFERS_UPDATE`.

mzabaluev commented Sep 7, 2021

View reviewed changes

mzabaluev mentioned this pull request Sep 9, 2021

Fixed buffers to support ReadFixed and WriteFixed ops #54

Merged

mzabaluev force-pushed the into-slice-trait branch from 4717bf6 to 8b74f9e Compare March 21, 2022 03:16

mzabaluev added 6 commits May 3, 2022 19:08

chore: Remove buf/slice_mut.rs

f24b264

The module's code is no longer in use.

Add tests for .slice on Slice

61e022f

Update doc tests for *Listener

3bc46d7

The doc tests need update after the original IntoSlice refactoring.

Convert RecvFrom and SentTo to work with IntoSlice

c357648

mzabaluev force-pushed the into-slice-trait branch from 8b74f9e to c357648 Compare May 3, 2022 16:08

Run rustfmt

8efb9bd

Merge branch 'master' into into-slice-trait

55b7c1e

mzabaluev force-pushed the into-slice-trait branch from ed7c0b3 to 55b7c1e Compare October 31, 2022 10:36

FrankReh added 2 commits October 31, 2022 08:28

Merge branch 'master' into mzabaluev/into-slice-trait

40d034d

fix example and write_all and docs

c64f004

Merge branch 'master' into into-slice-trait

76ca1e0

Make the internal Slice methods crate-private

bb2f218

There's no need to expose raw buffer access methods of Slice in the public API.

ollie-etl reviewed Nov 2, 2022

View reviewed changes

mzabaluev mentioned this pull request Nov 3, 2022

Fix chained .slice to not produce nested Slice wrappers #163

Closed

mzabaluev marked this pull request as draft November 3, 2022 08:57

mzabaluev closed this Nov 3, 2022

mzabaluev mentioned this pull request Nov 18, 2022

BoundedBuf trait to take .slice API out of IoBuf #172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the ergonomics of .slice with IntoSlice trait #53

Improve the ergonomics of .slice with IntoSlice trait #53

mzabaluev commented Sep 7, 2021 •

edited

Loading

mzabaluev Sep 7, 2021 •

edited

Loading

mzabaluev Sep 10, 2021

mzabaluev Nov 1, 2022

Noah-Kennedy commented Mar 9, 2022

mzabaluev commented Mar 17, 2022

mzabaluev commented Mar 21, 2022

FrankReh commented Oct 26, 2022

FrankReh commented Oct 28, 2022

mzabaluev commented Oct 28, 2022

FrankReh commented Oct 31, 2022

FrankReh commented Oct 31, 2022

ollie-etl commented Oct 31, 2022

mzabaluev commented Oct 31, 2022

FrankReh commented Oct 31, 2022

FrankReh commented Oct 31, 2022

mzabaluev commented Oct 31, 2022

FrankReh commented Oct 31, 2022 •

edited

Loading

mzabaluev commented Nov 1, 2022

FrankReh commented Nov 1, 2022

ollie-etl Nov 2, 2022 •

edited

Loading

ollie-etl Nov 2, 2022

ollie-etl Nov 2, 2022

mzabaluev Nov 3, 2022 •

edited

Loading

mzabaluev Nov 3, 2022

mzabaluev Nov 3, 2022

mzabaluev commented Nov 3, 2022

Improve the ergonomics of .slice with IntoSlice trait #53

Improve the ergonomics of .slice with IntoSlice trait #53

Conversation

mzabaluev commented Sep 7, 2021 • edited Loading

mzabaluev Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

mzabaluev Sep 10, 2021

Choose a reason for hiding this comment

mzabaluev Nov 1, 2022

Choose a reason for hiding this comment

Noah-Kennedy commented Mar 9, 2022

mzabaluev commented Mar 17, 2022

mzabaluev commented Mar 21, 2022

FrankReh commented Oct 26, 2022

FrankReh commented Oct 28, 2022

mzabaluev commented Oct 28, 2022

FrankReh commented Oct 31, 2022

FrankReh commented Oct 31, 2022

ollie-etl commented Oct 31, 2022

mzabaluev commented Oct 31, 2022

FrankReh commented Oct 31, 2022

FrankReh commented Oct 31, 2022

mzabaluev commented Oct 31, 2022

FrankReh commented Oct 31, 2022 • edited Loading

mzabaluev commented Nov 1, 2022

FrankReh commented Nov 1, 2022

ollie-etl Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

ollie-etl Nov 2, 2022

Choose a reason for hiding this comment

ollie-etl Nov 2, 2022

Choose a reason for hiding this comment

mzabaluev Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

mzabaluev Nov 3, 2022

Choose a reason for hiding this comment

mzabaluev Nov 3, 2022

Choose a reason for hiding this comment

mzabaluev commented Nov 3, 2022

mzabaluev commented Sep 7, 2021 •

edited

Loading

mzabaluev Sep 7, 2021 •

edited

Loading

FrankReh commented Oct 31, 2022 •

edited

Loading

ollie-etl Nov 2, 2022 •

edited

Loading

mzabaluev Nov 3, 2022 •

edited

Loading