-
Notifications
You must be signed in to change notification settings - Fork 928
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
perf: optimize performance of waybar cpuinfo and gpuinfo modules #952
base: main
Are you sure you want to change the base?
Conversation
Optimize cpuinfo to use less resources by minimizing the number of forks as it is called once in every 5 seconds. Revert to the previous icon generation method for faster performance compared to jq after reducing forks.
Revise icon generation method to minimize forks, particularly resource intensive calls to awk as the script runs once in every 5 seconds.
Convert cpuinfo into a continuous script to avoid redundant calculations of constant values. Calculate utilization from /proc/stat as 'top' has been causing the performance bottleneck. Improve accuracy by averaging utilization over a 5 second interval instead of using instantaneous values.
Print glyphs instead of emojis if NO_EMOJI env var is set to 1. Reduce the number of function calls and redeclaration of variables.
Print glyphs instead of emojis if NO_EMOJI env var is set to 1 in gpuinfo module.
Disclaimer: I did not fully review it yet. Looks good in the description, so I won't dig deeper if this works on my end. 😁 Additionally, I believe you can pass the
My sys info if needed.
|
Oh no, The same happening on my machine too O_O Demn, I forgot to pass the temperature and utilization value to the function. |
I thought of adding |
Fix the bug causing the unexpected behavior due to passing wrong arguments to the map_floor function.
I meant like this. As having an ENV is too specific and overkill for just a single script haha so example command might look like this and can be invoke at startup
Sorry for making that slightly scary, it's the only way I can think of catering all 3 GPUs. |
Umm.. I see. It looks great but feels a bit complicated to me . Can you add that to the script if you don't mind or I can try later today.
No no, don't get me wrong, I was saying shell scripts are scary as a whole. I fixed the bug you pointed out btw.. please check again. |
Also we have the
Ow, I was afraid this version is not maintainable, but I'm wrong. You guys are Pros. Everything works on my end, BTW. for now |
This comment was marked as spam.
This comment was marked as spam.
Excellent! Thank you, you saved a lot of my time ❤️
It's true that it is a bit hard to understand (I didn't read most of it) but it's amazing how fast it runs. Initially the cpuinfo took around 320ms but your script runs at around 22ms on my machine. Pure genius. |
prevStat=$(awk '{print $2+$3+$4+$6+$7+$8 }' <<< $statFile) | ||
prevIdle=$(awk '{print $5 }' <<< $statFile) | ||
|
||
while true; do |
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.
why the loop? its not recommended to run this as a daemon process.
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.
Continuous script
As the script has to recalculate values that doesn't change during runtime (like CPU name and max freq), it is inefficient to run on every 5s. Since waybar allows the scripts to be executed in continuous mode which allows significant improvement on performance as the static values doesn't required to be recalculated on each iteration, the cpuinfo.sh has been adjusted to run as a continuous script.
It can be made to execute once in every 5s instead by saving the prev statistics to a temp file. However, I don't see why it is not recommended to run it as a daemon as re-executing the script has to deal with unnecessary system calls, read-writes and re-interpreting the script once in every 5s.
Also, it is not going to save cpu time on the scheduler as it has to deal with the sleep syscall from the script otherwise the same from the waybar.
It is also noted that the waybar builtin modules which requires updates on periodic intervals (memory, cpu_usage) are running on separate daemon threads.
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.
Continuous script
As the script has to recalculate values that doesn't change during runtime (like CPU name and max freq), it is inefficient to run on every 5s. Since waybar allows the scripts to be executed in continuous mode which allows significant improvement on performance as the static values doesn't required to be recalculated on each iteration, the cpuinfo.sh has been adjusted to run as a continuous script.
It can be made to execute once in every 5s instead by saving the prev statistics to a temp file. However, I don't see why it is not recommended to run it as a daemon as re-executing the script has to deal with unnecessary system calls, read-writes and re-interpreting the script once in every 5s.
Oh my bad I missed it, you did mention it, but it was too long so I didn't read :(
But still my concern is valid... I do not use the any of these monitoring modules in waybar as i often switch the waybar mode... So my question is that, will this process still run when the waybar mode changes?
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.
As per the confgen.sh
architecture, the waybar configuration file config.jsonc
is regenerated on switching the waybar mode. So this process won't be summoned by new instances of waybar as the modules will not be present in the config.jsonc
.
hyprdots/Configs/.local/share/bin/wbarconfgen.sh
Lines 125 to 130 in 8369bed
# restart waybar | |
if [ "$reload_flag" == "1" ] ; then | |
killall waybar | |
waybar > /dev/null 2>&1 & | |
fi |
According the above snippet, when you switch the mode, the waybar and its entire process tree will be killed, including our poor little daemon.
Sorry for the late reply, was little too busy these days.
Oh cool idea, why cant we just show gpu and cpu info in the same module? the script output could be both GPU and CPU temps combined. |
We can, but should we consider integrated GPU temp as the CPU temp too? I'll try to merge cpuinfo and gpuinfo into a single script, so they should share a single metadata. Then use options to it so that we can still get flexibility [REF] snippet at the bottom we can change Example syntax With this we can have more options to what to show.
Of course, I need help from @mislah to refactor the functions. |
util_lv="90:, 60:, 30:, " | ||
|
||
# Get static CPU information | ||
model=$(lscpu | awk -F': ' '/Model name/ {gsub(/^ *| *$| CPU.*/,"",$2); print $2}') |
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.
lscpu
respects LANG
and thus requires en_us
for this to work.
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.
Great, good find. I'll add this on the next commit.
Excellent, I also thought the same, as they are both quite similar. I'm a little too busy these days. I assure you that I'll help as much as I can, but I'm afraid I might not have enough time. |
No rush on this one. Everything is working as intended for now. Will try to prioritize other issues. I will add a PR @ https://github.com/mislah/hyprdots/tree/waybar/optimize soon. |
closing #2054 addresses merge |
I request the maintainers @prasanthrangan @T-Crypt to revert #2054 and reopen this pull request, as none of the commits from this original contribution have been preserved in #2054. Since I spent valuable time working on this, I believe I deserve proper attribution for my efforts. |
Hi @mislah ! , I understand you and honor you for this contribution. However I don't have the capability to revert some changes. I needed a co-collaborator/ contributor to do a PR again for the revert then so I can merge it. Then we can continue on this PR. I hope @T-Crypt wakes up again from great slumber,😅. I hope you understand. |
Icon generation bottleneck
The function to generate icons have been causing performance bottleneck since the beginning of cpuinfo module. The original version (3cf5d29,
c.sh
in the screenshot) has been taking around 30.5ms (not absolute, for comparison).To reduce the performance overhead, the revision (9b9b1ce,
d.sh
in the screenshot) have improved the speed to 22.3ms (37% faster than the original version). Here a singlejq
has been used to replace multiple forks ofawk
to improve the performance.The original version turns out to run on 6.58ms (364% faster than original and 238% faster than the revised) after minimizing the number of forks and removing all forks to
awk
(b.sh
in the screenshot). I also tested possibly the minimal versiona.sh
which contains only if branches completed in 5.4ms.Despite
a.sh
is the fastest, I includedb.sh
in both gpuinfo and cpuinfo as it follows Object Oriented principles and performance difference is insignificant (1.18ms).The forks
There are a number of redundant forks throughout script like:
maxfreq=$(lscpu | grep "CPU max MHz" | awk -F: '{ print $2}' | sed -e 's/ //g' -e 's/\.[0-9]*//g')
Which could be improved by substituting with:
maxfreq=$(lscpu | awk '/CPU max MHz/ { sub(/\..*/,"",$4); print $4}')
The number of forks have been minimized completely on
cpuinfo.sh
and partially ongpuinfo.sh
.top
bottleneckThe overall performance of the script wasn't improved significantly even after minimizing forks as the
top -bn1
taking almost 260ms as it has to compute information of all running and sleeping processes. The utilitympstat
is a great alternative but it requires the program to be installed on the system. Hencetop
is replaced by calculating the utilization levels directly from/proc/stat
which is taking around 15ms, that is 1633% improvement in performance.Continuous script
As the script has to recalculate values that doesn't change during runtime (like CPU name and max freq), it is inefficient to run on every 5s. Since waybar allows the scripts to be executed in continuous mode which allows significant improvement on performance as the static values doesn't required to be recalculated on each iteration, the cpuinfo.sh has been adjusted to run as a continuous script.
Other changes
Improved accuracy of utilization level as it now prints average between each updates. Also added option to decide whether to print emojis or glyphs by setting an environment variable
NO_EMOJI
as the commit: 9b9b1ce converted emojis to glyphs.Warning to reviewers
The changes included in the commit: a5cfdd0 have been included in this PR, but haven't tested as my machine doesn't have AMD CPU. Although, I tried testing with passing faked sensors data and worked well.
Testing
All the unit testing has been done by the script:
python -m timeit -n 10 -s 'import subprocess' "subprocess.call('./test.sh', shell=True)"
The
python timeit
is not absolute but is highly precise and useful for comparison. I thinkstrace
ortime
gives more accurate results but are not statistical or precise.Type of change
Checklist