async-std support with trait objects #1346

yu-re-ka · 2022-04-17T13:27:09Z

Alternative to #1345

yu-re-ka · 2022-04-18T07:31:43Z

Surprisingly using the async-std/async-io based UdpSocket and Timer code (still running under the tokio runtime otherwise) leads to better performance in the bulk bench, even though it spawns a background thread.

bench results

$ cargo run --bin bulk --release 

Client 0 stats:
Overall download stats:

Transferred 1073741824 bytes on 1 streams in 1.86s (549.73 MiB/s)

Stream download metrics:

      │  Throughput   │ Duration 
──────┼───────────────┼──────────
 AVG  │  549.75 MiB/s │     1.86s
 P0   │  549.50 MiB/s │     1.86s
 P10  │  550.00 MiB/s │     1.86s
 P50  │  550.00 MiB/s │     1.86s
 P90  │  550.00 MiB/s │     1.86s
 P100 │  550.00 MiB/s │     1.86s

$ sed "s/TokioRuntime/AsyncStdRuntime/g" -i bench/src/lib.rs
$ cargo run --bin bulk --release --features runtime-async-std
[...]

Client 0 stats:
Overall download stats:

Transferred 1073741824 bytes on 1 streams in 1.70s (602.03 MiB/s)

Stream download metrics:

      │  Throughput   │ Duration 
──────┼───────────────┼──────────
 AVG  │  602.25 MiB/s │     1.70s
 P0   │  602.00 MiB/s │     1.70s
 P10  │  602.50 MiB/s │     1.70s
 P50  │  602.50 MiB/s │     1.70s
 P90  │  602.50 MiB/s │     1.70s
 P100 │  602.50 MiB/s │     1.70s

$

We could also try to choose the correct runtime for the user:

pub fn runtime() -> Box<dyn Runtime> {
    #[cfg(feature = "runtime-tokio")]
    {
        if let Ok(_) = tokio::runtime::Handle::try_current() {
            return TokioRuntime;
        }
    }

    #[cfg(feature = "runtime-async-std")]
    {
        return AsyncStdRuntime;
    }

    panic!("No usable runtime found");
}

But I don't know, should the user still be able to override this?

Ralith · 2022-04-25T00:12:06Z

Surprisingly using the async-std/async-io based UdpSocket and Timer code (still running under the tokio runtime otherwise) leads to better performance in the bulk bench, even though it spawns a background thread.

Interesting! How consistent is that across runs?

We could also try to choose the correct runtime for the user:

I actually really like the look of that helper, though I'm not sure it should be exposed directly. Let's use that to avoid complicating the Endpoint::client and Endpoint::server helpers (the entire purpose of those is to be simple), but take an explicit argument in Endpoint::new for when explicit selection is required.

Ralith

Thanks for your continued work on this! I'm liking how it's shaping up.

I do wonder if we could refactor quinn-udp to be plain stupid-simple helper functions on RawFd/RawHandle and isolate all the runtime abstraction into the quinn crate. I don't think this is a hard blocker for me, though.

Ralith · 2022-04-25T00:15:49Z

quinn/examples/client-async-std.rs

@@ -0,0 +1,161 @@
+//! This example demonstrates an HTTP client that requests files from a server.


I'm not sure the fairly trivial differences justify duplicating the maintenance cost of completely duplicating the client/server examples. Maybe a single dedicated, simpler example that just establishes a connection?

I also thought about that. I needed a way to test if everything worked correctly but you're right, a simpler example would also work for that.

yu-re-ka · 2022-04-27T16:50:12Z

Interesting! How consistent is that across runs?

After trying a few more things I think it comes down to the amount of threads the load is distributed across. When using the single-threaded tokio executor (like the bench is), it is bottle-necked on that single thread, and off-loading thread to another thread takes load off the executor thread.
When using a multi-threaded runtime, a few threads (4-6 threads on my machine) do increase performance because it allows the load to be handled by multiple cores, but increasing the thread count even further will result in slower performance because of lots of moving tasks around.

Let's use that to avoid complicating the Endpoint::client and Endpoint::server helpers (the entire purpose of those is to be simple), but take an explicit argument in Endpoint::new for when explicit selection is required.

Sounds good! I will implement that later.

yu-re-ka · 2022-04-27T16:50:32Z

(Close was accidental. Pressed the wrong button)

Ralith · 2022-04-27T17:11:53Z

After trying a few more things I think it comes down to the amount of threads the load is distributed across.

Interesting indeed; I wouldn't have guessed that there was any opportunity for parallelism there. Good investigation!

parazyd · 2022-05-04T10:02:41Z

@yu-re-ka This still seems to have a hard dependency on the tokio crate. Is that intended?

yu-re-ka · 2022-05-04T10:11:57Z

Yes, that was the suggestion from the maintainers:

I also don't think tokio's sync primitives are runtime-specific, so those can be left in on the first pass to simplify review.

I tested that it only builds the sync feature of the tokio crate, so you don't get a dependency on any of the runtime/executor code.

yu-re-ka · 2022-05-04T10:38:32Z

I have removed the runtime parameter from Endpoint::client, Endpoint::server (these now use the default mechanism described earlier), and Endpoint::connect, Endpoint::connect_with (these use the runtime the endpoint was created with).

parazyd · 2022-05-04T11:46:30Z

Brilliant work

Ralith · 2022-05-05T20:12:27Z

quinn-udp/src/unix.rs

+            ready!(self.io.poll_write_ready(cx))?;
+            match send(state, self.io.get_ref(), last_send_error, transmits) {
+                Err(e) if e.kind() == io::ErrorKind::WouldBlock => {
+                    self.io.clear_write_ready(cx);


What if the socket becomes ready in between the call to poll_write_ready and clear_write_ready? Could this miss a wakeup? Similarly for reads.

First, the clear_write_ready is a no-op on all implementations but tokio+unix (using AsyncFd), so we are only talking about this platform now. async-io doesn't require (or have a way of) clearing readiness events. tokio UdpSocket automatically clears readiness events for us in the call to try_recv_from, and it also has no way to manually do that.

The code here is following the instructions from the tokio documentation:

Indicates to tokio that the file descriptor is no longer ready. The internal readiness flag will be cleared, and tokio will wait for the next edge-triggered readiness notification from the OS.
It is critical that this function not be called unless your code actually observes that the file descriptor is not ready. Do not call it simply because, for example, a read succeeded; it should be called when a read is observed to block.

When we observe that the file descriptor is not ready (I/O operation returns WouldBlock), we clear the readiness status.
I don't know what would be the alternative? This is exactly how tokio documentation describes it and how tokio does it itself internally.
I can't really say that the concern you are raising is not justified, but if it applies here it would also apply to the previous code using the tokio ReadyGuard.

Also note that as long as we don't clear the readiness event, polling again will return ready right away, as that's the whole point of clearing readiness events: The readiness state is stored and returned unchanged until it is cleared. The code for that is here:
https://docs.rs/tokio/latest/src/tokio/io/driver/scheduled_io.rs.html#284
So it should not be a problem that we poll again before clearing the readiness event, which is the only difference between this code and the previous.

Thanks; I'll have to study this in some detail...

Your concern is indeed very justified:
microsoft/msquic#2654

Discussed with upstream tokio; this will miss wakeups. In particular, the implementation of clear_write_ready in terms of poll_write_ready will cause readiness events that occur after the initial poll_write_ready to be lost.

Our basic problem is that AsyncUdpSocket is trying to be too low-level. Rather than trying to make the methods of quinn_udp::UdpSocket abstract over the underlying async primitive, let's try converting the (already object-safe) quinn_udp::UdpSocket itself into a trait object, with separate implementations for tokio and async-std. Those separate implementations should then call into shared code to do the OS-specific heavy-lifting. We can then hoist the trait machinery up into the quinn crate and leave the shared OS-specific heavy lifting code in quinn_udp, concentrating awareness of async runtimes in one place.

I've implemented the approach I outlined above in #1364.

yu-re-ka · 2022-05-06T10:51:25Z

Rebased, resolved conflicts

djc

This is looking pretty good, thanks for working on it!

Some comments, mostly stylistic concerns -- hope that's okay.

quinn-udp/Cargo.toml

quinn-udp/src/runtime/async_std_runtime.rs

quinn-udp/src/runtime/mod.rs

quinn/src/runtime/async_std_runtime.rs

quinn/src/runtime/tokio_runtime.rs

quinn/src/endpoint.rs

quinn/src/runtime/mod.rs

djc · 2022-05-09T10:03:04Z

quinn/src/runtime/async_std_runtime.rs

@@ -0,0 +1,26 @@
+use super::{AsyncTimer, Runtime};


Style nit: please split the imports as mentioned elsewhere.

I can't infer from the other comments how you would like imports to be structured. Is there some styling guidelines where I can read up on this?

Our convention is to split imports into three blocks: std, followed by external crates, followed by the local crate. We don't currently have a formal style guide written, though we should; thanks for bearing with us in the mean time.

yu-re-ka · 2022-05-09T18:43:50Z

I can see from your comments that you would like things to be changed somehow, but there are so many options for how I could interpret these comments. I now just chose any one option, so you will have to make new comments if my interpretations did not match your comment intention - or send me a link to some guideline I can follow.

dignifiedquire · 2022-06-01T10:30:16Z

why was this closed?

yu-re-ka · 2022-06-01T13:30:11Z

I don't have motivation to refactor this again. Anyone feel free to open a new PR and use these changes if it is helpful.

Ralith · 2022-06-03T03:59:09Z

This was very close to completion, so I took it the last few steps in #1364.

yu-re-ka mentioned this pull request Apr 17, 2022

async-std support, second try #1345

Closed

3 tasks

yu-re-ka force-pushed the feature/async-std3 branch 13 times, most recently from b08772e to f8fbd8e Compare April 17, 2022 21:49

Ralith reviewed Apr 25, 2022

View reviewed changes

yu-re-ka closed this Apr 27, 2022

yu-re-ka reopened this Apr 27, 2022

yu-re-ka force-pushed the feature/async-std3 branch 2 times, most recently from 7d61ab1 to 229f3de Compare May 4, 2022 10:37

yu-re-ka force-pushed the feature/async-std3 branch 3 times, most recently from ca3307f to c12d09c Compare May 4, 2022 10:42

yu-re-ka force-pushed the feature/async-std3 branch from c12d09c to 76e77de Compare May 4, 2022 10:46

yu-re-ka requested a review from Ralith May 4, 2022 12:21

Ralith reviewed May 5, 2022

View reviewed changes

async-std support

a9a28fa

yu-re-ka force-pushed the feature/async-std3 branch from 20f3bb2 to a9a28fa Compare May 6, 2022 09:58

elenaf9 mentioned this pull request May 6, 2022

transports/quic: Add implementation based on quinn-proto libp2p/rust-libp2p#2289

Merged

7 tasks

djc reviewed May 9, 2022

View reviewed changes

apply requested style changes

698290a

yu-re-ka force-pushed the feature/async-std3 branch from 633f011 to 698290a Compare May 9, 2022 18:41

This was referenced May 22, 2022

New release #1352

Closed

Prepare 0.8.3 release #1358

Merged

yu-re-ka closed this May 31, 2022

Ralith mentioned this pull request Jun 3, 2022

Abstract runtime support #1364

Merged

yu-re-ka deleted the feature/async-std3 branch August 29, 2022 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async-std support with trait objects #1346

async-std support with trait objects #1346

yu-re-ka commented Apr 17, 2022 •

edited

Loading

yu-re-ka commented Apr 18, 2022 •

edited

Loading

Ralith commented Apr 25, 2022

Ralith left a comment

Ralith Apr 25, 2022

yu-re-ka Apr 27, 2022

yu-re-ka commented Apr 27, 2022

yu-re-ka commented Apr 27, 2022

Ralith commented Apr 27, 2022

parazyd commented May 4, 2022

yu-re-ka commented May 4, 2022 •

edited

Loading

yu-re-ka commented May 4, 2022

parazyd commented May 4, 2022

Ralith May 5, 2022

yu-re-ka May 5, 2022 •

edited

Loading

Ralith May 6, 2022

yu-re-ka May 7, 2022

Ralith May 22, 2022

Ralith Jun 3, 2022

yu-re-ka commented May 6, 2022

djc left a comment

djc May 9, 2022

yu-re-ka May 9, 2022

Ralith May 22, 2022

yu-re-ka commented May 9, 2022

dignifiedquire commented Jun 1, 2022

yu-re-ka commented Jun 1, 2022

Ralith commented Jun 3, 2022

		@@ -0,0 +1,161 @@
		//! This example demonstrates an HTTP client that requests files from a server.

async-std support with trait objects #1346

async-std support with trait objects #1346

Conversation

yu-re-ka commented Apr 17, 2022 • edited Loading

yu-re-ka commented Apr 18, 2022 • edited Loading

Ralith commented Apr 25, 2022

Ralith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yu-re-ka commented Apr 27, 2022

yu-re-ka commented Apr 27, 2022

Ralith commented Apr 27, 2022

parazyd commented May 4, 2022

yu-re-ka commented May 4, 2022 • edited Loading

yu-re-ka commented May 4, 2022

parazyd commented May 4, 2022

Choose a reason for hiding this comment

yu-re-ka May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yu-re-ka commented May 6, 2022

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yu-re-ka commented May 9, 2022

dignifiedquire commented Jun 1, 2022

yu-re-ka commented Jun 1, 2022

Ralith commented Jun 3, 2022

yu-re-ka commented Apr 17, 2022 •

edited

Loading

yu-re-ka commented Apr 18, 2022 •

edited

Loading

yu-re-ka commented May 4, 2022 •

edited

Loading

yu-re-ka May 5, 2022 •

edited

Loading