Support multi completion events: v2 #130

ollie-etl · 2022-10-12T16:08:09Z

@Noah-Kennedy @FrankReh . I took on board your comments with the other PR and wanted to offer an alternative implementation to #128. This addresses some of the shortcomings of #128.

Multi CQE's are now held on a driver-scoped slab of completions. This goes some way to address the biggest shortcoming of the other approach, which was its propensity to allocate if multiple completion events were received for a single Op in quick succession. Obviously, allocation can still occur, but the total storage space is scoped to the runtime rather than per Op - so making it big initially benefits all Ops.
Op is now parameterized over classes of Ops, discriminated with a marker type. This allows us to easily add classes of Operations. All current ones are represented by SingleCQE. ~~I shall (haven't yet) rebase Support zero-copy send #123 to show its use.~~ now merged onto this. Will update that with performance figures for my use case.
@FrankReh I took on board your comments around Completed + enum variants vs adding another variant.
- Lifecycle::Completed is now guaranteed to not contain the MORE flag.
Lifecycle::CompletionList will never be observed for SingleCQE types.

This is used to parameterize Op<T, CqeType>. It is now possible to specialize blanket implmentations, such as Future, to classes of Ops

Adds infrastructure to store Completions a slab backed indexed linked list

Provides the infrastructure for recieving multiple CQE's for a single Op's lifecycle

src/driver/op.rs

FrankReh · 2022-10-19T13:36:35Z

I'm starting to look at your branch. ~~Is there a test or example that uses the new functionality? If not, is it as simple as substituting a zc operation for a send in one of the example programs?~~

This branch isn't about zero copy commands, its a precursor, I see now. Is there any way of testing this?

FrankReh · 2022-10-19T16:14:16Z

I like the linked list in a slab implementation. I'll note its a doubly linked list and I didn't see the benefit of the backward links but maybe that detail future proofs the list somehow.

Is there a branch that shows the CompletionList or the CompletionEntries being passed or consumed by a zero copy operation?

I had other comments but the Completion slab is the big addition and I like it. I wonder if @Noah-Kennedy could chime in on it?

I could try to change my stream fork to use the same mechanism, getting rid of the Vec of results.

oliverbunting · 2022-10-20T05:54:04Z

I like the linked list in a slab implementation. I'll note its a doubly linked list and I didn't see the benefit of the backward links but maybe that detail future proofs the list somehow.

So both pop and push are 1 lookup. Admittedly, holding the end index is sufficient for this, so the list itself doesn't need to be doubly linked

oliverbunting · 2022-10-20T06:30:41Z

Is there a branch that shows the CompletionList or the CompletionEntries being passed or consumed by a zero copy operation?

Yes, I've rebased the send_zc branch on to this

Noah-Kennedy

Sorry about the delay! I really like this approach. It needs a few finishing touches before it's good to go.

src/driver/op/completion.rs

ollie-etl · 2022-10-23T20:54:40Z

@Noah-Kennedy - I think I've addressed your concerns - let me know if you've more.

src/driver/mod.rs

FrankReh · 2022-10-25T20:20:54Z

@ollie-etl

@FrankReh thanks for the fairly rapid turnaround on #149. It makes this much cleaner

Sorry I had to step out for a few hours. I'm on US East Coast time.

ollie-etl · 2022-10-25T20:23:34Z

@FrankReh Not at all, I'd noticed we were in different time-zones. I'm also I'm genuinely grateful for the level of scrutiny these Pr's are receiving - these things are generally better got right or close to it first time around.

src/driver/op.rs

FrankReh

This is very nice. I think Noah has already said that too. I have a few small issues around comments.

src/driver/op/slab_list.rs

src/driver/op.rs

ollie-etl · 2022-10-25T21:20:31Z

Too bad the length of the slab linked list is so hard to track

It'd be very easy to add in SlabListIndices

src/driver/op.rs

FrankReh

Hope you are fresh when you read these. I think they are all comment related.

src/driver/op.rs

ollie-etl · 2022-10-27T11:51:58Z

@FrankReh All comments addressed

src/driver/op.rs

src/driver/op/slab_list.rs

FrankReh · 2022-10-27T13:00:29Z

I'm going to have this page dismiss Noah's review. The page seems broken somehow. When his review is expanded, it shows everything having been resolved. Maybe it just means he was expected to review and accept.

I think we can risk it. If more changes are needed, we'll make them with a new PR.

I believe Noah's issues were handled. I don't believe there is anything else that should block this merge.

FrankReh · 2022-10-27T13:02:39Z

Thanks for this really nice work and for your patience and timely turn arounds. I look forward to using these changes myself.

# 0.4.0 (November 5th, 2022) ### Fixed - Fix panic in Deref/DerefMut for Slice extending into uninitialized part of the buffer ([#52]) - docs: all-features = true ([#84]) - fix fs unit tests to avoid parallelism ([#121]) - Box the socket address to allow moving the Connect future ([#126]) - rt: Fix data race ([#146]) ### Added - Implement fs::File::readv_at()/writev_at() ([#87]) - fs: implement FromRawFd for File ([#89]) - Implement `AsRawFd` for `TcpStream` ([#94]) - net: add TcpListener.local_addr method ([#107]) - net: add TcpStream.write_all ([#111]) - driver: add Builder API as an option to start ([#113]) - Socket and TcpStream shutdown ([#124]) - fs: implement fs::File::from_std ([#131]) - net: implement FromRawFd for TcpStream ([#132]) - fs: implement OpenOptionsExt for OpenOptions ([#133]) - Add NoOp support ([#134]) - Add writev to TcpStream ([#136]) - sync TcpStream, UnixStream and UdpSocket functionality ([#141]) - Add benchmarks for no-op submission ([#144]) - Expose runtime structure ([#148]) ### Changed - driver: batch submit requests and add benchmark ([#78]) - Depend on io-uring version ^0.5.8 ([#153]) ### Internal Improvements - chore: fix clippy lints ([#99]) - io: refactor post-op logic in ops into Completable ([#116]) - Support multi completion events: v2 ([#130]) - simplify driver operation futures ([#139]) - rt: refactor runtime to avoid Rc\<RefCell\<...>> ([#142]) - Remove unused dev-dependencies ([#143]) - chore: types and fields explicitly named ([#149]) - Ignore errors from uring while cleaning up ([#154]) - rt: drop runtime before driver during shutdown ([#155]) - rt: refactor drop logic ([#157]) - rt: fix error when calling block_on twice ([#162]) ### CI changes - chore: update actions/checkout action to v3 ([#90]) - chore: add all-systems-go ci check ([#98]) - chore: add clippy to ci ([#100]) - ci: run cargo test --doc ([#135]) [#52]: #52 [#78]: #78 [#84]: #84 [#87]: #87 [#89]: #89 [#90]: #90 [#94]: #94 [#98]: #98 [#99]: #99 [#100]: #100 [#107]: #107 [#111]: #111 [#113]: #113 [#116]: #116 [#121]: #121 [#124]: #124 [#126]: #126 [#130]: #130 [#131]: #131 [#132]: #132 [#133]: #133 [#134]: #134 [#135]: #135 [#136]: #136 [#139]: #139 [#141]: #141 [#142]: #142 [#143]: #143 [#144]: #144 [#146]: #146 [#148]: #148 [#149]: #149 [#153]: #153 [#154]: #154 [#155]: #155 [#157]: #157 [#162]: #162

ollie-etl added 5 commits October 12, 2022 16:31

Add Marker SingleCqe

47452df

This is used to parameterize Op<T, CqeType>. It is now possible to specialize blanket implmentations, such as Future, to classes of Ops

Add CompletionList

829e7df

Adds infrastructure to store Completions a slab backed indexed linked list

Add CompletionList variant

cfe82f6

Provides the infrastructure for recieving multiple CQE's for a single Op's lifecycle

fmt

de50370

clippy

5d46bd9

ollie-etl commented Oct 12, 2022

View reviewed changes

src/driver/op.rs Outdated Show resolved Hide resolved

ollie-etl commented Oct 12, 2022

View reviewed changes

src/driver/op.rs Show resolved Hide resolved

ollie-etl commented Oct 12, 2022

View reviewed changes

src/driver/op.rs Outdated Show resolved Hide resolved

Add the overlooked Lifecycle::Ignored state logic

7d59c25

ollie-etl mentioned this pull request Oct 12, 2022

Support zero-copy send #123

Merged

Merge branch 'master' into multi-cqe-slab

b6986e5

Noah-Kennedy previously requested changes Oct 22, 2022

View reviewed changes

src/driver/op/completion.rs Outdated Show resolved Hide resolved

src/driver/op/completion.rs Outdated Show resolved Hide resolved

src/driver/op/completion.rs Outdated Show resolved Hide resolved

ollie-etl added 5 commits October 23, 2022 19:49

Newtype Cqe resuult tuple

b8046dc

Add comments and generalise

85d82cd

Make singly linked

6d058e0

Add some tests

3050e91

fmt

b700775

Merge branch 'master' into multi-cqe-slab

02191ef

ollie-etl mentioned this pull request Oct 23, 2022

Support multi completion events #128

Closed

Add missing cfg(test)

5b6f1aa

ollie-etl commented Oct 23, 2022

View reviewed changes

src/driver/mod.rs Outdated Show resolved Hide resolved

Fix over-zealous refactoring

af0502b

ollie-etl requested a review from Noah-Kennedy October 24, 2022 09:24

ollie-etl and others added 2 commits October 24, 2022 10:25

Merge branch 'master' into multi-cqe-slab

f4cbc99

Update no_op complete signature

7f0b323

ollie-etl commented Oct 25, 2022

View reviewed changes

src/driver/op.rs Outdated Show resolved Hide resolved

FrankReh requested changes Oct 25, 2022

View reviewed changes

src/driver/op/slab_list.rs Show resolved Hide resolved

src/driver/op/slab_list.rs Show resolved Hide resolved

src/driver/op.rs Outdated Show resolved Hide resolved

src/driver/op.rs Outdated Show resolved Hide resolved

src/driver/op.rs Outdated Show resolved Hide resolved

Address nits

de1630f

ollie-etl commented Oct 25, 2022

View reviewed changes

src/driver/op.rs Show resolved Hide resolved

FrankReh requested changes Oct 25, 2022

View reviewed changes

ollie-etl added 2 commits October 27, 2022 12:26

Merge remote-tracking branch 'upsteam/master' into multi-cqe-slab

6de3ec1

More docs

1d9c360

ollie-etl requested review from FrankReh and Noah-Kennedy and removed request for Noah-Kennedy and FrankReh October 27, 2022 12:15

FrankReh reviewed Oct 27, 2022

View reviewed changes

src/driver/op.rs Show resolved Hide resolved

FrankReh approved these changes Oct 27, 2022

View reviewed changes

FrankReh reviewed Oct 27, 2022

View reviewed changes

src/driver/op/slab_list.rs Show resolved Hide resolved

FrankReh merged commit ecaaa3e into tokio-rs:master Oct 27, 2022

Noah-Kennedy mentioned this pull request Nov 5, 2022

chore: prepare tokio-uring v0.4.0 #166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi completion events: v2 #130

Support multi completion events: v2 #130

ollie-etl commented Oct 12, 2022 •

edited

Loading

FrankReh commented Oct 19, 2022 •

edited

Loading

FrankReh commented Oct 19, 2022

oliverbunting commented Oct 20, 2022

oliverbunting commented Oct 20, 2022

Noah-Kennedy left a comment

ollie-etl commented Oct 23, 2022

FrankReh commented Oct 25, 2022

ollie-etl commented Oct 25, 2022

FrankReh left a comment

ollie-etl commented Oct 25, 2022

FrankReh left a comment

ollie-etl commented Oct 27, 2022

FrankReh commented Oct 27, 2022

FrankReh commented Oct 27, 2022

Support multi completion events: v2 #130

Support multi completion events: v2 #130

Conversation

ollie-etl commented Oct 12, 2022 • edited Loading

FrankReh commented Oct 19, 2022 • edited Loading

FrankReh commented Oct 19, 2022

oliverbunting commented Oct 20, 2022

oliverbunting commented Oct 20, 2022

Noah-Kennedy left a comment

Choose a reason for hiding this comment

ollie-etl commented Oct 23, 2022

FrankReh commented Oct 25, 2022

ollie-etl commented Oct 25, 2022

FrankReh left a comment

Choose a reason for hiding this comment

ollie-etl commented Oct 25, 2022

FrankReh left a comment

Choose a reason for hiding this comment

ollie-etl commented Oct 27, 2022

FrankReh commented Oct 27, 2022

FrankReh commented Oct 27, 2022

ollie-etl commented Oct 12, 2022 •

edited

Loading

FrankReh commented Oct 19, 2022 •

edited

Loading