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

Reduce busywaiting in ChannelStream components #197

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

mmirate
Copy link
Contributor

@mmirate mmirate commented Oct 27, 2023

When I want to read some bytes from the server, but I don't expect output to be available to be read immediately, I observe that even when the resulting system is "idle", the read call consumes an entire CPU core!

The cause is fairly straightforward: calling Waker::wake_by_ref() in poll and similar methods, in circumstances where the method didn't make any progress before activating the Waker, means a busywait. When there is an underlying asynchronous operation, proper behavior is obtained by passing the Waker to the underlying operation's poll method.

In this case of the ChannelRx, I propose a very simple solution, made possible by https://docs.rs/tokio/latest/tokio/sync/mpsc/struct.UnboundedReceiver.html#method.poll_recv. Using this, I observe that I am able to tail -f an infrequently-written logfile with no CPU usage unless logfile contents are being transferred and processed.

grep told me that ChannelTx has a similar problem; because its body is less simple than "await a future then do synchronous things with the result", my proposed solution is markedly less simple. It is also not 100% complete; it will still busywait for the window size to be increased from zero; the obvious answer to that would be a condvar, except that doing these 100% correctly in async Rust appears to be an unsolved problem and thus there isn't (yet) a condvar in tokio::sync.

I've placed these in separate commits so that the ChannelTx change may be set aside if it is undesirable.

Completely eliminating the busywaiting will require using a different or
additional synchronization primitive for the window size.
`tokio::sync::Notify` will not (yet) suffice because it has no "_owned"
variant of its `Notified` future.
@Eugeny
Copy link
Member

Eugeny commented Nov 1, 2023

Thank you! Sorry for the delay reviewing, I needed to really wrap my head around this. While I agree that the Tx implementation is not the simplest, it's still a significant improvement, so I'm merging both 🙌

@Eugeny Eugeny merged commit 84072f3 into warp-tech:main Nov 1, 2023
3 checks passed
@mmirate mmirate deleted the mmirate/performance branch November 1, 2023 14:12
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.

None yet

2 participants