Improve performance and code style, refactor, partial rewrite #17
Conversation
This commit addresses TODO#2 by universalizing the functions handling the zombie processes and sending `SIGCHLD` in the case where the user did not explicitly tell the program to reap zombies (reflected in the `terminate` member of our settings. Whenever a signal is sent, the used signal is displayed to the user. Some parts of old functions were put into new functions to also improve readability. The most substantial improvement lies in the performance aspect of the program. The use of `nftw()` in the old code was the biggest bottleneck, as an insane, unnecessary number of syscall are issued for each process. Thus, the directory and file reading part was adapted, using manual `readdir()` calls. As the `-f` flag did not make sense to have in the program anyway (and could lead to OOM kill), it was also dropped. Regex was also dropped, as its use was unnecessary and complicated things, and the parsing of `"/proc/<pid>/stat"` was rewritten. As kernel processes were not excluded in the program before, this commit checks for processes which should be left out of our considerations (PID `2`, as well as its children; `init` is also excluded). Display widths for the output are now defined in the accompanying header, and should be in line with the `proc(5)` documentation. The code for the prompt was improved with respect to the handling of invalid user input and coloring. Finally, to completely remove the unnecessary globals, a vector structure was defined in the header, which can dynamically grow. As the old version of the code did not check for overflows in this context, this is also a great improvement.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #17 +/- ##
==========================================
- Coverage 96.18% 86.80% -9.38%
==========================================
Files 1 2 +1
Lines 131 235 +104
==========================================
+ Hits 126 204 +78
- Misses 5 31 +26 ☔ View full report in Codecov by Sentry. |
|
Oh wow, this is huge! I believe there are a lot of things that I will learn from this code so I will review it carefully. Obviously I'm not a C guru so expect some beginner questions :) Anyhoo, thanks a lot for this. I just tested it and the speed difference is freaking unbelievable ⚡ |
Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>
This will disable the pre-condition assertions in all functions, which should never be false for releases anyway.
This commit ... * allows users to directly specify signals to send to zombie parents (using common signal numbers, abbreviations and abbreviations with "sig" in front of them (case-insensitive) * changes existing command-line options to simpler ones * places constraints on the possible combinations of command-line options used * adds functionality to detect if the program's output is, for example, piped to another program that will automatically disable ANSI color codes * adds a `--no-color` option to manually disable the default coloring The manual page and READMEs would still need to be updated.
mforthewin
changed the title
SIGCHLD signal by default and remove the -f flag
|
I've also made some changes to the CLI options, adding some new functionalities, such as a signal option. Let me know what you think – the man page and the READMEs would need an update before we should merge this, so I marked it as a draft again. |
|
Looks good! Let's go ahead and finalize this, I can provide a review soon. |
Alright! I've updated the remaining files and also added updated demo GIFs. |
|
Is there anything left to do? Personally, I think this is in a good state now for merging. |
|
Yes, this is awesome, thanks a lot once again! I will create a release now and share it on my socials tomorrow. |
The program in its current state takes a lot of time to execute compared to programs like
ps.The reasons for this behavior are for example:
nftw(), which results in LOTS of unnecessarynewfstatat()syscalls"/proc/<pid>/stat"This becomes clear when profiling the syscalls:
Those are almost 500k syscalls! If we compare this to the new version of the code, the difference is huge:
Also looking at real execution times, we can see the following speedup:
Another aspect is that having a
-fflag along withnftw()is not sensible, as this could happen on a box with2GB of RAM:As we can see, it is trying to allocate more than
5.21GB and the program is getting OOM-killed! This defeats the actual purpose of the program (freeing resources). Thus, I propose these changes to fix any issues of performance or resource usage. Additionally, I addressed TODO#2, and the program now sends aSIGCHLDsignal by default (without the reap option). Also, as thestaticglobal array in the program was not optimal, a simple vector implementation is used now, eliminating the overflow issues from before. Please see commit b20d954 for more specific details.