Split DiskIOMeter into disk IO time and rate sub-meters #1763

Explorer09 · 2025-08-31T18:28:52Z

Split the DiskIOMeter into two, DiskIOTimeMeter and DiskIORateMeter, showing the time and the rate separately.

The name DiskIOMeter is retained but now shows the combined view of time and rate, similar to MemorySwapMeter.

This PR depends on #1721, which introduces an isPercentChart property to meters. Note this isPercentChart property is different between sub-meters, and it could explain the reason why the split is worth it.

DiskIOMeter.c

BenBE · 2025-10-30T09:27:11Z

What's currently still open for this PR to become ready?

Explorer09 · 2025-10-30T11:22:46Z

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.

I also intended that DiskIOTimeMeter will include the number of disks in the display, so that the greater-than-100% disk time can be understood without reading more context or manual, however I don't have an idea on how to implement that well. I think I can postpone that part into a separate PR. I just leave a reminder here.

The new function is named DiskIOUpdateCache(). Allows code reuse. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>

No changes in behavior. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>

BenBE · 2025-10-30T11:59:32Z

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.

Explorer09 · 2025-10-30T12:23:31Z

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

BenBE · 2025-10-31T01:03:58Z

DiskIOMeter.c

 };

+static const int DiskIOTimeMeter_attributes[] = {
+   METER_VALUE_NOTICE


Suggested change

METER_VALUE_NOTICE

METER_VALUE_NOTICE,

BenBE · 2025-10-31T01:07:05Z

DiskIOMeter.c

+   assert(data->diskIORateMeter->draw);
+   data->diskIORateMeter->draw(data->diskIORateMeter, x, y, colwidth);
+   assert(data->diskIOTimeMeter->draw);
+   data->diskIOTimeMeter->draw(data->diskIOTimeMeter, x + colwidth + diff, y, colwidth);


While initializing or stale, this looks kinda strange (in text mode).

The IO Time meter itself could benefit from actually labeling the value it shows in text mode.

What do you mean when you say it looks strange?

Basically you have like:

Dsk: Stale ... Dsk: Stale ...

where you somewhat break the illusion of this being one meter …

I think:

Dsk: Stale ...

should suffice.

Similar, in the text mode, having values from both meters right next to each other

Dsk: read: 42.23 KiB/s, write: 666.69 MiB/s, usage: 1337%

compare to current:

Dsk: read: 42.23 KiB/s, write: 666.69 MiB/s Dsk: 1337%

would be somewhat more intuitive.

That means merging the two strings in the Text meter display. It's a bit out of scope of my PR, but I know it's a good feature. I'm not sure how to make it, though.

BenBE · 2025-10-31T01:08:41Z

DiskIOMeter.c

+   DiskIOMeterData* data = this->meterData;
+
+   if (!data) {
+      data = this->meterData = xCalloc(1, sizeof(DiskIOMeterData));
+   }


Maybe avoid this double assignment?

Suggested change

DiskIOMeterData* data = this->meterData;

if (!data) {

data = this->meterData = xCalloc(1, sizeof(DiskIOMeterData));

}

if (!this->meterData) {

this->meterData = xCalloc(1, sizeof(DiskIOMeterData));

}

DiskIOMeterData* data = this->meterData;

BenBE · 2025-10-31T01:10:42Z

DiskIOMeter.c

+   assert(data->diskIORateMeter->draw);
+   data->diskIORateMeter->draw(data->diskIORateMeter, x, y, colwidth);
+   assert(data->diskIOTimeMeter->draw);
+   data->diskIOTimeMeter->draw(data->diskIOTimeMeter, x + colwidth + diff, y, colwidth);


There's Meter_drawFn that could be used here.

Meter_drawFn refers to the (custom) draw function associated with the meter class, not the meter mode. So no, they are not the same.

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 <explorer09@gmail.com>

The new meter is a combined display of DiskIORate and DiskIOTime. The mechanism is similar to MemorySwapMeter. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>

Explorer09 · 2025-11-02T16:33:38Z

@BenBE On the topic of avoiding double assignment in code style, I found there are three other files that had this "double assignment" syntax when initializing data. Not sure if you want these to be changed as well, but I'm reluctant to file a new PR for this minor style change:

--- a/CPUMeter.c
+++ b/CPUMeter.c
@@ -241,9 +241,10 @@ static void CPUMeterCommonInit(Meter* this) {
 
    CPUMeterData* data = this->meterData;
    if (!data) {
-      data = this->meterData = xMalloc(sizeof(CPUMeterData));
+      data = xMalloc(sizeof(CPUMeterData));
       data->cpus = this->host->existingCPUs;
       data->meters = count ? xCalloc(count, sizeof(Meter*)) : NULL;
+      this->meterData = data;
    }
 
    Meter** meters = data->meters;
--- a/Machine.c
+++ b/Machine.c
@@ -79,9 +79,11 @@ void Machine_populateTablesFromSettings(Machine* this, Settings* settings, Table
 
    for (size_t i = 0; i < settings->nScreens; i++) {
       ScreenSettings* ss = settings->screens[i];
+
+      if (!ss->table)
+         ss->table = processTable;
+
       Table* table = ss->table;
-      if (!table)
-         table = ss->table = processTable;
       if (i == 0)
          this->activeTable = table;
 
--- a/MemorySwapMeter.c
+++ b/MemorySwapMeter.c
@@ -47,11 +47,10 @@ static void MemorySwapMeter_draw(Meter* this, int x, int y, int w) {
 }
 
 static void MemorySwapMeter_init(Meter* this) {
-   MemorySwapMeterData* data = this->meterData;
+   if (!this->meterData)
+      this->meterData = xCalloc(1, sizeof(MemorySwapMeterData));
 
-   if (!data) {
-      data = this->meterData = xCalloc(1, sizeof(MemorySwapMeterData));
-   }
+   MemorySwapMeterData* data = this->meterData;
 
    if (!data->memoryMeter)
       data->memoryMeter = Meter_new(this->host, 0, (const MeterClass*) Class(MemoryMeter));

Based on a patch in #1763 (comment) Co-authored-by: Kang-Che Sung <explorer09@gmail.com>

BenBE · 2025-11-02T16:52:49Z

@Explorer09 Patch applied on main with slight modifications.

Explorer09 mentioned this pull request Aug 31, 2025

Dis (disk usage) graph appears flat with root-on-ZFS on Linux #1762

Open

BenBE added the new feature Completely new feature label Sep 1, 2025

Explorer09 mentioned this pull request Sep 13, 2025

Two-channel graph (Graph2) meter style #1415

Open

Explorer09 force-pushed the disk-io-meter-split branch 2 times, most recently from 68ccd1f to a718df6 Compare October 10, 2025 05:41

Explorer09 force-pushed the disk-io-meter-split branch 2 times, most recently from 1b33b9b to 8b5bf63 Compare October 15, 2025 06:50

Explorer09 force-pushed the disk-io-meter-split branch 3 times, most recently from 26ea738 to eecc641 Compare October 28, 2025 12:33

BenBE reviewed Oct 30, 2025

View reviewed changes

DiskIOMeter.c Outdated Show resolved Hide resolved

DiskIOMeter.c Show resolved Hide resolved

Explorer09 added 2 commits October 30, 2025 19:34

DiskIOMeter: Move cache update code to a new function

a124dde

The new function is named DiskIOUpdateCache(). Allows code reuse. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>

DiskIOMeter: Adjust code indent and formatting

c9b997d

No changes in behavior. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>

Explorer09 force-pushed the disk-io-meter-split branch from eecc641 to 1ec2f55 Compare October 30, 2025 20:13

Explorer09 marked this pull request as ready for review October 30, 2025 20:15

BenBE reviewed Oct 31, 2025

View reviewed changes

Explorer09 added 2 commits October 31, 2025 21:38

Rework DiskIOMeter into a combined display of 2 sub-meters

a5d1e6d

The new meter is a combined display of DiskIORate and DiskIOTime. The mechanism is similar to MemorySwapMeter. Signed-off-by: Kang-Che Sung <explorer09@gmail.com>

Explorer09 force-pushed the disk-io-meter-split branch from 1ec2f55 to a5d1e6d Compare October 31, 2025 13:40

BenBE added a commit that referenced this pull request Nov 2, 2025

Avoid double assignment in code style

3df2921

Based on a patch in #1763 (comment) Co-authored-by: Kang-Che Sung <explorer09@gmail.com>

Uh oh!

Split DiskIOMeter into disk IO time and rate sub-meters #1763

Are you sure you want to change the base?

Split DiskIOMeter into disk IO time and rate sub-meters #1763

Uh oh!

Conversation

Explorer09 commented Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BenBE commented Oct 30, 2025

Uh oh!

Explorer09 commented Oct 30, 2025

Uh oh!

BenBE commented Oct 30, 2025

Uh oh!

Explorer09 commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Explorer09 commented Nov 2, 2025

Uh oh!

BenBE commented Nov 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Explorer09 commented Aug 31, 2025 •

edited

Loading