Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Builder framework #171

Closed
wants to merge 3 commits into from
Closed

Conversation

ollie-etl
Copy link
Contributor

@ollie-etl ollie-etl commented Nov 7, 2022

This PR Introduces

pub(crate) trait Buildable
where
    Self: 'static + Sized + Completable,
{
    // The CqeType type which results from Submission
    type CqeType;

    // The closure from submit_with, made a function
    fn create_sqe(&mut self) -> squeue::Entry;

    /// Build an Operation, and submit to uring.
    ///
    /// `state` is stored during the operation tracking any state submitted to
    /// the kernel.
    fn submit(mut self) -> io::Result<Op<Self, <Self as Buildable>::CqeType>> {
        todo!("This was previously `submit_with` in impl Op")
    }
}

And provides an implementation for all Ops (there are a lot aren't there?).
Where an Op is Op<OP_HERE,_>. This makes a lot of sense, as the structs already contained all (or most) of the data required to create the Op itself.

What this PR doesn't do is expose them, it just puts the framework in place. Tests still pass, and Benchmarks improve fractionally, seemingly due to better L1 cache hits. Haven't looked into, and not that interested in why.

Future PR's can go several ways from here.

  • Ops can have Builder APi's wrapped around them one at a time, and things will just work
  • Ops can be linked. If we view a link as consuming a builder, which I think is correct, because you must ensure its not yet been submitted, which the builder does. I don't plan on doing linking in this PR, although might show it bolted on top. I left some commented code to show where it would go, which provides it to all ops.

For the sake of clarity, this doesn't introduce Deferred Ops, which is a seperate concern. It doesn't block it either.

d as draft because I expect significant discussion

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.

1 participant