htop icon indicating copy to clipboard operation
htop copied to clipboard

Add the "Active users" meter.

Open nojhan opened this issue 9 years ago • 7 comments

A text meter that display a list of users owning processes that are actually running. The list is truncated to the size of the display. Color active users according to levels of activity.

Users are categorized in 3 classes: high, middle and low activity. Users are displayed in descending order of activity, with different colors. Users shown in higher levels are not shown in lower ones, even if they have active processes in those levels. Bounds of the level classes are given as percentages: >95%, 95-50%, <50%.

Data are stored in a NEW Meter.strings array, that behaves just like the Meter.values one, but that handle strings instead of doubles. Remove the static attribute for RichString_writeFrom, because it is used by this module. Adds ACTIVE_USERS_* style fields.

This is a fixed and rebased follow-up of pull requests #219, #222 and #264.

nojhan avatar May 13 '16 19:05 nojhan

If I'm allowed to comment your patch:

  1. I don't like the Meter.strings[] idea. Is there any reason that you have to add such variable in the main Meter structure? And you added the same thing in the GraphData structure, why?
  2. Your function naming style is unlike the other parts of htop code. Maybe they can be changed right before merging.
  3. You did many malloc 's and calloc 's on function local variables without freeing them. This will result in memory leaks. You better fix them.

Explorer09 avatar May 14 '16 06:05 Explorer09

Thanks for the review!

  1. The e8fd6f0 commit adds the missing free (missed that one, indeed).
  2. While I failed to see how the prefixed convention is useful for static functions that are not part of the API, I prefixed all of them in e8fd6f0.
  3. I followed the implementation of the load meter, to avoid having different way of doing the same thing across the code base. Unfortunately, the load meter use the double values[METER_BUFFER_LEN] in GraphData (with a pointer from Meter), which cannot decently be used to store char*. So I added an additional strings array, using the very same approach than for values. I don't think I would risk myself refactoring the general data handling system in this PR, but this could be done later.

nojhan avatar May 14 '16 09:05 nojhan

@nojhan Could you please tell us the reason you need this strings array, so that I can free up the time understanding your code, and discuss whether there are alternatives to implement the same thing.

EDIT: Hey wait. I found a problem. Why are you comparing users by the username string while comparing by UID is enough? There is Process.st_uid. I hope you don't miss it.

EDIT2: Oh no. You implemented many things wrong. First you looped the process list with the conditional (i < pl->runningTasks) instead of (i < pl->totalTasks). This will let your meter not working if the process list is not sorted by State or is sorted in reverse order. Second, the processes with CPU% > 0.0 need not be in 'R' state. This is because a process can just be woken up to handle something and then go sleep immediately, without ps or top to ever record it's 'R' state. Third, the "CPU%" you use could go more than 100%, in case of a multi-threaded system. You should take the number of CPU cores into account. Fourth, I have no idea why you don't add up the CPU percentages of each process and use the sum total for each user? For example a multi-threaded program:

USER       PID %CPU STAT START   TIME COMMAND
explorer  3482  0.0 Sl   May14   0:00 /opt/google/chrome/chrome --type=renderer ...
explorer  3486  0.0 Sl   May14   0:00 /opt/google/chrome/chrome --type=renderer ...
explorer  3488  0.5 Sl   May14   0:14 /opt/google/chrome/chrome --type=renderer ...
explorer  3490  1.0 Sl   May14   0:27 /opt/google/chrome/chrome --type=renderer ...
explorer  3492  0.0 Sl   May14   0:00 /opt/google/chrome/chrome --type=renderer ...
explorer  3493  0.0 Sl   May14   0:00 /opt/google/chrome/chrome --type=renderer ...
explorer  3495  0.0 Sl   May14   0:00 /opt/google/chrome/chrome --type=renderer ...
explorer  3497  0.2 Sl   May14   0:06 /opt/google/chrome/chrome --type=renderer ...
explorer  3550  1.5 Sl   May14   0:41 /opt/google/chrome/chrome --type=renderer ...

Do you get the idea?

Explorer09 avatar May 14 '16 15:05 Explorer09

0.The strings array is used for the very same reason that the values array is used. AFAI, values stores the collected data to be displayed. As values is an array of doubles, it cannot be used to store the strings needed by the active users meter. It's the same logic than in the load meter (three data fields with different colors, for instance): htop_active_users I suppose that, as it would be better to use UIDs, we can (ab)use values and thus do not need a strings array.

0.5. At the time I wrote the patch, I did not found Process.st_uid (a dev doc would really be a plus, btw), I will check that.

1-2. So, is it sufficient to iterate over totalTasks and check for percent_cpu?

3-4. I was (naively) thinking that htop treated differently threads and processes. How do I properly iterate over threads of the same process? Is the CPU% given for the whole system or each core? How do I know on which core a process run?

nojhan avatar May 14 '16 19:05 nojhan

  1. The values array have one more purpose than just "the collected data to be displayed". They are also used for bar or graph plotting! Look at the CPU meters that you often have on the left - the (stacked) bars are drawn using this values thing! However, your newly added strings array is only meant for temporary buffer storage, which is why I feel it is inappropriate to put it there. Some meters do not use values for a purpose. Take the HostnameMeter for example; it's simply not applicable. You should not abuse values to store anything other than numeric values. Reasons are given above.
  2. I have mostly got the idea of what you want to achieve. So if I have time, I might implement a version by myself (as a code exercise :) ). What you want is essentially a (sorted) list of which users currently have processes been running (within the time interval). The list will be ordered by who takes the most of CPU time, to who takes the least time, if not zero. Am I correct? Just a reminder that this procedure could be expensive. (I cannot think of an algorithm that brings time complexity to lower than O(totalTasks * N * log(N)) where N=number of active users.)

Explorer09 avatar May 14 '16 20:05 Explorer09

  1. I agree that one "should not abuse values to store anything other than numeric values", that's precisely why I added the strings array in the first place. I'm afraid I failed to see the fundamental difference between data used by the CPU meter and data used by the load meter. Obviously, you better understand the htop architecture than me after having read the code of the load meter :-)
  2. It seems you correctly understood the description (note that users should not be shown twice in different levels, though, as stated in the PR description). Note that iff speed is your primary objective, I think it's pretty obvious that there is an algorithm in O(k log n). I'm always happy to provide exercises :-)

I would be greatly grateful if you could ping me once you merged your implementation in master, so that I could deploy it on my cluster instead of my current rough implementation.

nojhan avatar May 15 '16 10:05 nojhan

@nojhan Well, it seems like I've bumped into a technical limit when I try to implement this myself, see #499. Now I can excuse about your Meter.strings[] change.

Explorer09 avatar May 22 '16 14:05 Explorer09