-
-
Notifications
You must be signed in to change notification settings - Fork 442
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
add grouped memory & cpu columns #1338
base: main
Are you sure you want to change the base?
Conversation
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.
Have you done some performance testing how much this additional column influences the overall performance?
Also it's likely desirable to skip calculating these aggregates, if none of the aggregate columns is used. See the various column flags that are in place for other optional information.
if (parentPid == pid) { | ||
looping = false; | ||
|
||
break; | ||
} |
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.
No need to set logging=false;
if it's not used beyond this loop anyway.
Also, have a look at Brent's algorithm for loop detection.
if (parentProc == NULL) { | ||
// TODO: or assert ? | ||
looping = false; | ||
break; | ||
} |
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.
if (parentProc == NULL) { | |
// TODO: or assert ? | |
looping = false; | |
break; | |
} | |
if (!parentProc) { | |
break; | |
} |
An assert may trigger accidently here, if the parent of a process is not captured in the process scan (e.g. just started)
@@ -146,9 +146,15 @@ typedef struct Process_ { | |||
/* CPU usage during last cycle (in percent) */ | |||
float percent_cpu; | |||
|
|||
/* Sum of percent_mem for self and children */ |
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.
/* Sum of percent_mem for self and children */ | |
/* Sum of percent_cpu for self and children */ |
@@ -55,10 +55,16 @@ static void ProcessTable_prepareEntries(Table* super) { | |||
Table_prepareEntries(super); | |||
} | |||
|
|||
static void ProcessTable_aggregate(ProcessTable* this) { | |||
Hashtable_foreach(this->super.table, Process_resetGroupFields, NULL); |
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.
Try to include this inside the call to ProcessTable_goThroughEntries
, as this avoids one iteration over the hash map (which is not the fastest).
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.
This particular feedback might take time.
Specifically, I don't want to write anything in platform-specific files as the idea or the implementation isn't platform-specific at all. But I also agree with you on the unnecessary performance cost. In this dilemma, I want to explore my options and find something better, which may take time, maybe in weeks.
The rest of the feedback, including using PROCESS_FLAG_* like flags, is already done.
PERCENT_CPU_GROUP = 130, | ||
PERCENT_MEM_GROUP = 131, |
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.
Global fields should be defined in the enum definition before platform-specific ones.
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.
The only concern was not to affect existing enum values. Now, I've chosen to make these 127, 128 and had to do AUTOGROUP_ID = 129
, AUTOGROUP_NICE = 130
, CCGROUP = 131
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.
I didn't see any place in htop codebase mentioning that global fields should be before platform specific ones. Personally I think htop has no specific rules for assigning field IDs and the current allocation is messy. Ideally the fields IDs should be stable across htop releases so the configuration files can refer the fields by numbers. In addition, we should reserve ranges of field IDs for specific purposes so that, e.g. dynamic fields would not use them causing collisions.
I would cite MediaWiki namespace IDs as a good example of how IDs can be allocated are how IDs are reserved for future uses.
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.
It's not a matter of numeric values; just a code ordering. The macro including the platform flags should be the last line in that definition.
Regarding actual allocations: Old versions of htop used to write the field IDs into the config files. With more recent versions this has been changed to use name representations instead, thus the only thing that needs to be kept for BC is with the columns available with old htop versions that still stored the numeric IDs.
But yes, the current assignment is kinda messy, but nothing too easy to fix over night.
[PERCENT_NORM_CPU] = { .name = "PERCENT_NORM_CPU", .title = "NCPU%", .description = "Normalized percentage of the CPU time the process used in the last sampling (normalized by cpu count)", .flags = 0, .defaultSortDesc = true, .autoWidth = true, }, | ||
[PERCENT_MEM] = { .name = "PERCENT_MEM", .title = "MEM% ", .description = "Percentage of the memory the process is using, based on resident memory size", .flags = 0, .defaultSortDesc = true, }, | ||
[PERCENT_MEM] = { .name = "PERCENT_MEM", .title = "MEM% ", .description = "Percentage of the memory the process and its children are using, based on resident memory size", .flags = 0, .defaultSortDesc = true, }, |
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 was this description updated?
I don't like this.
|
Names can be changed. And I'm not quite happy with them either …
That's mostly a matter of taste, as you can argue both ways. Either you display always the true values and have aggregate columns to add things up, or you have a mixed column, where some values are aggregates and you end up having to mark, which values are aggregates and which are not. I personally prefer the solution using a separate column, as it does not loose you any information, but to the contrary actually tells you, how much of the aggregate value is from child processes.
Can you elaborate on this item? |
@BenBE
This is my personal opinion. I think the advantage of having a tree view is to have a quick idea which processes are part of a group and which process groups form a session. It's not just about tracking the parents of the processes. So I think there are rooms to improve the Tree View usability, especially with the goal of managing processes in groups and sessions. When it comes to sorting, I mean the |
@Explorer09 I agree with most of what you said.
I am still thinking about the right name and am not convinced with what current ones. I've mentioned same in PR body also, and will try to find better ones. If using non-alphabetical ASCII characters ( like %CPU┼ , %CPU± , %CPU┬ , %CPU╦ , %CPU» , %CPU* etc.) is fine, then we can have more combinations to try & explore. Not good examples, but I will give it more time.
Actually, I thought in this direction originally. My requirement was simple, to see the aggregate CPU/MEM%, irrespective of whether it is shown by a new setting or a new column. But then I wanted to see how many resources a tree is using vs. the parent & children nodes side-by-side. This could be achieved only by adding a new column type. We can also have more aggregate columns which aren't derived from current ones, like
I also noticed the same. I'm thinking of making a separate PR for that as it will impact the existing columns also. |
A quick and dirty implementation shows that it takes <10 ms on my machine, running ~200tasks at the moment. |
Using Unicode is fine in htop, but we would need ASCII fallback as not every user would run htop with UTF-8 mode enabled.
I knew your idea, but the problem is that there could be a lot of columns introduced just for aggregated stats after we pull in your patch as the first example. I have a better vision for this: if the aggregated data for a process group or session is important and worth displaying separately from the parent process node, I would introduce separate rows for displaying the aggregated info. Such rows would be colored differently from the processes and threads (for a better visual distinction). As these row do not refer to individual processes or threads:
How is that? |
To be precise: Enabling Unicode support is an optional compile-time feature, thus columns should be able to be displayed with ASCII alone.
ACK. That's also why I'm not really fired-up about this yet. I see both sides and my initial review was from a technical PoV only.
Not really a great option either, as this causes vertical screen estate to be used up for this. An idea to explore may be implementing "multi-mode" columns, similar to the meter display mode, where the same column could be switched to display different values (in the configuration).
That looks like #303 on first glance … |
I think the idea needs to be refined more to be convincing. Not sure if it is only me but when I try to evaluate this idea based on the value it adds relative to the development effort, I don't find it much pleasing.
"multi-mode" columns also sound like a great idea. If we're having that, then we can also allow the user to add duplicate columns (same columns with different modes) to his screen.
|
Well, the development effort is not something I would evaluate as htop is an open source project without any definite deadline or goal (in terms of features). Maintenance effort is what we would evaluate instead for every new feature added.
Do you mean the number of child process in the Running (
I will let @BenBE correct me if I get this wrong, but my vision is that the columns' mode would be global, and you won't see per-process CPU% and aggregated CPU% side by side. Only one will be displayed at a time, but a hotkey would be available to quickly switch between modes -- that is, between per-process data and aggregated data. |
Currently there's no real column for this. If you stretch definitions you could re-use the
ACK.
Correct. And one of the reasons #303 didn't get anywhere yet is the vague definition for what constitutes an application and what not. But that's something to discuss over there.
That would be either multiple columns, or with multi-mode columns this could be done as one mode:
Both a "global column mode" and a "per-column mode" are possible options. It's basically a question of which direction we decide to go with this. But I'm more in the "per-column mode" camp with that idea, as that is the most flexible and is the closest to what this PR already tries to do. That being said: We should keep discussions for this PR on topic and split the "multi-mode columns" and "row grouping" (e.g. applications) to separate discussions to keep this PR manageable. Those topics would need to be addressed in separate PRs anyway. Finally, while from a technical PoV this PR LGTM (sans notes) I'm not completely happy with the overall feature (despite generally being in favour for it). The issue I think we need to address before continuing is the general discussion about whether we'd potentially want to introduce quite a few (similar) such columns, or if there's a better way to go ahead with this. |
Fixes #301 , #920 , #1154
Add
GCPU
(Grouped CPU) andGMEM
(Grouped Memory) columns which show sum of cpu/memory for the current process & its children.Open Questions
GCPU
,GMEM
) acceptable ? Could've used keywords liketree
,children
to derive new names ( likeTCPU
,CPUT
, or maybe something else. )Linux 6.5.0-13-generic
- Kubuntu 23.10 )Screenshot