Refactor main TUI loop to reduce latency in handling messages #187
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LucasPickering
force-pushed
the
loop-refactor
branch
2 times, most recently
from
4f05aef to
28bfc88
Compare
Previously, the main loop was rate-limited by the terminal input poll() call. This means when no user input was present, it would block for 250ms before handling any async messages. This could cause some annoying UI delays for changes triggered by background IO (e.g. HTTP responses could take up to 250ms *extra* to appear). By breaking the input loop into a separate task, and having the main loop block directly on the message queue, we ensure that every message is handled immediately and we update the UI immediately. I measured CPU usage on this, and it's the same as master. Locally I see ~5% usage whlie running in debug and ~0.7% while running with --release. This makes sense because the CPU is doing all the same work, just rearranged slightly. This has the added benefit that it accidentally fixed #136, which is great because I really didn't want to debug that anymore.
LucasPickering
force-pushed
the
loop-refactor
branch
from
28bfc88 to
a75ecff
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Describe the change. If there is an associated issue, please include the issue link (e.g. "Closes #xxx"). For UI changes, please also include screenshots.
Previously, the main loop was rate-limited by the terminal input poll() call. This means when no user input was present, it would block for 250ms before handling any async messages. This could cause some annoying UI delays for changes triggered by background IO (e.g. HTTP responses could take up to 250ms extra to appear). By breaking the input loop into a separate task, and having the main loop block directly on the message queue, we ensure that every message is handled immediately and we update the UI immediately.
I measured CPU usage on this, and it's the same as master. Locally I see ~5% usage whlie running in debug and ~0.7% while running with --release. This makes sense because the CPU is doing all the same work, just rearranged slightly.
This has the added benefit that it accidentally fixed #136, which is great because I really didn't want to debug that anymore.
Known Risks
What issues could potentially go wrong with this change? Is it a breaking change? What have you done to mitigate any potential risks?
Both high and low. This is the main loop so obviously if something is wrong it would be very bad, but that means it's very easy to check if it's working. Basic TUI functionality (accepting input, sending requests) works so I'm confident the loop is doing it's thing. I never fully understood the 100% CPU bug so there's a risk it still exists in some form, but I wasn't able to reproduce it anymore.
QA
How did you test this?
Run some requests in the TUI, measure CPU usage. To test the fixed bug, I ran
cargo runfrom inside azellijsession, then quit zellij as a whole (with ctrl-q) and watched the Slumber process die. The exact same steps previously would leave the Slumber process behind and it would spike to 100% CPU usage.Checklist
CHANGELOG.md?