Add configurable SI decimal vs IEC binary unit prefixes#1970
Conversation
|
Why do we need this feature? Also I have concerns about how the feature is implemented:
|
|
Nobody requested such a feature, what is the rationale @tenox7 ? |
73563ea to
5698430
Compare
I humbly believe that display of a transfer rate / bandwidth usage should be in SI decimal units (power of 10) and not IEC units (power of 2). In my personal use case output of htop network speed doesn't align with network equipment bandwidth usage measurement. Or for that matter with
Absolutely agree, however I did not see any other place where transfer rate is being displayed. Saying that I found and updated per process table rate. So this should be global now?
Are you referring to visual misalignment? I think this is user choice. If someone wants to display something in small units, large numbers and misalignment are expected. Most people sticks with auto. Forcing specific unit like MB/s is rather rare and specific.
Good question! I don't think these are achievable on a single machine at this time outside of L1/L2 cache speed. I have added TB/s and TiB/s for future use. |
|
Until there's a clarification on the requirements I don't think it worth it for you to revise the patches.
I have no personal objections on the SI vs. IEC unit switch. My concern is on the other part of the requirements that's either unclear or not worth it.
Grep the two keywords:
My suggestion was to remove the fixed unit options altogether, and leave only the IEC and SI unit options. That would simplify the UI. Because look at this: 5698430#diff-07aa5e06e3d4142e8e68a7c8ad5cc2da7a4843241640263649a7e24409d94792R317 Panel_add(super, (Object*) NumberItem_newByRef("IO rate unit (0-auto IEC 1-auto SI 2-KiB/s 3-MiB/s 4-GiB/s 5-TiB/s 6-KB/s 7-MB/s 8-GB/s 9-TB/s)", &(settings->ioRateUnit), 0, 0, LAST_IO_UNIT - 1));It can be expected in the future that there would be |
|
I think a IEC / SI switch could be acceptable, probably best to make an issue to discuss this first. I know this topic is a bit controversial among IT engineers. I think the bulk of the PR does not stand a chance of getting merged. |
|
FYI, btop does have the SI / IEC unit switch. |
5599d33 to
49acbd5
Compare
I have removed the non-auto sizing.
Okay I think this is addressed now.
Done! I have no problem with that. My only requirement is to have SI / IEC unit choice in order to align with other tools. |
Awesome. This is really all thats needed.
Actually I don't think it's controversial among IT engineers. Whats controversial is how this whole IEC units came to light and how it was adopted in software. Making it configurable addresses the issue.
Removed the non-auto scaled path. |
39324dd to
82fb340
Compare
|
|
||
| /* Takes number in kibibytes (base 1024). Prints 6 columns. */ | ||
| void Row_printKBytes(RichString* str, unsigned long long number, bool coloring); | ||
| /* Takes number in kibibytes (base 1024) or kilobytes (base 1000) per 'decimal'. Prints 6 columns. */ |
There was a problem hiding this comment.
I was thinking how the memory numbers are best represented when the SI base-10 units are enabled. In cases you didn't know, memory spaces of processes are allocated in pages, and a page is usually 4 KiB in size.
In other words, the smallest unit for memory allocation is always KiB, and forcing it to display as KB would distract users more than it helps. (Imagine, when a 64 KiB memory becomes 65 KB (65536 bytes) because of the base-10 conversion.)
My question is whether it's good to make a special case to display memory in KiB instead of KB, and only convert the unit to base-10 when the number is 100 MB or larger.
There was a problem hiding this comment.
That's how you end up with 3½" HD ("1.44 MB") floppy disks that were neither 1.44 MB nor 1.44 MiB, but 160 sectors on 18 tracks with 512 byte sectors, i.e. ~1.47 MB or 1.40 MiB if you do the math right …
There was a problem hiding this comment.
@BenBE My request was to keep the smallest unit for memory numbers to be KiB, even when the SI unit mode is set.
And no, I don't support the strange floppy-MB unit (1024000 bytes). What I mean is when the memory is converted from KiB to MB, it would be multiplied by 1024 / 1000000, that is, the real SI megabyte.
There was a problem hiding this comment.
Thought about it and I never really intended SI units for memory display — only rates, for data in flight. So memory now always stays IEC, and the SI toggle only applies to the I/O rates (disk/network meters + the per-process IO rate columns).
|
Just a note on organizing the enum: To be future-proof the most elegant solution would be to encode the LSb as IEC vs. SI, and use the other bits to denote the power to use, where 0 is "auto" and everything else is one step of the unit ladder. But that aside, I see little benefit in this overall feature except to confuse people (for which it would work great). Historically we chose to use IEC throughout due to (visual) space constraints where we could not reliably tell the unit to the user otherwise. And given htop is resource monitor for telling you about memory consumption it makes most sense to use the IEC units as a basis.. This even holds when talking about transfer rates from and to disks, as the OS operates on sectors (IEC-based) and cache pages (also IEC-based). The only place where this historically broke down somewhat is with network related metrics as these use SI units mostly. |
I agree that the IEC units should remain the default as the main thing htop users would want to monitor is the memory usage. For hard disks and SSDs, despite the fact that sector sizes are usually engineered to be a power of two, disk sizes are instead marketed in base-10 units. (A 1 TB SSD is 1000 GB of space, not 1024 GiB.) Fortunately monitoring hard disks is not the primary use case of htop, thus the unit problem affects only a niche group of people. Just for your information. |
|
It is a niche group of people, but the problem is that having only IEC units is sometimes incompatible with other monitoring tools, for example I would like to have option to switch to SI units to compare or correlate results from htop with network monitoring software which as @BenBE mentioned uses SI units, or Perhaps instead of menu configurable option we can add flag |
|
So what would take to merge this PR? Does anything need to be changed? |
|
For me, the only question remains is whether we need to special case the memory KiB units (this comment). I have no other objections. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughOverviewAdds a global, persistent setting to display I/O throughput using SI decimal units (base‑1000: KB/s, MB/s, etc.) instead of the existing IEC binary units (base‑1024: KiB/s, MiB/s). The option is exposed in Display Options and applied consistently to per-process I/O columns, disk meters, network meters, and PCP dynamic/process columns. What changed
Quality of implementation
Notable design trade-offs and risks
Recommendation / follow-upConsider unifying the formatting paths (IEC and SI) into a single helper to ensure identical field‑width/precision behavior between modes, or add tests/visual samples for boundary cases to validate alignment across common terminal widths. WalkthroughAdds a boolean Settings.decimalUnits (default false) with config read/write and a DisplayOptionsPanel checkbox. Introduces Meter_ioRateUnit(...) which formats bytes/sec and returns the unit suffix for SI ("B/s") or IEC ("iB/s"). Row_printRate gains a decimal flag and is refactored to scale using base-1000 or base-1024. DiskIOMeter and NetworkIOMeter call Meter_ioRateUnit, cache the returned suffix, and append it to text and RichString outputs. Linux and PCP process writers and dynamic columns pass settings->decimalUnits when printing rates. Poem
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a9972ce-0472-4b07-81a8-cedfe13dc8a5
📒 Files selected for processing (12)
DiskIOMeter.cDisplayOptionsPanel.cMeter.cMeter.hNetworkIOMeter.cRow.cRow.hSettings.cSettings.hlinux/LinuxProcess.cpcp/PCPDynamicColumn.cpcp/PCPProcess.c
| static const char prefixes[] = { 'B', 'K', 'M', 'G', 'T', 'P' }; | ||
| const int colors[ARRAYSIZE(prefixes)] = { | ||
| baseColor, baseColor, megabytesColor, | ||
| largeNumberColor, largeNumberColor, largeNumberColor | ||
| }; | ||
|
|
||
| const double base = decimal ? ONE_DECIMAL_K : ONE_K; | ||
| size_t i = 0; | ||
| double scaled = rate; | ||
| while (scaled >= base && i + 1 < ARRAYSIZE(prefixes)) { | ||
| scaled /= base; | ||
| i++; | ||
| } | ||
|
|
||
| int color = (rate < 0.005) ? shadowColor : colors[i]; | ||
| int len = xSnprintf(buffer, sizeof(buffer), "%7.2f %c/s ", scaled, prefixes[i]); | ||
| RichString_appendnAscii(str, color, buffer, len); |
There was a problem hiding this comment.
I would appreciate if you make this part of the change a separate commit. In particular the reduction of code size by changing the series of if-conditionals into a loop (without the base-10 or base-2 switch).
Also, Since you've transformed this into a loop, it's helpful to use unitPrefixes array in XUtils.h. That way you can get sizes up to quettabytes and quebibytes with little cost.
There was a problem hiding this comment.
Done — pulled the loop refactor out into its own commit (no unit switch), and it now uses the unitPrefixes table so it scales up to Q. The SI/IEC feature sits on top.
Replace the if-else unit ladder with a loop over the shared unitPrefixes table (XUtils.h). This extends the range up to quetta and drops the duplicated snprintf calls. Displayed output is unchanged for realistic rates.
Adds a Display Options toggle (and decimal_units config key) to show I/O rates in SI decimal units (KB/s, MB/s) instead of IEC binary (KiB/s, MiB/s). Affects the disk/network I/O meters and the per-process IO rate columns; memory stays IEC.
|
Is there anything outstanding on this PR? Can I get an approval? Thanks! |
|
@tenox7 This PR has not been forgotten. Just had little time the past days and also there's internally still an open question. Of the two commits I'm inclined to merge at least the first one (might even happen out-of-line); the second one still is under consideration. |
when displaying network or disk io meters allow to choose whether to use IEC or SI unit prefixes