Skip to content

Conversation

@mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Nov 7, 2022

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.slice(range) -> Slice (i.e. slicing an IoBuf BoundedBuf produces an IoBuf Slice)
  • A Slice is also a BoundedBuf by definition
  • And to void the problem of a Slice<Slice<>> hierarchy building up
    • A BoundedBuf<Slice>.slice(range) -> Slice (keeping the slice flat)

All calls that take a BoundedBuf input type can take an IoBuf and a Slice.

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 Fix chained .slice to not produce nested Slice wrappers #163;
  • Reduce monomorphization bloat, so that most of the machine code to work with buffers passed in as, say, Vec and Slice<Vec> is emitted in one instance, rather than replicated instances of exactly the same code.

It was an unused file laying about.
Introduce new traits, BoundedBuf and BoundedBufMut, taking the
slice method and related infrastructure off IoBuf and IoBufMut.
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.
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.
@mzabaluev mzabaluev marked this pull request as ready for review November 7, 2022 23:42
Will need access to the buffer object when implementing, for example,
ReadFixed/WriteFixed ops.
@Noah-Kennedy
Copy link
Contributor

I need to take some time this weekend to go through this.

@FrankReh
Copy link
Collaborator

@mzabaluev How would you describe the trait relationships that make this work. Such a description could be good to put into the top description of this feature. I'm trying to boil this down into a short description of how this works, why two layers of trait helps, and how it can still be easy for the user to understand and appreciate. The three lines of code required in File.read_exact_at to wrap File.read_exact_slice_at cause me to look for a simple paradigm to have in mind when implementing a feature at this level.

Is it

  • 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)

So all calls that take a BoundedBuf input type can take an IoBuf and a Slice<IoBuf>?

Maybe a std::convert::From impl would even help.

I'm not going to try to boil down the Mut versions yet.

@FrankReh
Copy link
Collaborator

FrankReh commented Nov 17, 2022

Edit: Anyone reading this later, should be aware it was fixed and discarded below.

Note: this isn't critical for the PR, but an Into would make some parameter passing cleaner.

I tried to impl the Into trait for a BoundedBufMut into a Slice but I
get this compile error. I haven't figured out how to resolve. It's supposed to mean one of the types I'm using isn't from the local trait, but except for the Into trait, the traits are from the tokio-uring crate.

use std::convert::Into;

impl<T, B> Into<Slice<T>> for B
where
    T: IoBufMut,
    B: BoundedBufMut<BufMut = T>,
 
{
    fn into(self) -> Slice<T> {
        self.slice(..)
    }
}

compiles with error:

  Compiling tokio-uring v0.4.0 (/home/frank/forks/tokio-uring)
error[E0210]: type parameter `B` must be covered by another type when it appears before the first local type (`Slice<T>`)
   --> src/buf/bounded.rs:160:9
    |
160 | impl<T, B> Into<Slice<T>> for B
    |         ^ type parameter `B` must be covered by another type when it appears before the first local type (`Slice<T>`)
    |
    = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters appear before that first local type
    = note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last

For more information about this error, try `rustc --explain E0210`.
error: could not compile `tokio-uring` due to previous error

@mzabaluev
Copy link
Contributor Author

@FrankReh

I'm trying to boil this down into a short description of how this works, why two layers of trait helps, and how it can still be easy for the user to understand and appreciate. [...]

Is it

The bullet points are correct, as is this:

So all calls that take a BoundedBuf input type can take an IoBuf and a Slice<IoBuf>?

✔️

The three lines of code required in File.read_exact_at to wrap File.read_exact_slice_at cause me to look for a simple paradigm to have in mind when implementing a feature at this level.

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 Fix chained .slice to not produce nested Slice wrappers #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.

Maybe a std::convert::From impl would even help.

I don't see a particular need for it, the I/O methods taking buffer arguments are as polymorphic as they were prior to this change.

@mzabaluev
Copy link
Contributor Author

impl<T, B> Into<Slice<T>> for B
where
    T: IoBufMut,
    B: BoundedBufMut<BufMut = T>,

You only really need an IoBuf bound on T.
The signature of that implementation is too loose for the orphan rule.
And you should always try to implement From rather than Into, as the latter then works via its blanket impl.
Try this:

impl<T: IoBuf> From<T> for Slice<T> {
    // ...
}

@FrankReh
Copy link
Collaborator

@mzabaluev Congrats on fixing my attempt at an Into, or a From also. Even with it implemented, the code in read_exact_at couldn't be changed without making it specific to the IoBufMut case it seemed. (Of course I could have done something else wrong too.) I thought the case where a BoundedBufMut was already a Slice would be covered because I had read we get the From and Into for free for types that are already the type of the parameter.

No matter. I agree there is no call to create the From implementation because the wrapper function serves an important function of reducing the monomorphism bloat.

Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most are just observations, as I become enlightened. This is cool.

But one question about how buf.slice(..) is used in the wrapper functions and one about where the right place to call into_inner() is.

It's not necessary for the trait definition, and the implementations
of BoundedBuf are sealed in this crate.
This is an optimization of .slice(..) for internal use,
skipping unnecessary consistency checks.
Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LVGTM. Thanks!

@FrankReh
Copy link
Collaborator

@Noah-Kennedy Can you take another look?

Copy link
Collaborator

@FrankReh FrankReh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What an achievement. Let's see how long this slice implementation lasts. It has certainly been contentious and remains so in some discussions with rust long timers. Big thanks!

@FrankReh FrankReh merged commit b1ca3aa into tokio-rs:master Nov 21, 2022
FrankReh pushed a commit that referenced this pull request Nov 23, 2022
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants