Skip to content
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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AryanGitHub
Copy link
Contributor

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
Copy link
Contributor Author

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:

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.

Row.c Outdated
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. precision takes the type int. You should set the data type properly.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

| 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 Outdated Show resolved Hide resolved
@BenBE BenBE added the enhancement Extension or improvement to existing feature label Oct 19, 2024
@BenBE
Copy link
Member

BenBE commented Oct 19, 2024

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 AryanGitHub force-pushed the main branch 3 times, most recently from 042354a to 66bdf58 Compare October 20, 2024 02:28
@AryanGitHub
Copy link
Contributor Author

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
Copy link
Contributor

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.)

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Row.c Outdated
}
}

int len = snprintf(buffer, sizeof(buffer), "%5.*f", precision, rate);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AryanGitHub AryanGitHub Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AryanGitHub
Copy link
Contributor Author

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
Copy link
Contributor

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 AryanGitHub force-pushed the main branch 3 times, most recently from c326af3 to 720c3d0 Compare October 22, 2024 10:22
@AryanGitHub
Copy link
Contributor Author

AryanGitHub commented Oct 22, 2024

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
Copy link
Contributor Author

AryanGitHub commented Oct 27, 2024

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. :)

image

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

@AryanGitHub AryanGitHub requested a review from BenBE November 17, 2024 23:09
@BenBE
Copy link
Member

BenBE commented Nov 18, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Extension or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants