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

make tokio dependency optional #160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FlorianUekermann
Copy link
Contributor

@FlorianUekermann FlorianUekermann commented Feb 5, 2023

The h3 crate depends on tokio, but only uses it for the unbounded channel, which signals request completion. I would like to replace the use of tokio::sync::mpsc with a lighter alternative, or at least make it optional.

This PR adds an optional dependency on async-channel and makes tokio optional. Alternatively any other mpsc implementation or even a Arc<Mutex<(VecDeque, Waker)>> should be sufficient.

@seanmonstar
Copy link
Member

Thanks for sending us the PR!

Does the channel you suggested actually "weigh" less? Currently, h3 only depends on the sync feature of Tokio, so it's not compiling the runtime part. I would assume they are similar amounts of code, and so then would tend to lean toward depending on Tokio's because it's usage by other is extensive.

@FlorianUekermann
Copy link
Contributor Author

FlorianUekermann commented Feb 6, 2023

Does the channel you suggested actually "weigh" less?

Depends by what metric. In terms of code being downloaded from crate.io it's roughly 50 times lighter. In terms of performance, and compiled size I have no idea (but I can check...).

A motivation was to make it obvious that the crate is runtime independent and doesn't spawn tasks. With tokio as a dependency that is tricky to tell for most people, since some things in tokio require running inside a tokio runtime, while others don't. It's not the case here, but I almost decided not to use h3 because I didn't look close enough at first.

Currently, h3 only depends on the sync feature of Tokio, so it's not compiling the runtime part. I would assume they are similar amounts of code, and so then would tend to lean toward depending on Tokio's because it's usage by other is extensive.

Same with async-channel. That's the one async-std and smol use. That was my reasoning behind making it optional.

Maybe I should have used Arc<Mutex<(VecDeque, Option<Waker>)>>, I just wanted to send something that works to start the conversation.

@FlorianUekermann
Copy link
Contributor Author

FlorianUekermann commented Feb 6, 2023

I did a quick benchmark that seemed reasonable for this case and tokio was about 70% slower than futures-channel. It's also a much smaller download and as frequently used (according to crates.io) as tokio. Compiled sizes didn't differ.

I would propose to change the PR and get rid of both tokio and async-channel in favor of futures-channel with no feature flags (or make it default and hide the other two behind features).

use futures_util::StreamExt;

fn main() {
    let w = noop_waker::noop_waker();
    let mut cx = std::task::Context::from_waker(&w);
    let cx = &mut cx;

    #[cfg(feature = "async-channel")]
    let (s, mut r) = async_channel::unbounded();
    #[cfg(feature = "tokio")]
    let (s, mut r) = tokio::sync::mpsc::unbounded_channel();
    #[cfg(feature = "futures-channel")]
    let (s, mut r) = futures_channel::mpsc::unbounded();

    for i in 0..10_000_000u64 {
        #[cfg(feature = "async-channel")]
        let x = {
            let _ = r.poll_next_unpin(cx);
            s.try_send(i).unwrap();
            r.poll_next_unpin(cx)
        };
        #[cfg(feature = "tokio")]
        let x = {
            let _ = r.poll_recv(cx);
            s.send(i).unwrap();
            r.poll_recv(cx)
        };
        #[cfg(feature = "futures-channel")]
        let x = {
            let _ = r.poll_next_unpin(cx);
            s.unbounded_send(i).unwrap();
            r.poll_next_unpin(cx)
        };
        std::hint::black_box(x);
    }
}

@seanmonstar
Copy link
Member

Performance characteristics for channels really depends on usage. They all chose different pros and cons, such as using a linked list versus arrays and blocks, instrusive collections, locks vs spinning, preferring allocating in send or recv or reserve, etc. Depending on how contended they'll be, how big the message type is, how often an item is sent, etc.

For that reason, I tend to find Tokio's channel preferable because it sees more usage, and there are several active maintainers available to fix any bugs that are reported.

@Ruben2424
Copy link
Contributor

A motivation was to make it obvious that the crate is runtime independent and doesn't spawn tasks. With tokio as a dependency that is tricky to tell for most people, since some things in tokio require running inside a tokio runtime, while others don't. It's not the case here, but I almost decided not to use h3 because I didn't look close enough at first.

Independent of the channel, I think it is a good idea to address in the readme that h3 is runtime independent. I will submit a PR for this.

@FlorianUekermann
Copy link
Contributor Author

Independent of the channel, I think it is a good idea to address in the readme that h3 is runtime independent. I will submit a PR for this.

Thx, that helps.

Performance characteristics for channels really depends on usage.

That's why the benchmark looks the way it does. I suspect the channel signalling request ends will pretty much always be empty and at least half off the polls result in Poll::Pending.

I tend to find Tokio's channel preferable because it sees more usage, and there are several active maintainers available to fix any bugs that are reported.

I would have assumed that anything in futures-rs project is quite actively maintained as well and the recent download numbers on crates.io are practically identical. But as a contributor to both of them, you are obviously more qualified to comment on that.

To be clear, I only sent this PR because of the size and the "looks like depends on tokio runtime" concern, not performance. I just wanted to alleviate potential performance concerns with the benchmark. I personally would still prefer the smaller dependency, but it is pretty subjective, so feel free to close. I'm happy to send a PR for any variant mentioned in the thread otherwise.

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