-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: Switch to futures-0.3
channels
#6283
Conversation
Signed-off-by: ktf <krunotf@gmail.com>
I think this can be driven by benchmarks and profiling. I would not go ahead and spend the time to change everything everywhere without good evidence that performance improves meaningfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far this looks reasonable to me!
I agree, then we'll postpone that to a different issue where it will be easier to bench the difference. |
Signed-off-by: ktf <krunotf@gmail.com>
@@ -81,6 +81,7 @@ where | |||
Box::pin( | |||
input_rx | |||
.map(Message::Process) | |||
.fuse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes a bug where if no events are sent before shutting down this, then into_future
will poll this and get None
which it will pass with the rest of the stream which will be polled again later therefor breaking the stream contract of not calling it after first None
.
I was able to run the test harness against these changes and the latest nightly. Specifically for the real world performance test mentioned in #6043 it looks like this is an improvement:
However I do see some less favorable results for other configurations, for example:
My confidence in the test harness is lower than it has been though, for this magnitude of difference. I think #6274 will help here. Full test harness results: https://gist.github.com/jszwedko/9f4236899a0df1450d698f08d86a20bb I'll try running the criterion benches. |
I keep the criterion ones are already in CI 😄 For the topology benches I see:
Which does show some improvement, though not more than our noise threshold for those tests (20%). The fact that the improvement is consistent across all of them does give me some confidence though. Given that, I'm fine with this change going in. Hopefully #6274 will illuminate more as well as #6166. |
Signed-off-by: ktf <krunotf@gmail.com>
Indeed, in criterion it's more or less consistent, even all of 15 Looking at this results, my main worry is, that since |
I merged in master so what the new topology bench looks like. |
Now we are talking
The new benchmark shows consistent 10% improvement, so I'll run this one more time for good measure since it's a new bench and then we can go ahead with this. cc @jszwedko |
Nice! 🎉 |
Signed-off-by: ktf <krunotf@gmail.com>
Closes #6043
Replaces
tokio-0.2
channels introduced in #5868 withfutures-0.3
channels.Todo
test-harness
benches. Currently I'm failing to do soYour query returned no results
error vector-test-harness#67futures
channels when shutting downRace condition in futures::sync::mpsc rust-lang/futures-rs#909, fortunately there is PR that fixes it Port "fix race with dropping mpsc::Receiver" to 0.3 rust-lang/futures-rs#2304.timely_shutdown_lua_timer
is failing consistently. (EDIT: nope, it was just a regular bug)OpenQuestions
tokio
channels withfuture3
channels? (EDIT: Nope, better to leave that for a future issue)