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

Avoid consuming CPU when waiting for input. #651

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

sunfishcode
Copy link
Contributor

When reading input, start by calling event::read, which efficiently blocks until input arrives, rather than polling for input every 100 milliseconds.

This preserves the existing POLL_WAIT polling behavior by moving the poll after the first read call returns.

Fixes #521.

When reading input, start by calling `event::read`, which efficiently blocks
until input arrives, rather than polling for input every 100
milliseconds.

This preserves the existing `POLL_WAIT` polling behavior by moving the
poll after the first `read` call returns.

Fixes nushell#521.
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #651 (0745f64) into main (973dbb5) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #651   +/-   ##
=======================================
  Coverage   49.17%   49.17%           
=======================================
  Files          42       42           
  Lines        7921     7920    -1     
=======================================
  Hits         3895     3895           
+ Misses       4026     4025    -1     
Files Coverage Δ
src/engine.rs 5.14% <0.00%> (+<0.01%) ⬆️

@fdncred
Copy link
Collaborator

fdncred commented Oct 30, 2023

I'm excited to see this. Thanks! Is there some way that once can confirm this is working? I've never see excess CPU so I'm not sure of the procedure exactly.

@sunfishcode
Copy link
Contributor Author

On Linux, for example, build the basic example, install strace, and then run strace target/debug/examples/basic. Before this PR, you'll see it calling epoll_wait 10 times a second, once it starts up. With this PR, it'll start up and then enter a call to epoll_wait that waits until a key is pressed.

@sunfishcode
Copy link
Contributor Author

Another way to see this on Linux is the powertop program. Before this PR, powertop shows a running basic example process having about 10 events/sec., which doesn't put it at the top of the list on my system, but it is on the list. With the PR, it's not on the list at all.

@fdncred
Copy link
Collaborator

fdncred commented Oct 30, 2023

The strace trick works as described. I can also see it in powertop as you suggest.

Before
image

After - i'm guessing these 0.25 events per second are me doing ls twice
image

I didn't notice anything abnormal with the basic example. It seemed to work as expected.

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me!

Also saw the reduction in epolls via strace.

I like how the change removes one layer of the loop making things a bit more readable. And having the wait condition directly between the reading and processing of the events makes intuitive sense.

@sholderbach sholderbach merged commit 2b6790c into nushell:main Nov 1, 2023
8 checks passed
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.

nushell consumes CPU when idle
3 participants