Merging all the points related to calculating time in one place by smalinux · Pull Request #574 · htop-dev/htop

smalinux · 2021-03-22T22:35:25Z

The end goal is to consolidate all the points in htop that can only work in
live-only mode today, so that will be able to inject PCP archive mode and have
a chance at it working.
The biggest problem we've got at this moment is all the places that are
independently asking the kernel to 'give me the time right now'.
Each of those needs to be audited and ultimately changed to allow platforms to
manage their own idea of time.
So, all the calls to gettimeofday(2) and time(2) are potential problems.
Ultimately I want to get these down to just one or two.

natoscott · 2021-03-22T23:40:59Z

@smalinux looks like there's an inconsistency between a header and associated function call (LinuxProcessList_updateCPUcount?) - possibly merge fallout. Its causing CI issues currently.

BenBE · 2021-03-22T23:47:55Z

Also AFAIR there is a timestamp passed around for each update already. Please check if there are any functions that don't currently already use that timestamp for their timing needs.

a3f

Having to list multiple bullet points is a good indicator a commit should be split up into a number of commits where each message just describes a single issue.

You can still give an overview in the cover-letter (here, PR caption), but the individual commits should focus on the backstory of the changeset at hand.

PR also seems to introduce a regression.

a3f · 2021-03-22T23:21:16Z


 #if defined(HAVE_CLOCK_GETTIME)

-   return clock_gettime(CLOCK_MONOTONIC, tp);


This change should be in a separate commit.

a3f · 2021-03-22T23:23:05Z

 }
+
+int xClock_gettime (clockid_t __clock_id, struct timespec *__tp) {
+   return clock_gettime (__clock_id, __tp);


What's the added value of having this wrapper? Also __ prefixed identifiers are reserved for the C implementation and/or denote internal stuff. You could just call it clock_id and tp directly here.

a3f · 2021-03-22T23:27:01Z

 ssize_t xReadfile(const char* pathname, void* buffer, size_t count);
 ssize_t xReadfileat(openat_arg_t dirfd, const char* pathname, void* buffer, size_t count);

+int xClock_gettime (clockid_t __clock_id, struct timespec *__tp);


Seems to me this header has a dependency on clockid_t being defined externally. You should probably #include <time.h> above (provided you keep xClock_gettime).

Nitpick: coding style seems to not use a space before parentheses.

a3f · 2021-03-22T23:46:09Z

+   gettimeofday(&tv, NULL);
+
+   pl->timestamp     = tv;
+   pl->timestampMs   = (uint64_t)tv.tv_sec * 1000 + (uint64_t)tv.tv_usec / 1000;


Is it worth it to precompute this on every updateTime even if we don't use it? Wouldn't it be better to just store timestamp and compute timestampMs if needed?

I think it is OK to compute the millisecond value here once every cycle.

a3f · 2021-03-22T23:47:01Z

   LinuxProcessList_scanMemoryInfo(super);
   LinuxProcessList_scanHugePages(this);
   LinuxProcessList_scanZfsArcstats(this);
+   LinuxProcessList_updateCPUcount(this);


copy-paste error? Shouldn't this be LinuxProcessList_updateTime?

a3f · 2021-03-22T23:50:08Z

-   struct timeval tv;
-   gettimeofday(&tv, NULL);
-   unsigned long long now = tv.tv_sec * 1000ULL + tv.tv_usec / 1000ULL;
+   unsigned long long now = super->timestamp.tv_sec * 1000ULL + super->timestamp.tv_usec / 1000ULL;


Given that it's repeated quite a few times, a prerequisite patch adding a tv_to_msec() might be useful.

It exists in two different places, each one of them has its own calculation, the rest of the duplicate places are removed.

I can't follow. I still see it here and in LinuxProcessList_updateTime.

Can't now be replaced by pl->timestampMs?

If unsigned long long now == uint64_t pl->timestampMs? Yes :) (and I 'think' it-is)

smalinux · 2021-03-23T07:13:51Z

Also AFAIR there is a timestamp passed around for each update already. Please check if there are any functions that don't currently already use that timestamp for their timing needs.

I used $ git grep "gettimeofday", $ git grep "time.h" and $ git grep "timestamp" so I 'think' all places that used timestamp exists in one place (my changes)

smalinux · 2021-03-23T07:14:07Z

I retreated all 'xClock' related changes, I think it will not affect PCP-related work, so I left clock_gettime(2) as-is at this stage.

Also, I retreated from removing #include <time.h> although did not use it, because other Platforms may depend on this, I think it may be causes problems.

BenBE · 2021-03-23T08:30:04Z

The CI still reports one issue:

	clang -DHAVE_CONFIG_H -I.  -DNDEBUG -DGCC_PRINTF -DGCC_SCANF -D_FORTIFY_SOURCE=2 -Wall -Wcast-align -Wcast-qual -Wextra -Wfloat-equal -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Wnull-dereference -Werror -pedantic -std=c99 -D_XOPEN_SOURCE_EXTENDED -DSYSCONFDIR="\"/usr/local/etc\"" -I"./darwin" -g -O2 -MT BatteryMeter.o -MD -MP -MF $depbase.Tpo -c -o BatteryMeter.o BatteryMeter.c &&\
	mv -f $depbase.Tpo $depbase.Po
BatteryMeter.c:28:40: error: too many arguments to function call, expected 2, have 3
   Platform_getBattery(this, &percent, &isOnAC);
   ~~~~~~~~~~~~~~~~~~~                 ^~~~~~~
./darwin/Platform.h:70:1: note: 'Platform_getBattery' declared here
void Platform_getBattery(double *percent, ACPresence *isOnAC);
^
1 error generated.
make[1]: *** [BatteryMeter.o] Error 1

cgzones · 2021-03-24T18:52:31Z

+   struct timeval previous;   /* time of the previous sample */
+   uint64_t timestampMs;   /* current time in milliseconds */
+   uint64_t previousMs;		/* previous time in milliseconds */
+   uint64_t intervalMs;		/* milliseconds between 2 samples */


The fields previous, previousMs and intervalMs are currently only assigned, but never used.
I suppose they are getting used in future additions?

Also, to please my eyes, please align the comments.

The fields previous, previousMs and intervalMs are currently only assigned, but never used.
I suppose they are getting used in future additions?

Yes, should I leave them as-is? or remove them for now?

That's already wrong with the current code flow, as the process update updates some fields for the task counts displayed in the header …

I've opened #587 to ensure this bug fix goes in as a standalone fix, as discussed on IRC.

cgzones · 2021-03-24T18:56:04Z

+   gettimeofday(&tv, NULL);
+
+   pl->timestamp     = tv;
+   pl->timestampMs   = (uint64_t)tv.tv_sec * 1000 + (uint64_t)tv.tv_usec / 1000;


I think it is OK to compute the millisecond value here once every cycle.

cgzones · 2021-03-24T18:56:27Z

+   pl->timestampMs   = (uint64_t)tv.tv_sec * 1000 + (uint64_t)tv.tv_usec / 1000;
+   pl->intervalMs    = pl->timestampMs - pl->previousMs;
+   pl->previousMs    = cached_last_update;
+   cached_last_update = pl->timestampMs;


Please align all the assignments

cgzones · 2021-03-24T18:57:24Z

   LinuxProcessList_scanMemoryInfo(super);
   LinuxProcessList_scanHugePages(this);
   LinuxProcessList_scanZfsArcstats(this);
+   LinuxProcessList_updateTime(this);


Maybe insert this function call as first action, so every function can assume the data is up-to-date.

There's still a separate gettimeofday call in checkRecalculation ScreenManager.c too - isn't that the very first place we could capture the 'current time of sampling' (perhaps via a new routine named like Platform_getCurrentTime?) Then we'd not need a later LinuxProcessList_updateTime call.

@natoscott Excellent, I can't call Platform_getCurrentTime directly from ScreenManager.c (this causes problems with CI and other OSs)
So, I made ProcessList_getCurrentTime.
I think this's optimal because our time-related variables exist within ProcessList struct (ProcessList.h) and my new function in ProcessList.c. I could call it directly from there also.

@smalinux I'll send some code through shortly - we should be able to call platform code from ScreenManager.c if we need to. I'll need to reproduce that CI failure though to see the exact issue.

cgzones · 2021-03-24T18:58:24Z

-   struct timeval tv;
-   gettimeofday(&tv, NULL);
-   unsigned long long now = tv.tv_sec * 1000ULL + tv.tv_usec / 1000ULL;
+   unsigned long long now = super->timestamp.tv_sec * 1000ULL + super->timestamp.tv_usec / 1000ULL;


Can't now be replaced by pl->timestampMs?

cgzones · 2021-03-25T13:53:57Z

I think we need to reorder the updates in Screenmanager.c:

diff --git a/ScreenManager.c b/ScreenManager.c
index c4fbfee..23a635f 100644
--- a/ScreenManager.c
+++ b/ScreenManager.c
@@ -105,13 +105,13 @@ static void checkRecalculation(ScreenManager* this, double* oldTime, int* sortTi
 
    if (*rescan) {
       *oldTime = newTime;
-      // always update header, especially to avoid gaps in graph meters
-      Header_updateData(this->header);
       ProcessList_scan(pl, this->state->pauseProcessUpdate);
       if (!this->state->pauseProcessUpdate && (*sortTimeout == 0 || this->settings->treeView)) {
          ProcessList_sort(pl);
          *sortTimeout = 1;
       }
+      // always update header, especially to avoid gaps in graph meters
+      Header_updateData(this->header);
       *redraw = true;
    }
    if (*redraw) {

else the NetworkIO and DiskIO meters do not see the updated timestamps.

BenBE · 2021-03-25T20:07:04Z

That's already wrong with the current code flow, as the process update updates some fields for the task counts displayed in the header …

natoscott · 2021-03-29T04:45:59Z

I retreated all 'xClock' related changes, I think it will not affect PCP-related work, so I left clock_gettime(2) as-is at this stage.

We'll need to come back to it later - but for now this PR is looking like a good standalone improvement (independent of the needs with historical reporting via PCP).

Refactor the sample time code to make one call to gettimeofday (aka the realtime clock in clock_gettime, when available) and one to the monotonic clock. Stores each in more appropriately named ProcessList fields for ready access when needed. Every platform gets the opportunity to provide their own clock code, and the existing Mac OS X specific code is moved below darwin instead of in Compat. A couple of leftover time(2) calls are converted to use these ProcessList fields as well, instead of yet again sampling the system clock. Related to htop-dev#574

natoscott · 2021-03-30T04:59:34Z

@smalinux I've created a branch with a series of updates: https://github.com/natoscott/htop/tree/smalinux-CtrTime

Could you review and merge with your branch if it looks OK? This adds code to handle both realtime (gettimeofday) and monotonic clock accesses, with a single realtime clock access at the start of sampling (i.e. in ScreenManager.c).

Refactor the sample time code to make one call to gettimeofday (aka the realtime clock in clock_gettime, when available) and one to the monotonic clock. Stores each in more appropriately named ProcessList fields for ready access when needed. Every platform gets the opportunity to provide their own clock code, and the existing Mac OS X specific code is moved below darwin instead of in Compat. A couple of leftover time(2) calls are converted to use these ProcessList fields as well, instead of yet again sampling the system clock. Related to htop-dev#574

The end goal is to consolidate all the points in htop that can only work in live-only mode today, so that will be able to inject PCP archive mode and have a chance at it working. The biggest problem we've got at this moment is all the places that are independently asking the kernel to 'give me the time right now'. Each of those needs to be audited and ultimately changed to allow platforms to manage their own idea of time. So, all the calls to gettimeofday(2) and time(2) are potential problems. Ultimately I want to get these down to just one or two. Related to htop-dev#574

Refactor the sample time code to make one call to gettimeofday (aka the realtime clock in clock_gettime, when available) and one to the monotonic clock. Stores each in more appropriately named ProcessList fields for ready access when needed. Every platform gets the opportunity to provide their own clock code, and the existing Mac OS X specific code is moved below darwin instead of in Compat. A couple of leftover time(2) calls are converted to use these ProcessList fields as well, instead of yet again sampling the system clock. Related to htop-dev#574

The end goal is to consolidate all the points in htop that can only work in live-only mode today, so that will be able to inject PCP archive mode and have a chance at it working. The biggest problem we've got at this moment is all the places that are independently asking the kernel to 'give me the time right now'. Each of those needs to be audited and ultimately changed to allow platforms to manage their own idea of time. So, all the calls to gettimeofday(2) and time(2) are potential problems. Ultimately I want to get these down to just one or two. Related to htop-dev#574

Refactor the sample time code to make one call to gettimeofday (aka the realtime clock in clock_gettime, when available) and one to the monotonic clock. Stores each in more appropriately named ProcessList fields for ready access when needed. Every platform gets the opportunity to provide their own clock code, and the existing Mac OS X specific code is moved below darwin instead of in Compat. A couple of leftover time(2) calls are converted to use these ProcessList fields as well, instead of yet again sampling the system clock. Related to htop-dev#574

BenBE

Mostly LGTM. Not yet happy with the rename of seenTs and tombTs.

BenBE · 2021-03-30T17:06:25Z

-   time_t seenTs;
-   time_t tombTs;
+   uint64_t seenMs;
+   uint64_t tombMs;


Why rename these fields?

Mainly cos I wanted it clear in my head that these timestamps are in milliseconds, and it follows the convention being established in ProcessList.h - but, let's change it back if its confusing.

[...] let's change it back if its confusing.

@smalinux are you happy to do the rename? I'm about to go on holidays for a few days. :)

@BenBE oh, also, time_t is typically a time-in-seconds (since the epoch) - hence the change in type (i.e. that part should stay as-is IMO).

@smalinux are you happy to do the rename?

@natoscott Yes, certainly, happy holiday :)

I'm both fine with the type change (that should stay) and also with the Ms marker at the end. It's just a bit unfortunate to drop the mid-part Ts altogether. Maybe seenStampMs/tombStampMs?

@BenBE @smalinux that sounds good to me

BenBE points out that some header meters use values calculated during process scanning - make sure we scan processes first in order that current values are displayed. Related to htop-dev#574

BenBE points out that some header meters use values calculated during process scanning - make sure we scan processes first in order that current values are displayed. Related to #574

BenBE points out that some header meters use values calculated during process scanning - make sure we scan processes first in order that current values are displayed. Related to htop-dev#574

The end goal is to consolidate all the points in htop that can only work in live-only mode today, so that will be able to inject PCP archive mode and have a chance at it working. The biggest problem we've got at this moment is all the places that are independently asking the kernel to 'give me the time right now'. Each of those needs to be audited and ultimately changed to allow platforms to manage their own idea of time. So, all the calls to gettimeofday(2) and time(2) are potential problems. Ultimately I want to get these down to just one or two. Related to htop-dev#574

Refactor the sample time code to make one call to gettimeofday (aka the realtime clock in clock_gettime, when available) and one to the monotonic clock. Stores each in more appropriately named ProcessList fields for ready access when needed. Every platform gets the opportunity to provide their own clock code, and the existing Mac OS X specific code is moved below darwin instead of in Compat. A couple of leftover time(2) calls are converted to use these ProcessList fields as well, instead of yet again sampling the system clock. Related to htop-dev#574

The end goal is to consolidate all the points in htop that can only work in live-only mode today, so that will be able to inject PCP archive mode and have a chance at it working. The biggest problem we've got at this moment is all the places that are independently asking the kernel to 'give me the time right now'. Each of those needs to be audited and ultimately changed to allow platforms to manage their own idea of time. So, all the calls to gettimeofday(2) and time(2) are potential problems. Ultimately I want to get these down to just one or two. Related to htop-dev#574

Refactor the sample time code to make one call to gettimeofday (aka the realtime clock in clock_gettime, when available) and one to the monotonic clock. Stores each in more appropriately named ProcessList fields for ready access when needed. Every platform gets the opportunity to provide their own clock code, and the existing Mac OS X specific code is moved below darwin instead of in Compat. A couple of leftover time(2) calls are converted to use these ProcessList fields as well, instead of yet again sampling the system clock. Related to htop-dev#574

BenBE

LGTM.

The end goal is to consolidate all the points in htop that can only work in live-only mode today, so that will be able to inject PCP archive mode and have a chance at it working. The biggest problem we've got at this moment is all the places that are independently asking the kernel to 'give me the time right now'. Each of those needs to be audited and ultimately changed to allow platforms to manage their own idea of time. So, all the calls to gettimeofday(2) and time(2) are potential problems. Ultimately I want to get these down to just one or two. Related to htop-dev#574

Refactor the sample time code to make one call to gettimeofday (aka the realtime clock in clock_gettime, when available) and one to the monotonic clock. Stores each in more appropriately named ProcessList fields for ready access when needed. Every platform gets the opportunity to provide their own clock code, and the existing Mac OS X specific code is moved below darwin instead of in Compat. A couple of leftover time(2) calls are converted to use these ProcessList fields as well, instead of yet again sampling the system clock. Related to htop-dev#574

Related to htop-dev#574

Related to #574

smalinux force-pushed the CtrTime branch from ec05a2b to f83a095 Compare March 22, 2021 22:54

a3f suggested changes Mar 22, 2021

View reviewed changes

BenBE added the PCP PCP related issues label Mar 23, 2021

smalinux force-pushed the CtrTime branch from f83a095 to fd7539d Compare March 23, 2021 06:47

smalinux force-pushed the CtrTime branch from fd7539d to 9973f26 Compare March 23, 2021 09:10

cgzones reviewed Mar 24, 2021

View reviewed changes

smalinux force-pushed the CtrTime branch from 9973f26 to 1f6c940 Compare March 25, 2021 02:20

smalinux force-pushed the CtrTime branch 2 times, most recently from f227e01 to 2c634a5 Compare March 29, 2021 15:11

smalinux force-pushed the CtrTime branch from 2c634a5 to 462efa4 Compare March 30, 2021 14:39

smalinux force-pushed the CtrTime branch from 462efa4 to 435f60f Compare March 30, 2021 14:57

smalinux force-pushed the CtrTime branch from 435f60f to d194ccc Compare March 30, 2021 15:05

BenBE reviewed Mar 30, 2021

View reviewed changes

natoscott mentioned this pull request Mar 31, 2021

Reorder the header update and process scanning #587

Merged

smalinux force-pushed the CtrTime branch from d194ccc to 8d7f196 Compare March 31, 2021 08:55

smalinux force-pushed the CtrTime branch from 8d7f196 to f082e54 Compare March 31, 2021 16:02

BenBE reviewed Mar 31, 2021

View reviewed changes

Comment thread linux/LinuxProcessList.c Outdated

smalinux force-pushed the CtrTime branch from f082e54 to 8586edc Compare March 31, 2021 16:51

BenBE approved these changes Mar 31, 2021

View reviewed changes

smalinux and others added 2 commits April 5, 2021 23:40

smalinux force-pushed the CtrTime branch from 8586edc to 356488a Compare April 5, 2021 21:41

natoscott merged commit f3a37f9 into htop-dev:master Apr 7, 2021

natoscott added a commit to natoscott/htop that referenced this pull request Apr 8, 2021

Add time handling interfaces for the pcp platform

3fc589c

Related to htop-dev#574

natoscott added a commit to natoscott/htop that referenced this pull request Apr 14, 2021

Add time handling interfaces for the pcp platform

199e965

Related to htop-dev#574

natoscott added a commit to natoscott/htop that referenced this pull request Apr 14, 2021

Add time handling interfaces for the pcp platform

bdd4d39

Related to htop-dev#574

natoscott added a commit to natoscott/htop that referenced this pull request May 27, 2021

Add time handling interfaces for the pcp platform

4971253

Related to htop-dev#574

natoscott added a commit that referenced this pull request Jun 9, 2021

Add time handling interfaces for the pcp platform

d4a2587

Related to #574


		#if defined(HAVE_CLOCK_GETTIME)

		return clock_gettime(CLOCK_MONOTONIC, tp);

Uh oh!

Conversation

smalinux commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natoscott commented Mar 22, 2021

Uh oh!

BenBE commented Mar 22, 2021

Uh oh!

a3f left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

smalinux commented Mar 23, 2021

Uh oh!

smalinux commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenBE commented Mar 23, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgzones commented Mar 25, 2021

Uh oh!

BenBE commented Mar 25, 2021

Uh oh!

natoscott commented Mar 29, 2021

Uh oh!

natoscott commented Mar 30, 2021

Uh oh!

BenBE left a comment

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!

smalinux commented Mar 22, 2021 •

edited

Loading

smalinux commented Mar 23, 2021 •

edited

Loading