-
-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -660,12 +660,14 @@ void Process_writeField(const Process* this, RichString* str, RowField field) { | |||||||||||||||||
xSnprintf(buffer, n, "%4ld ", this->nlwp); | ||||||||||||||||||
break; | ||||||||||||||||||
case PERCENT_CPU: Row_printPercentage(this->percent_cpu, buffer, n, Row_fieldWidths[PERCENT_CPU], &attr); break; | ||||||||||||||||||
case PERCENT_CPU_GROUP: Row_printPercentage(this->percent_cpu_group, buffer, n, Row_fieldWidths[PERCENT_CPU_GROUP], &attr); break; | ||||||||||||||||||
case PERCENT_NORM_CPU: { | ||||||||||||||||||
float cpuPercentage = this->percent_cpu / host->activeCPUs; | ||||||||||||||||||
Row_printPercentage(cpuPercentage, buffer, n, Row_fieldWidths[PERCENT_CPU], &attr); | ||||||||||||||||||
break; | ||||||||||||||||||
} | ||||||||||||||||||
case PERCENT_MEM: Row_printPercentage(this->percent_mem, buffer, n, 4, &attr); break; | ||||||||||||||||||
case PERCENT_MEM_GROUP: Row_printPercentage(this->percent_mem_group, buffer, n, 4, &attr); break; | ||||||||||||||||||
case PGRP: xSnprintf(buffer, n, "%*d ", Process_pidDigits, this->pgrp); break; | ||||||||||||||||||
case PID: xSnprintf(buffer, n, "%*d ", Process_pidDigits, Process_getPid(this)); break; | ||||||||||||||||||
case PPID: xSnprintf(buffer, n, "%*d ", Process_pidDigits, Process_getParent(this)); break; | ||||||||||||||||||
|
@@ -915,8 +917,12 @@ int Process_compareByKey_Base(const Process* p1, const Process* p2, ProcessField | |||||||||||||||||
case PERCENT_CPU: | ||||||||||||||||||
case PERCENT_NORM_CPU: | ||||||||||||||||||
return compareRealNumbers(p1->percent_cpu, p2->percent_cpu); | ||||||||||||||||||
case PERCENT_CPU_GROUP: | ||||||||||||||||||
return compareRealNumbers(p1->percent_cpu_group, p2->percent_cpu_group); | ||||||||||||||||||
case PERCENT_MEM: | ||||||||||||||||||
return SPACESHIP_NUMBER(p1->m_resident, p2->m_resident); | ||||||||||||||||||
case PERCENT_MEM_GROUP: | ||||||||||||||||||
return SPACESHIP_NUMBER(p1->percent_mem_group, p2->percent_mem_group); | ||||||||||||||||||
case COMM: | ||||||||||||||||||
return SPACESHIP_NULLSTR(Process_getCommand(p1), Process_getCommand(p2)); | ||||||||||||||||||
case PROC_COMM: { | ||||||||||||||||||
|
@@ -1073,6 +1079,51 @@ void Process_updateCPUFieldWidths(float percentage) { | |||||||||||||||||
Row_updateFieldWidth(PERCENT_NORM_CPU, width); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
void Process_updateCPUGroupFieldWidth(float percentage) { | ||||||||||||||||||
if (percentage < 99.9F) { | ||||||||||||||||||
Row_updateFieldWidth(PERCENT_CPU_GROUP, 4); | ||||||||||||||||||
return; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// Add additional two characters, one for "." and another for precision. | ||||||||||||||||||
uint8_t width = ceil(log10(percentage + 0.1)) + 2; | ||||||||||||||||||
|
||||||||||||||||||
Row_updateFieldWidth(PERCENT_CPU_GROUP, width); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
void Process_resetGroupFields(pid_t _pid, Process* this, void* _data) { | ||||||||||||||||||
this->percent_cpu_group = this->percent_cpu; | ||||||||||||||||||
this->percent_mem_group = this->percent_mem; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
void Process_updateGroupFields(pid_t _pid, Process* this, void* _data) { | ||||||||||||||||||
const Machine* host = this->super.host; | ||||||||||||||||||
const ProcessTable* pt = (const ProcessTable*) host->processTable; | ||||||||||||||||||
Process* curProcess = this; | ||||||||||||||||||
|
||||||||||||||||||
bool looping = curProcess->percent_cpu_group > 0 || curProcess->percent_mem_group > 0; | ||||||||||||||||||
|
||||||||||||||||||
while (looping) { | ||||||||||||||||||
const pid_t pid = Process_getPid(curProcess); | ||||||||||||||||||
const pid_t parentPid = Process_getGroupOrParent(curProcess); | ||||||||||||||||||
if (parentPid == pid) { | ||||||||||||||||||
looping = false; | ||||||||||||||||||
|
||||||||||||||||||
break; | ||||||||||||||||||
} | ||||||||||||||||||
Process* parentProc = Hashtable_get(pt->super.table, parentPid); | ||||||||||||||||||
if (parentProc == NULL) { | ||||||||||||||||||
// TODO: or assert ? | ||||||||||||||||||
looping = false; | ||||||||||||||||||
break; | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
+1115
to
+1119
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
An assert may trigger accidently here, if the parent of a process is not captured in the process scan (e.g. just started) |
||||||||||||||||||
parentProc->percent_cpu_group += this->percent_cpu; | ||||||||||||||||||
parentProc->percent_mem_group += this->percent_mem; | ||||||||||||||||||
curProcess = parentProc; | ||||||||||||||||||
} | ||||||||||||||||||
Process_updateCPUGroupFieldWidth(curProcess->percent_cpu_group); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
const ProcessClass Process_class = { | ||||||||||||||||||
.super = { | ||||||||||||||||||
.super = { | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
float percent_cpu_group; | ||||||
|
||||||
/* Memory usage during last cycle (in percent) */ | ||||||
float percent_mem; | ||||||
|
||||||
/* Sum of percent_mem for self and children */ | ||||||
float percent_mem_group; | ||||||
|
||||||
/* Scheduling priority */ | ||||||
long int priority; | ||||||
|
||||||
|
@@ -324,4 +330,8 @@ void Process_writeCommand(const Process* this, int attr, int baseAttr, RichStrin | |||||
|
||||||
void Process_updateCPUFieldWidths(float percentage); | ||||||
|
||||||
void Process_resetGroupFields(pid_t _pid, Process* this, void* _data); | ||||||
|
||||||
void Process_updateGroupFields(pid_t pid, Process* this, void* _data); | ||||||
|
||||||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Try to include this inside the call to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
Hashtable_foreach(this->super.table, Process_updateGroupFields, NULL); | ||
} | ||
|
||
static void ProcessTable_iterateEntries(Table* super) { | ||
ProcessTable* this = (ProcessTable*) super; | ||
// calling into platform-specific code | ||
ProcessTable_goThroughEntries(this); | ||
ProcessTable_aggregate(this); | ||
} | ||
|
||
static void ProcessTable_cleanupEntries(Table* super) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,8 @@ typedef enum ReservedFields_ { | |
|
||
/* Platform specific fields, defined in ${platform}/ProcessField.h */ | ||
PLATFORM_PROCESS_FIELDS | ||
PERCENT_CPU_GROUP = 130, | ||
PERCENT_MEM_GROUP = 131, | ||
Comment on lines
+47
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
|
||
/* Do not add new fields after this entry (dynamic entries follow) */ | ||
LAST_RESERVED_FIELD | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,8 +41,10 @@ const ProcessFieldData Process_fields[LAST_PROCESSFIELD] = { | |
[M_RESIDENT] = { .name = "M_RESIDENT", .title = " RES ", .description = "Resident set size, size of the text and data sections, plus stack usage", .flags = 0, .defaultSortDesc = true, }, | ||
[ST_UID] = { .name = "ST_UID", .title = "UID", .description = "User ID of the process owner", .flags = 0, }, | ||
[PERCENT_CPU] = { .name = "PERCENT_CPU", .title = " CPU%", .description = "Percentage of the CPU time the process used in the last sampling", .flags = 0, .defaultSortDesc = true, .autoWidth = true, }, | ||
[PERCENT_CPU_GROUP] = { .name = "PERCENT_CPU_GROUP", .title = " GCPU%", .description = "Percentage of the CPU time the process and its children used in the last sampling", .flags = 0, .defaultSortDesc = true, .autoWidth = true, }, | ||
[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 commentThe reason will be displayed to describe this comment to others. Learn more. Why was this description updated? |
||
[PERCENT_MEM_GROUP] = { .name = "PERCENT_MEM_GROUP", .title = "GMEM% ", .description = "Percentage of the memory the process is using, based on resident memory size", .flags = 0, .defaultSortDesc = true, }, | ||
[USER] = { .name = "USER", .title = "USER ", .description = "Username of the process owner (or user ID if name cannot be determined)", .flags = 0, }, | ||
[TIME] = { .name = "TIME", .title = " TIME+ ", .description = "Total time the process has spent in user and system time", .flags = 0, .defaultSortDesc = true, }, | ||
[NLWP] = { .name = "NLWP", .title = "NLWP ", .description = "Number of threads in the process", .flags = 0, }, | ||
|
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.