-
Notifications
You must be signed in to change notification settings - Fork 83
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for sending us the PR! Does the channel you suggested actually "weigh" less? Currently, h3 only depends on the |
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.
Same with Maybe I should have used |
I did a quick benchmark that seemed reasonable for this case and I would propose to change the PR and get rid of both 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);
}
} |
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 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. |
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.
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
I would have assumed that anything in 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. |
1464442
to
1f10a0b
Compare
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 oftokio::sync::mpsc
with a lighter alternative, or at least make it optional.This PR adds an optional dependency on
async-channel
and makestokio
optional. Alternatively any other mpsc implementation or even aArc<Mutex<(VecDeque, Waker)>>
should be sufficient.