search

apognu / tuigreet

Graphical console greeter for greetd
GNU General Public License v3.0
858 stars 43 forks source link

High CPU consumption since 0.7.0 #44

Closed varqox closed 2 years ago

varqox commented 2 years ago

In 0.7.0, 0.7.1 and master (64a41c33999b37b06e8799810b742b12ec4a30ad), the CPU consumption is unreasonably high -- htop shows that tuigreet uses around 160% CPU. When I tried 0.6.1 it came down to around 0% and this is reasonable. When I inspected with strace what was happening at 0.7.1, a lot of futex() calls appeared, so it looks like a lot of busy waiting (I saw that in between 0.6.1 and 0.7.0 some code was moved to async).

greetd command = tuigreet -c zsh System: Arch Linux

bezirg commented 2 years ago

I can confirm as well about the high cpu usage. I can also confirm that 0.6.0/0.6.1 have negligible CPU usage.

DjLogozzo commented 2 years ago

I can confirm to also having high CPU usage in 0.7.0 and later, Using git bisect, it seems the commit that introduced the issue was f12688fa0d34a2063a086f84b6144d6dbaac7f8f. With a cursory glance at the commit, this segment of code, https://github.com/apognu/tuigreet/blob/f12688fa0d34a2063a086f84b6144d6dbaac7f8f/src/main.rs#L65-L95 which corrosponds to https://github.com/apognu/tuigreet/blob/64a41c33999b37b06e8799810b742b12ec4a30ad/src/main.rs#L71-L83 in the current HEAD (64a41c33999b37b06e8799810b742b12ec4a30ad), seems to be the most likely candidate for the high CPU usage, which makes sense that varqox saw a high number of futex() calls.

DjLogozzo commented 2 years ago

After a bit of testing, this loop continuously obtaining a write lock on the greeter does seem to be the issue, a simple (if a bit crude) solution to this would be to add a small async sleep to the loop so that it isn't free-running constantly. Here is an example of this solution committed to my fork https://github.com/DjLogozzo/tuigreet/commit/8c1044dec4e2d05cc69bac20ce4651eed19568e9 (And accompanying PR #46).

A more complete solution using something like message passing (possibly using a tokio oneshot channel or similar) would likely require some amount of refactoring of the power_command related code

The only drawback I can think of for a solution like this would be an additional 0-100ms delay (Depending on what point during the sleep you issue the power command, on average this would result in a 50ms delay, or about 3 frames on a 60Hz monitor) between selecting a power command and the command actually executing, however in testing this seems to be imperceptible (at least I haven't been able to notice a difference).

DjLogozzo commented 2 years ago

After thinking about it for a bit, a fairly unobtrusive but more complete solution (compared to a simple async sleep, however it's still a band-aid solution) would be to use a tokio Notify system, and parking the thread until the power_command gets updated by the user, I've implemented a fix using this solution to my fork https://github.com/DjLogozzo/tuigreet/commit/5cab7deaed2af565aefa09795e4026690a6f3896 (Which has updated the accompanying PR #46).

apognu commented 2 years ago

Sorry I haven't been available those past few months, it's been a bit hectic on my side. I'll look at the PR and come back to you.

apognu commented 2 years ago

0.7.2, which includes this fix, was tagged and deployed.