Enhance Readability of DISK READ/DISK WRITE Columns by Shortening Labels and Values #1557

AryanGitHub · 2024-10-19T04:01:09Z

Observations before addressing GitHub issue #991:

The column titles for RCHAR & WCHAR take 6 characters, while the titles for DISK READ, DISK WRITE, and DISK R/W (representing total I/O rate in bytes per second) take 12 characters. This PR reduces them to 9 characters.
Why not make the column titles for DISK operations consistent with RCHAR & WCHAR?
The RCHAR & WCHAR columns use an unsigned long long data type, while DISK READ & DISK WRITE use double, which adds an extra character for the decimal point when printing.
The DISK READ & DISK WRITE columns represent rates, which include a time dimension, thus requiring 2 additional characters compared to RCHAR & WCHAR.
In total, we need 3 extra characters for DISK READ, DISK WRITE, and DISK R/W compared to RCHAR & WCHAR.

What has this PR changed?

This PR changed the column titles as required and
introduced a dynamic precision variable, adjusting based on the value to be printed. The total number of numerical characters allowed for printing is 4. The precision adjusts as follows:

If the value is between 0-9, the precision is 3, e.g., 3.141.
If the value is between 10-99, the precision is 2, e.g., 31.41.
If the value is greater than 99, the precision is 1, e.g., 314.1.
If the value is greater than 999, the precision is 0, e.g., 3141.

AryanGitHub · 2024-10-19T04:50:35Z

I also wanted to know when, while changing the total width of "Row_printRate", do we need to modify this line in ./pcp/PCPDynamicColumn.c:

htop/pcp/PCPDynamicColumn.c

Line 298 in 4102862

column->super.width = 11; // Row_printRate

This is because I don't know, what this does. please inform me about this, I will try to make the changes accordingly.

Explorer09 · 2024-10-19T07:28:51Z

Row.c

-      int len = snprintf(buffer, sizeof(buffer), "%7.2f K/s ", rate / ONE_K);
+      double rateToPrint =  rate / ONE_K;
+      unsigned char precision = Row_getPrecision (rateToPrint);
+      int len = snprintf(buffer, sizeof(buffer), "%5.*fK/s ", precision, rateToPrint);


precision takes the type int. You should set the data type properly.

There is a potential of converting this into a loop. You can see the code of Meter_humanUnit() function for how it can be done. There is a global unitPrefixes array that you can utilize.

| I don't know, what this does. please inform me about this, I will try to make the changes accordingly.

This (11) sets the default column width if none is specified in the configuration file from memory - if we're changing the default reporting width in Row.c its very likely this pcp code will need to change to match it.

Row.c

BenBE · 2024-10-19T08:32:15Z

The spacing for RDISK, WDISK and RW DISK is inconsistent.

Further more, DIOR, DIOW and DIORW would be even shorter and even consistent.

From the display side of things: There's a list of IEC unit prefixes for a reason. Please use that one to avoid code duplication.

Also, the space before B/s is intentional to ease reading 8B/s. Which can be hard to discern with some console fonts.

AryanGitHub · 2024-10-20T02:35:47Z

Row.c:511:13: warning: 6th function call argument is an uninitialized value [core.CallAndMessage]
  511 |       len = snprintf(buffer, sizeof(buffer), "%c/s ", unitPrefixes[unitPrefixIndex]);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/stdio2.h:77:3: note: expanded from macro 'snprintf'
   77 |   __builtin___snprintf_chk (str, len, __USE_FORTIFY_LEVEL - 1,                \
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   78 |                             __glibc_objsize (str), __VA_ARGS__)
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

What is left uninitialized? I am unable to understand.

Explorer09 · 2024-10-20T07:17:15Z

Row.c:511:13: warning: 6th function call argument is an uninitialized value [core.CallAndMessage]
  511 |       len = snprintf(buffer, sizeof(buffer), "%c/s ", unitPrefixes[unitPrefixIndex]);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/stdio2.h:77:3: note: expanded from macro 'snprintf'
   77 |   __builtin___snprintf_chk (str, len, __USE_FORTIFY_LEVEL - 1,                \
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   78 |                             __glibc_objsize (str), __VA_ARGS__)
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

What is left uninitialized? I am unable to understand.

Potential reading out of bounds. unitPrefixes[-1] unhandled (you probably want it to print B instead.)

Explorer09 · 2024-10-21T03:25:23Z

Row.c

-      int len = snprintf(buffer, sizeof(buffer), "%7.2f T/s ", rate / ONE_T);
-      RichString_appendnAscii(str, largeNumberColor, buffer, len);
+      int len = snprintf(buffer, sizeof(buffer), "%4.0f B/s ", rate);
+      RichString_appendnAscii(str, shadowColor, buffer, len);


Why using shadowColor? Ideally nonzero values should not use that text color -- the visual of that would let users skip that entry.

I used it because it was mentioned in the Issue.
#991 (comment)

Well. I would personally disagree with @BenBE unless these small values would practically make a lot of visual noise. But I am not the one to decide on this (I just put my suggestion).

IIRC, the current code shadows values below 1KiB/s. Fine either way IMO, but the note from @Explorer09 regarding accidentally visually skipping small values is valid.

As of now, according to the code in the main branch, only values below 0.005 B/s are in shadowColor.

However, in this pull request, I have made shadowColor include all values below 1 KiB/s. This change may cause small values to be accidentally skipped visually, especially for existing users. Therefore, I will change it back to the original settings.

Row.c

Explorer09 · 2024-10-21T03:42:57Z

Row.c

+         }
+      }
+
+      int len = snprintf(buffer, sizeof(buffer), "%5.*f", precision, rate);


I thought you would want "%4.*f" as the format. That is, one digit less.

Output example A

0 B/s - 9999 B/s 9.77 K/s - 9.99 K/s 10.0 K/s - 99.9 K/s 100 K/s - 9999 K/s 9.77 M/s - 9.99 M/s ... 100 Q/s - 9999 Q/s infinity

Output example B

0 B/s - 1023 B/s 1.00 K/s - 9.99 K/s 10.0 K/s - 99.9 K/s 100 K/s - 1023 K/s 1.00 M/s - 9.99 M/s ... 100 Q/s - 9999 Q/s infinity

we actually want 4 digits.
By using "%5.*f", it meant the total width of the output field is 5, including the decimal point and digits.

Then you would get 999.9 K/s. I bet you didn't test that one out.

As its yes for this, should I shift to 4.*f now, or use 5.*f.
I am asking this because for 4.*f I have to change the precision logic and if it stays 5.*f I have to add a column width, currently its 9 (for any kind of Disk IO rates), it will be then 10.

Then you would get 999.9 K/s. I bet you didn't test that one out.

I wanted to understand, is there any issue with printing 999.9 K/s? I am unable to get it...

For size units we usually just use 3 significant digits as extra precisions are less useful for UI display purpose. Add a decimal point and it becomes 4 characters of width. When the decimal points are not needed, i.e. for numbers in range 100 - 9999, four digits can be shown for the same width, but that's more of a special case.

The numbers jump from one unit to the next in power of 1024, and that was the reason of 3 significant digits.

perfect, I understood why to use 3 significant digits for UI displays. Will now fix this to make 4.*f.
Thanks.

AryanGitHub · 2024-10-21T04:25:55Z

The spacing for RDISK, WDISK and RW DISK is inconsistent.

Further more, DIOR, DIOW and DIORW would be even shorter and even consistent.

From the display side of things: There's a list of IEC unit prefixes for a reason. Please use that one to avoid code duplication.

Also, the space before B/s is intentional to ease reading 8B/s. Which can be hard to discern with some console fonts.

Should there be space for other values too, like when it is K/s or M/s, like 31.41 K/s? Rn I have space only for B/s.

Explorer09 · 2024-10-21T04:28:29Z

Also, the space before B/s is intentional to ease reading 8B/s. Which can be hard to discern with some console fonts.

Should there be space for other values too, like when it is K/s or M/s, like 31.41 K/s? Rn I have space only for B/s.

I can help answer that. Yes. Because otherwise the numbers can look misaligned, when there is an entire column of the numbers.

AryanGitHub · 2024-10-22T10:34:17Z

I made the rate to 4.*f and changed INF to infinity, as discussed.

Also ran a small independent test to ensure the printf decimal precision logic always prints 4 characters only. Link for the Test script.

AryanGitHub · 2024-10-27T18:06:36Z

Thank you, everyone, for the previous review. It helped me understand the project better and improved the application overall!
I've made the requested changes, as suggested, and would appreciate it if you could review them again. :)

This is the current view, made by the changes of this PR.

BenBE · 2024-11-18T08:28:23Z

Judging from the screenshot, I think marking the entries with exactly 0 B/s (probably <0.5 to account for floating point stuff) as shadowed might better emphasize the non-zero values.

Additionally, what would you think of highlighting the unit in the value the same as its value? With other columns the unit marker has the same color as the most-significant unit used.

AryanGitHub · 2024-11-21T14:01:04Z

Judging from the screenshot, I think marking the entries with exactly 0 B/s (probably <0.5 to account for floating point stuff) as shadowed might better emphasize the non-zero values.

Additionally, what would you think of highlighting the unit in the value the same as its value? With other columns the unit marker has the same color as the most-significant unit used.

@BenBE

Yes, I think providing a shadow colour for 0 B/s is good, as it emphasizes the non-zero values. because a good number of rows will be 0 B/s, at least for Fedora Linux-based OS, which I tested.

I believe the disk read speed in htop on Linux is never between 1 and 1024 bits/s. Instead, the speed was observed to be between 0 and 1 bits/s, but never between 1 and 1024. I think the same applies to disk write speeds as well.
To test this, I created a simple if block in linux/LinuxProcessTable.c and set a breakpoint inside the if block. I let gdb run for 4 hours, but it never caught the breakpoint.

// linux/LinuxProcessTable.c
// Debug
if (lp->io_rate_read_bps > 1 && lp->io_rate_read_bps <= 1024) {
printf("breakpoint");
}

I also believe that implementing colour coding for M/s, G/s, and T/s would be beneficial. In this image, the column is sorted based on these rate values, but that doesn't happen often.
To help the human eye observe sudden spikes in data rates across a significant number of rows, this would be helpful. Additionally, since the rate fluctuates very quickly and the read/write lasts for only a few seconds, this enhancement would be particularly useful.

Explorer09 · 2024-11-22T06:43:26Z

@AryanGitHub

I believe the disk read speed in htop on Linux is never between 1 and 1024 bits/s. Instead, the speed was observed to be between 0 and 1 bits/s, but never between 1 and 1024. I think the same applies to disk write speeds as well. To test this, I created a simple if block in linux/LinuxProcessTable.c and set a breakpoint inside the if block. I let gdb run for 4 hours, but it never caught the breakpoint.

I think less than 1 KiB/s may be observed if you set a long refresh rate for htop. For example one minute, so that the value would be "averaged" to less than 1 KiB.

Explorer09 reviewed Oct 19, 2024

View reviewed changes

BenBE added the enhancement Extension or improvement to existing feature label Oct 19, 2024

Shorten DISK READ/WRITE columns to DIOR/DIOW and DISK R/W to DIORW

56e9ee6

AryanGitHub force-pushed the main branch 3 times, most recently from 042354a to 66bdf58 Compare October 20, 2024 02:28

AryanGitHub force-pushed the main branch from 66bdf58 to 29546dd Compare October 20, 2024 08:35

AryanGitHub requested a review from Explorer09 October 20, 2024 16:09

Explorer09 reviewed Oct 21, 2024

View reviewed changes

Row.c Outdated Show resolved Hide resolved

Explorer09 reviewed Oct 21, 2024

View reviewed changes

AryanGitHub force-pushed the main branch 3 times, most recently from c326af3 to 720c3d0 Compare October 22, 2024 10:22

AryanGitHub requested a review from Explorer09 October 22, 2024 10:35

AryanGitHub added 2 commits October 27, 2024 19:12

Optimize DISK I/O Rate precision and add shading for clearer display

b443a37

Update I/O rate labels in man page

f3673f3

AryanGitHub force-pushed the main branch from 720c3d0 to f3673f3 Compare October 27, 2024 13:43

Adjust Row_printRate column width in PCPDynamicColumn_setupWidth

e0aaa54

AryanGitHub requested a review from BenBE November 17, 2024 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Readability of DISK READ/DISK WRITE Columns by Shortening Labels and Values #1557

Enhance Readability of DISK READ/DISK WRITE Columns by Shortening Labels and Values #1557

AryanGitHub commented Oct 19, 2024

AryanGitHub commented Oct 19, 2024

Explorer09 Oct 19, 2024

natoscott Oct 22, 2024

BenBE commented Oct 19, 2024

AryanGitHub commented Oct 20, 2024

Explorer09 commented Oct 20, 2024

Explorer09 Oct 21, 2024

AryanGitHub Oct 21, 2024

Explorer09 Oct 21, 2024

BenBE Oct 25, 2024

AryanGitHub Oct 25, 2024

Explorer09 Oct 21, 2024

AryanGitHub Oct 21, 2024

Explorer09 Oct 21, 2024

AryanGitHub Oct 21, 2024 •

edited

Loading

AryanGitHub Oct 21, 2024

Explorer09 Oct 21, 2024

AryanGitHub Oct 21, 2024

AryanGitHub commented Oct 21, 2024

Explorer09 commented Oct 21, 2024

AryanGitHub commented Oct 22, 2024 •

edited

Loading

AryanGitHub commented Oct 27, 2024 •

edited

Loading

BenBE commented Nov 18, 2024

AryanGitHub commented Nov 21, 2024

Explorer09 commented Nov 22, 2024

Enhance Readability of DISK READ/DISK WRITE Columns by Shortening Labels and Values #1557

Are you sure you want to change the base?

Enhance Readability of DISK READ/DISK WRITE Columns by Shortening Labels and Values #1557

Conversation

AryanGitHub commented Oct 19, 2024

Observations before addressing GitHub issue #991:

What has this PR changed?

AryanGitHub commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenBE commented Oct 19, 2024

AryanGitHub commented Oct 20, 2024

Explorer09 commented Oct 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AryanGitHub Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AryanGitHub commented Oct 21, 2024

Explorer09 commented Oct 21, 2024

AryanGitHub commented Oct 22, 2024 • edited Loading

AryanGitHub commented Oct 27, 2024 • edited Loading

BenBE commented Nov 18, 2024

AryanGitHub commented Nov 21, 2024

Explorer09 commented Nov 22, 2024

AryanGitHub Oct 21, 2024 •

edited

Loading

AryanGitHub commented Oct 22, 2024 •

edited

Loading

AryanGitHub commented Oct 27, 2024 •

edited

Loading