Split DiskIOMeter into disk IO time and rate sub-meters #1763
Conversation
Explorer09
force-pushed
the
disk-io-meter-split
branch
2 times, most recently
from
68ccd1f to
a718df6
Compare
Explorer09
force-pushed
the
disk-io-meter-split
branch
2 times, most recently
from
1b33b9b to
8b5bf63
Compare
Explorer09
force-pushed
the
disk-io-meter-split
branch
3 times, most recently
from
26ea738 to
eecc641
Compare
DiskIOMeter.c
Outdated
| if (status == RATESTATUS_NODATA) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "no data"); | ||
| return; | ||
| } | ||
| if (status == RATESTATUS_INIT) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "init"); | ||
| return; | ||
| } | ||
| if (status == RATESTATUS_STALE) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "stale"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Any reason for not using a switch/case like in the function below?
There was a problem hiding this comment.
It's to prevent too much style changes to the original code. Not a big deal to switch to switch-case though. (Pun not intended)
| const MeterClass DiskIOMeter_class = { | ||
| .super = { | ||
| .extends = Class(Meter), | ||
| .delete = Meter_delete, | ||
| }, | ||
| .updateValues = DiskIOMeter_updateValues, | ||
| .defaultMode = TEXT_METERMODE, | ||
| .supportedModes = METERMODE_DEFAULT_SUPPORTED, | ||
| .isMultiColumn = true, | ||
| .name = "DiskIO", | ||
| .uiName = "Disk IO", | ||
| .caption = "Disk IO: " | ||
| .description = "Disk IO time & rate combined display", | ||
| .caption = "Dsk: ", | ||
| .draw = DiskIOMeter_draw, | ||
| .init = DiskIOMeter_init, | ||
| .updateMode = DiskIOMeter_updateMode, | ||
| .done = DiskIOMeter_done | ||
| }; |
There was a problem hiding this comment.
Having this moved after the rate-based meter from where it was before introduces quite a bit of noise in this commit. Try to keep the commits minimal without unnecessarily re-ordering code. I had to resort to git lol -p --color-moved --color-moved-ws=allow-indentation-change main... to get at least partially sensible diffs; and those were still PITA to review. ;-)
Similar goes for the MeterClass constants, where putting things in the order DiskIOMeter, DiskIORateMeter, DiskIOTimeMeter would have produced a much shorter/cleaner diff.
There was a problem hiding this comment.
I thought DiskIOMeter goes after DiskIORateMeter and DiskIOTimeMeter because the former depends on the functionality of the latter. If that makes the diff messy, I can split the reordering into a separate commit. Will that be acceptable?
There was a problem hiding this comment.
I think just having the meter classes in the same order s previously with the new meters added below is fine. Moving code always teds to get messy no matter if you split it or not. So if you can avoid moving code (and here keeping things ordered "Ye Old Way"™ is fine), just keep it that way.
NB: The first 3 commits are fine. Only the last commit may benefit from avoiding to move code.
There was a problem hiding this comment.
When I try to rework the last commit (just now), I realised the problem wasn't on the code reordering, but that you failed to recognize the old DiskIOMeter_ functions was renamed to DiskIOTimeMeter_ functions intentionally.
The "DiskIOMeter" name was kept for compatibility, but the new "DiskIOMeter" in this PR is, actually, entirely new. Users with older htop config files that use the "DiskIOMeter" will new see two bars with the new code.
I'm not sure what's the best way to fix the confusion though. Maybe I could introduce both DiskIORate and DiskIOTime first (DiskIOTimeMeter be mostly duplicate of the old DiskIOMeter code). Then rework DiskIOMeter to the now-seen, final form.
|
What's currently still open for this PR to become ready? |
I originally made this PR as a "proof of concept" and to grab early comments about the meters' look and functions.
|
The new function is named DiskIOUpdateCache(). Allows code reuse. Signed-off-by: Kang-Che Sung <[email protected]>
No changes in behavior. Signed-off-by: Kang-Che Sung <[email protected]>
|
Just did a quick check with the design and I think the combined meter should show IO rates left, IO Time used on the right. Else you get quite a big gap with just the percentage shown in text mode. |
Makes sense. Perhaps another good reason is that people might look at the I/O rate graph more often than the time/utilisation percentage. (Besides, the I/O rate graph can support two channels while the percentage graph cannot.) |
The two meters are split from DiskIOMeter and they allow separate display of disk read & write rates and the busy time percentage, including drawing the data as separate bars and graphs. The old DiskIOMeter is kept for backward compatibility. It will be reworked in the next commit. Note that DiskIORateMeter and DiskIOTimeMeter have different 'isPercentChart' values. Signed-off-by: Kang-Che Sung <[email protected]>
The new meter is a combined display of DiskIORate and DiskIOTime. The mechanism is similar to MemorySwapMeter. Signed-off-by: Kang-Che Sung <[email protected]>
Explorer09
force-pushed
the
disk-io-meter-split
branch
from
eecc641 to
1ec2f55
Compare
2 participants
Split the
DiskIOMeterinto two,DiskIOTimeMeterandDiskIORateMeter, showing the time and the rate separately.The name
DiskIOMeteris retained but now shows the combined view of time and rate, similar toMemorySwapMeter.This PR depends on #1721, which introduces an
isPercentChartproperty to meters. Note thisisPercentChartproperty is different between sub-meters, and it could explain the reason why the split is worth it.