Defer work until polled #165

ollie-etl · 2022-11-04T22:10:30Z

This PR closes #120. It changes the semantics of all oeprations to do no work unless polled, which is generally the expected behavior of Futures.

As an aside, the API change gets rid of quite a few unwraps around Op creation, which are now mostly infallible, unless they have fallible steps during their creation

ollie-etl · 2022-11-04T22:23:43Z

~~This isn't ready, but I thought I'd solicit feedback early~~

ollie-etl · 2022-11-05T10:59:57Z

@Noah-Kennedy I have a question: What are the semantics for Ops polled outside of the runtime? Failure or panic?
In this test

tokio-uring/tests/driver.rs

Line 75 in 4b0e07e

drop(file);

we are to using io_uring resources out of context, and there appears to be significant special casing for this Op.

This PR keeps that special casing (Close is the only caller of try_submit_with). I actually think try_submit_with with should be try_immediate_submit_with in this case, to force the submission into a submitted state. This would be the only user of Submitted then also.

However, I'm unsure why we're going to such lengths to allow Ops to execute outside of a runtime?

ollie-etl · 2022-11-05T11:07:32Z

src/driver/op.rs

@@ -40,14 +42,18 @@ pub(crate) struct Op<T: 'static, CqeType = SingleCQE> {
 /// A Marker for Ops which expect only a single completion event
 pub(crate) struct SingleCQE;

+/// A Marker for Ops which may be submitted when the ring is not present
+/// The only user of this is `Close` currently
+pub(crate) struct Fallible;


I'm unconvinced this is needed. changing try_submit_with to try_submit_with_immediate or similar, which would submit the operation immediately would be sufficient

ollie-etl · 2022-11-05T21:49:11Z

Summary:

Ops no longer immediately submit. Instead, when a new op is created, the Sqe is calculated as before, and placed in a new Lifecycle variant, Lifecycle::Pending. If the op is then dropped, the Lifecycle is removed, but no transaction occurs on the ring. If instead the Op is Polled, the Lifecycle transitions to Lifecycle::Waiting, and the sqe placed on the submission ring. From this point onward, things proceed as before.

This has some nice properties.

Op construction is infallible, unless other IO is required in the creation.
Errors that occur when pushing the op to the queue can now be handled. They cause the op to complete with io::Error
Futures behave as advertised / expected.

The question re Close remains, although now I've dug into it, i have that question anyway. Its mostly orthogonal to this PR
I also need to do something with the unit tests on Op. Because Polling now does the submit, not creation, its much harder (impossible) to create a fake Op which doesn't submit as a test. Or rather I could, but it'd need its own future impl, which seems a bit pointless.

Noah-Kennedy · 2022-11-05T22:49:42Z

@Noah-Kennedy I have a question: What are the semantics for Ops polled outside of the runtime? Failure or panic? In this test

tokio-uring/tests/driver.rs

Line 75 in 4b0e07e

drop(file);

we are to using io_uring resources out of context, and there appears to be significant special casing for this Op.

This PR keeps that special casing (Close is the only caller of try_submit_with). I actually think try_submit_with with should be try_immediate_submit_with in this case, to force the submission into a submitted state. This would be the only user of Submitted then also.

However, I'm unsure why we're going to such lengths to allow Ops to execute outside of a runtime?

I actually think that this special casing of close needs to go. We should really be doing a synchronous close anyways.

That said, this is an issue that needs some discussion. Right now "panic" is fine, but if we add a Handle type, this would change, but we can cross that bridge if we get there.

ollie-etl · 2022-11-07T08:55:54Z

@Noah-Kennedy @FrankReh Ready for review

All ops are now deferred until Polled. The special case handling for Close shutdown is preserved (that is outside the scope of this PR), although submit_try_with has been renamed enqueue, and the signature changed to separate Op creation from submission.
Several Ops new signatures have changed from io::result<Op<T>> to Op<T>, as the Op creation is now infallible. If an op is encountered during ring submission, the error is propagated in the Future. See https://github.com/etlsystems/tokio-uring/blob/a04e4bcce198e883022af5e95b164f79299e6f90/src/driver/op.rs#L192
As a summary of performance, this causes either no change or maybe a slight performance improvement vs master

Benchmarks

Benchmarking no_op/1: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 22.1s, or reduce sample count to 20.
no_op/1                 time:   [215.09 ms 215.73 ms 216.45 ms]
                        thrpt:  [462.00 Kelem/s 463.55 Kelem/s 464.91 Kelem/s]
                 change:
                        time:   [-2.6494% -2.0570% -1.4711%] (p = 0.00 < 0.05)
                        thrpt:  [+1.4930% +2.1002% +2.7215%]
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  4 (4.00%) high mild
  3 (3.00%) high severe
no_op/32                time:   [31.378 ms 31.428 ms 31.481 ms]
                        thrpt:  [3.1765 Melem/s 3.1818 Melem/s 3.1870 Melem/s]
                 change:
                        time:   [-2.1162% -1.9144% -1.7006%] (p = 0.00 < 0.05)
                        thrpt:  [+1.7300% +1.9517% +2.1619%]
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  6 (6.00%) high mild
  1 (1.00%) high severe
no_op/64                time:   [28.056 ms 28.098 ms 28.146 ms]
                        thrpt:  [3.5529 Melem/s 3.5590 Melem/s 3.5643 Melem/s]
                 change:
                        time:   [-1.4561% -1.2270% -0.9898%] (p = 0.00 < 0.05)
                        thrpt:  [+0.9997% +1.2423% +1.4777%]
                        Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
  5 (5.00%) high mild
  3 (3.00%) high severe
no_op/256               time:   [25.777 ms 25.821 ms 25.868 ms]
                        thrpt:  [3.8658 Melem/s 3.8728 Melem/s 3.8795 Melem/s]
                 change:
                        time:   [-0.3218% -0.0816% +0.1423%] (p = 0.51 > 0.05)
                        thrpt:  [-0.1421% +0.0817% +0.3228%]
                        No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high mild

examples/cat.rs

examples/tcp_listener.rs

ollie-etl · 2022-11-07T09:23:42Z

src/driver/accept.rs

@@ -9,7 +9,7 @@ pub(crate) struct Accept {
 }

 impl Op<Accept> {
-    pub(crate) fn accept(fd: &SharedFd) -> io::Result<Op<Accept>> {


Op creation is infallible

ollie-etl · 2022-11-07T09:24:36Z

src/driver/close.rs


 pub(crate) struct Close {
    fd: RawFd,
 }

 impl Op<Close> {
+    /// Close a file descriptor


Comments unrelated to the PR, but helped me understand the whole Close shutdown saga whilst implementing

ollie-etl · 2022-11-07T09:24:54Z

src/driver/close.rs

    pub(crate) fn close(fd: RawFd) -> io::Result<Op<Close>> {
        use io_uring::{opcode, types};

-        Op::try_submit_with(Close { fd }, |close| {
+        let op = Op::submit_with(Close { fd }, |close| {


Create a Close Op: infallible

ollie-etl · 2022-11-07T09:25:08Z

src/driver/close.rs

-        })
+        });
+
+        op.enqueue()


immediately enqueue the Op: fallible

ollie-etl · 2022-11-07T09:25:25Z

src/driver/connect.rs

@@ -13,7 +13,7 @@ pub(crate) struct Connect {

 impl Op<Connect> {
    /// Submit a request to connect.
-    pub(crate) fn connect(fd: &SharedFd, socket_addr: SockAddr) -> io::Result<Op<Connect>> {
+    pub(crate) fn connect(fd: &SharedFd, socket_addr: SockAddr) -> Op<Connect> {


Op creation is infallible

ollie-etl · 2022-11-07T09:26:52Z

src/driver/mod.rs

+        for (_, cycle) in self.lifecycle.iter() {
+            match cycle {
+                Lifecycle::Completed(_) | Lifecycle::Pending(_) => {}
+                c => unreachable!("Lifecycle {:?} found during driver drop", c),
+            }
+        }


Stricvly unrelated, but gives better panic messages when something goes wrong during debug. can revert or PR elsewhere if preferred.

ollie-etl · 2022-11-07T09:27:19Z

src/driver/op.rs

@@ -46,7 +48,10 @@ pub(crate) trait Completable {
 }

 pub(crate) enum Lifecycle {
-    /// The operation has been submitted to uring and is currently in-flight
+    /// The operation has been created, and will be submitted when polled
+    Pending(Entry),


The meat of the PR

ollie-etl · 2022-11-07T09:27:45Z

src/driver/op.rs

+impl std::fmt::Debug for Lifecycle {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::Pending(_) => f.debug_tuple("Pending").finish(),
+            Self::Submitted => f.debug_tuple("Submitted").finish(),
+            Self::Waiting(_) => f.debug_tuple("Waiting").finish(),
+            Self::Ignored(_) => f.debug_tuple("Ignored").finish(),
+            Self::Completed(_) => f.debug_tuple("Completed").finish(),
+            Self::CompletionList(_) => f.debug_tuple("CompletionList").finish(),
+        }
+    }
+}


Debug only. Again, can revert or PR elsewhere if preferred.

ollie-etl · 2022-11-07T09:29:16Z

src/driver/op.rs

    /// the kernel.
-    pub(super) fn submit_with<F>(data: T, f: F) -> io::Result<Self>
+    pub(super) fn submit_with<F>(mut data: T, f: F) -> Self


Submission to the driver is infallible. This now places the seq for submission into the Pending state

ollie-etl · 2022-11-07T09:31:29Z

src/driver/op.rs

+    ///
+    /// This is useful when any failure in io_uring submission needs
+    /// to be handled at the point of submission.
+    pub(super) fn enqueue(self) -> io::Result<Self> {


Does nothing if called on an already submitted operation. TODO: improve docs

ollie-etl · 2022-11-07T09:32:15Z

src/driver/op.rs

                    Lifecycle::Submitted | Lifecycle::Waiting(_) => {
                        *lifecycle = Lifecycle::Ignored(Box::new(self.data.take()));
                    }
-                    Lifecycle::Completed(..) => {
+                    Lifecycle::Completed(..) | Lifecycle::Pending(_) => {


If the op is dropped before it is submitted to the ring, no work on the ring is scheduled.

ollie-etl · 2022-11-07T09:36:30Z

src/driver/shared_fd.rs

+        //
+        // TODO: Should we warn?
+        *state = match CONTEXT.try_with(|cx| cx.is_set()) {
+            Ok(true) => match Op::close(self.fd) {


As Close is special cased (preserving the current behavior to immediately submitting to the ring) , we can fallback to synchronous Close as required.

ollie-etl · 2022-11-07T09:51:19Z

src/driver/shared_fd.rs

                    *state = State::Closed;
-                    Poll::Ready(())
+                    Poll::Ready(r)


Propagates any error in Close from io_uring

FrankReh · 2022-11-07T15:57:41Z

src/driver/op.rs

-        } else {
-            Err(io::ErrorKind::Other.into())
-        }
+    /// Enqueue an operation on the submission ring, without `await`ing the future


Could end the sentence with a period, to be consistent with other changes to this file.

FrankReh

Not expressing an opinion on this one. @Noah-Kennedy 's is the only one that matters here as he's working on this issue too. I'm happy to read and think through this more if either of you think it's warranted however.

Noah-Kennedy · 2022-11-07T16:51:25Z

@ollie-etl see my comments in #120. I'm not sure that this is the right approach.

ollie-etl · 2022-11-08T12:23:22Z

@mzabaluev As you're pretty active in PR's currently, do you have a view on deferred operations?

mzabaluev · 2022-11-08T19:11:30Z

src/driver/op.rs

+                            // Try to push the new operation
+                            while unsafe { driver.uring.submission().push(&sqe).is_err() } {
+                                // Fail with an IoError if encountered
+                                driver.submit()?;


The lifecycle entry will stay in the Submitted state even if this fails?

Hmm, yes. Bug. It should be completed

mzabaluev · 2022-11-08T19:22:26Z

src/driver/socket.rs

@@ -40,12 +40,12 @@ impl Socket {
    }

    pub(crate) async fn write<T: IoBuf>(&self, buf: T) -> crate::BufResult<usize, T> {
-        let op = Op::write_at(&self.fd, buf, 0).unwrap();
+        let op = Op::write_at(&self.fd, buf, 0);


I believe the async transformation works so that this is not run until the future returned by write is polled for the first time. If Op creation only happens in such places, we have nothing to worry about?

I don't follow, sorry

Line 43 is not executed synchronously, that happens on the first poll of the future returned by write. So this method does not actually do any work until polled.
Is there any instance where submit_with is called inside a synchronous fn?

Noah-Kennedy · 2022-11-09T18:48:23Z

@oliverbunting as a result of the convo we had in the issue thread as well as discord, I think it might be better to move this to draft

Noah-Kennedy · 2022-11-09T18:49:07Z

Or potentially even close this since its actually part of a larger discussion we are having.

oliverbunting · 2022-11-09T19:37:16Z

In my experience, PRs marked draft get ignored, and Id like to invite feedback

ollie-etl added 5 commits November 4, 2022 21:16

Defer work until polled

3627571

Lints

3620360

fmt

8e579c9

Restore tests

79ffe3c

Handle Close on sharedFd

60c62f6

ollie-etl changed the title ~~Defer work~~ Defer work until polled Nov 4, 2022

ollie-etl added 2 commits November 4, 2022 22:22

Identify failing test: disable

f21b850

fmt

bdf2891

ollie-etl added 3 commits November 5, 2022 09:24

Remember we need to wake

499ec52

Improve debug

694e4be

Fix close outside of runtime

ae1c034

Driver tests pass

14f2049

ollie-etl commented Nov 5, 2022

View reviewed changes

ollie-etl mentioned this pull request Nov 5, 2022

possibly deferring work in the creation of an operation #120

Open

ollie-etl and others added 5 commits November 6, 2022 10:37

Simplify the fallback logic for shared_fs

a94375b

Merge branch 'master' into simplify-close

dbf82a0

Tidy

2126aaa

Merge remote-tracking branch 'origin/simplify-close' into simplify-close

65c6060

Merge branch 'simplify-close' into defer-work

a04e4bc

ollie-etl marked this pull request as ready for review November 7, 2022 08:50