[REQUEST] Improve multi-threaded code

[imwints]

I had a deeper look at the code in the last few weeks and came to the conclusion that the Runner::_runner() method is redundant.

If I understand everything correctly, we execute the Runner::run() method. At the end, we signal the Runner thread that there's work to do, so it starts executing.

There is hardly any gain from executing this piece of code in a background thread. The work performed there does not take advantage of parallel execution because it has to wait for the main thread anyways. The current downsides are having lots of atomics and unreadable code.

Changing this will make the code much easier to understand because the control flow follows the execution, you don't have to guess what is happening in parallel and jump around.

I've already tested it and moved all the code from Runner::_runner in the Runner::run() method and removed the thread and most of it's atomic booleans. There is no loss in performance (as I've said, the code gets executed in a serial fashion anyway).

There are other areas which would benefit from parallel execution like polling for input and an multi-threaded signal handler.

[aristocratos]

@nobounce
This would work fine on most modern single-user systems.
The issue is with resource constrained and low powered systems, where the total time for information collection takes above ~30ms (approximate number for when people start to notice lag).

For example on a server with multiple thousands running processes, it will take a long time to parse through /proc.
Or on a low powered raspberry pi you might be lucky to have a collection cycle below 50ms.
In those cases it there will be a noticeable lag when for example just browsing up and down the process list.

So mostly the reason for having it on separate threads is to make sure the UI can respond to input (that don't require a new collection cycle) even if the collection is slow.

However the current threading mess in btop can surely be improved.
As I've mentioned before, btop++ was the first software I ever wrote in C++... a lot of mistakes were made, and I would have written it very differently if I started with the C++ experience I have now :P

[imwints]

I can take on that challenge.
I will rewrite the threading code if this is in your interest.

I have a couple of PI's around to test it, maybe I get to write some tests that simulate heavy loaded systems.

[imwints]

@aristocratos
My goals would be:

1. Cleaner threading code

• using std::thread over pthread
• maybe a custom std::jthread, as it's only in GCC right now

2. Less globals

• it's hard to track what the thread does if you have to also check what globals changed where and when

3. Cleaner code path when exiting

• currently we have recursive calls to clean_quit
• I'm not a 100% sure but I don't think local vars in main are cleaned up properly

4. Fast collecting and redrawing

• run collect right after redrawing (or as soon as it makes sense)
• if the thread is done early, wait for the main thread to draw

[aristocratos]

@nobounce

I can take on that challenge.

You're most certainly welcome to try :)

Less globals

The globals used are mostly for atomic signaling between threads and all are contained in the Global:: namespace, I don't really see any major issue with it and the need for globals should decrease with better thread handling.

currently we have recursive calls to clean_quit

This is because of how signal handling was initially setup. This can be avoided by setting a flag in the signal handler, returning to the thread where the signal was caught and then have a delayed response to the signal in the main thread.

I'm not a 100% sure but I don't think local vars in main are cleaned up properly

There's no need to clean anything up really with the exception of making sure no threads are left running. The OS will take care of clean up.

run collect right after redrawing (or as soon as it makes sense)

You've got it turned around. You want to run the collection as late as possible before drawing to screen, otherwise you are not getting realtime data anymore.
Right now the collection cycles are triggered according to the set update frequency, the counter restarts as the collection cycle begins to get an as stable sample period as possible (since some stats are dependent on comparison with the previous results).

However, moving all "drawing to screen" to the main thread might be a good idea. But there will still be a need for either waiting for the collection cycle to finish or signal it to stop when changing options that inform how collection should be done.

[imwints]

@aristocratos
I've got some new ideas on this. Currently working on it.
Can we drop GCC 10 support? It wasn't recommended due to partial C++ 20 support (I think) anyways. Is there any Distro out there without access to GCC 11 or later?

[imwints]

Also can I assume #624 is going to be merged? I'll have overlapping code and I'm not looking forward to rebase this. So I would like start on that branch

[aristocratos]

@nobounce
I have a replacement for threading ready, from another project based on btop where I rewrote large parts of the threading model, configuration model and more. (The project is not public yet).

#624 will be merged if it's stable on all platforms (when I have time to do testing).

Many of your current pull requests are mostly 😉 good improvements/ideas, but they touch a lot code in a lot of different places (not helped by your automatic linting changing indentations, headers order and so on).
So they'll have to wait until I have time to finish up GPU support and the pending threading and configuration changes.

The code in the other project was cleaned of warnings with the following clangd compile_flags.txt settings:

-std=c++20
-pthread
-g
-O0
-D_FILE_OFFSET_BITS=64
-fexceptions
-Wall
-Wextra
-Wpedantic
-Wnon-virtual-dtor
-Wcast-align
-Wunused
-Woverloaded-virtual
-Wnull-dereference
-Wdouble-promotion
-Wno-unused-command-line-argument
-Wmisleading-indentation
-Wformat=2
-Wduplicated-cond
-Wduplicated-branches
-Wlogical-op
-Wnull-dereference
-Wold-style-cast
-Wuseless-cast
-Wjump-misses-init
-Wdouble-promotion
-Wshadow
-fsanitize=undefined,address,leak
-fsanitize-recover=address
-fsanitize-address-use-after-scope
-fsanitize-address-use-odr-indicator
-fsanitize-cfi-cross-dso
-fsanitize-memory-track-origins=2
-fsanitize-recover=address
-Iinclude

So will be bringing that over at some point in the future also, for some added safety checks.

[imwints]

Many of your current pull requests are mostly 😉 good improvements/ideas, but they touch a lot code in a lot of different places (not helped by your automatic linting changing indentations, headers order and so on).

I'm sorry, that's an ick, I can't help it 😆 I understand that the GPU thingy is on your list for 1.3 so you don't want to delay it further. If the header stuff is the only thing holding my PR's back I can omit them.