Unhardcode tick-to-ms conversion #381

amomchilov · 2020-12-11T01:42:13Z

Division by 100000.0 worked because sysconf(_SC_CLK_TCK) happened to be 100.

By unhardcoding:

It becomes more clear what this 10000.0 figure comes from.
It protects against bugs in the case sysconf(_SC_CLK_TCK) ever changes.

natoscott · 2020-12-11T02:36:18Z

darwin/DarwinProcess.c

@@ -240,6 +240,12 @@ void DarwinProcess_setFromKInfoProc(Process* proc, const struct kinfo_proc* ps,
 proc->updated = true;
 }

+double ticks_to_nanoseconds(const double ticks) {
+ const double secs_per_tick = 1.0 / (double)sysconf(_SC_CLK_TCK);


Probably best to cache the results of this sysconf call so that we're not recalculating it on every sample? (perhaps a global 'hertz' var set from either the darwin Platform_init or ProcessList_new routine)

sysconf can also fail, so doing this call once on startup will make error checking / handling easier.

Agreed! Though I'm not a fan of herz, I'd rather something like clock_ticks_per_s. Thoughts?

I just feeling out if this PR is a move in the right direction.

I noticed that a lot of this code-base is unclear about units, exact derivations, etc., so I'm not sure if this kind of change would be desired or not.

Agreed! Though I'm not a fan of herz, I'd rather something like clock_ticks_per_s. Thoughts?

Sure, doesn't have to be hertz - if its multi-words like that though camel case is more commonly used in htop (see darwin/Platform.c for some examples).

I just feeling out if this PR is a move in the right direction.

Looks like its moving in a less broken direction - so two thumbs up from me.

Ah yes, I didn't notice. Looks like local vars are snake case, and functions/types are camel case. I'll change my identifiers to match.

Some other questions:

Should I add an explicit but technically redundant #include <unistd.h>? It's what defines sysconf, but it's also already indirectly included (probably by CoreFoundation, but I haven't checked.

The return type of sysconf is long. Should the cache variable match that and be a long, or use uint64_t like the rest? Or should I cache it after conversion to double?

I saw that sysconf(_SC_CLK_TCK) is also called in LinuxProcess_adjustTime. They called the cache var jiffy. It appears that they use a lazy-initialization pattern, that makes sysconf(_SC_CLK_TCK) called upon the first call to LinuxProcess_adjustTime.

They fail silently if the errno is set, by just assuming 100 Hz. That seems... like the wrong choice to me. Wouldn't it be better to fail loudly than to assume a potentially wrong value? I feel like I'm pulling a thread here haha.

1. Should I add an explicit but technically redundant `#include <unistd.h>`? It's what defines `sysconf`, but it's also already indirectly included (probably by CoreFoundation, but I haven't checked.

I'd not add it explicitly unless its needed to compile - as you say, chances are its included indirectly already.

2. The return type of `sysconf` is `long`. Should the cache variable match that and be a `long`, or use `uint64_t` like the rest? Or should I cache it after conversion to `double`?

I recommend you do as much work up front as possible, at startup, and as little work as possible during each sample.
Thus, I think its best to store it as a double (like you had it in the original patch).

3. I saw that `sysconf(_SC_CLK_TCK)` is also called in `LinuxProcess_adjustTime`. They called the cache var `jiffy`. It appears that they use a lazy-initialization pattern, that makes `sysconf(_SC_CLK_TCK)` called upon the first call to `LinuxProcess_adjustTime`.

There's a good chance this code can be improved also.

They fail silently if the `errno` is set, by just assuming 100 Hz. That seems... like the wrong choice to me. Wouldn't it be better to fail loudly than to assume a potentially wrong value? I feel like I'm pulling a thread here haha.

Heh - yep. It definitely would be better to fail htop at startup if this call fails - on Linux too, IMO.

I recommend you do as much work up front as possible, at startup, and as little work as possible during each sample.
Thus, I think its best to store it as a double (like you had it in the original patch).

I had a change of mind on this. Though double suits my usecase, some other feature might end up re-using it and converting it to uint64_t, so I was leaning towards keeping it as its original long. It's just one cvttsd2si instruction, and the compiler could probably lift the conversion of out of the loop anyway.

Heh - yep. It definitely would be better to fail htop at startup if this call fails - on Linux too, IMO.

Opened #384 to track this.

@natoscott NVM, the style guide suggests to add such headers: https://github.com/htop-dev/htop/blob/master/docs/styleguide.md#import-and-use-of-headers

BenBE · 2020-12-11T06:51:25Z

Might help resolve #368 …

darwin/Platform.c

BenBE

Not sure if all the explicit double casts are technically required.

darwin/DarwinProcess.c

darwin/Platform.c

BenBE · 2020-12-11T17:54:00Z

darwin/Platform.h

@@ -31,6 +31,8 @@ extern const unsigned int Platform_numberOfSignals;

 extern const MeterClass* const Platform_meterTypes[];

+extern long Platform_clockTicksPerSec;


Probably something for another PR, but if multiple platforms have this (Darwin + Linux, but basically *BSD too), this should most likely go into a global platform header (cf. #306) for some central interface for such universally needed information.

Agreed. See also: #384

This comment was mostly about this declaration itself, which is probably better suited to have in some platform-independent header and not with every platform.

That's different from #384, but could easily be resolved there.

BenBE · 2020-12-12T14:02:15Z

darwin/DarwinProcess.c

@@ -6,6 +6,7 @@ in the source distribution for its full text.
 */

 #include "DarwinProcess.h"
+#include "Platform.h"


Cf. styleguide on include order.

I think I got it right this time, let me know.

BenBE · 2020-12-12T14:03:37Z

darwin/DarwinProcess.c

- / ((double)dpl->global_diff * 100000.0);
+ double time_interval = ticksToNanoseconds(dpl->global_diff) / (double) dpl->super.cpuCount;
+
+ proc->super.percent_cpu = ((double)diff / time_interval) * 100.0;


Should include some check for time_interval to not be too small. Epsilon check for interval > 1e-6 should be fine.

What's the meaning if interval is so small? Two back-to-back updates were done in very short succession?

For example. Another possibility is, when the notebook was in standby for a certain time. @cgzones fixed a similar issue for Linux recently, cf. #353 …

darwin/Platform.c

BenBE · 2020-12-13T11:33:46Z

I rebased you PR onto #392 to include the changes from there.
Resolved the inevitable merge conflicts in the process.

cgzones · 2020-12-13T13:17:20Z

darwin/DarwinProcess.c

@@ -240,23 +241,28 @@ void DarwinProcess_setFromKInfoProc(Process* proc, const struct kinfo_proc* ps,
 proc->updated = true;
 }

+static double ticksToNanoseconds(const double ticks) {
+ const double nanos_per_sec = 1e9;
+ return ticks / (double) Platform_clockTicksPerSec * Platform_timebaseToNS * nanos_per_sec;


Platform_clockTicksPerSec * Platform_timebaseToNS * nanos_per_sec is a runtime constant.
Maybe pre-compute it

You mean something like?

static const double tickToNanosecondsScale = (double) Platform_clockTicksPerSec * Platform_timebaseToNS * 1e9; return ticks / tickToNanosecondsScale;

I think this would not work, as static variables are initialized before main() (and Platform_clockTicksPerSec and Platform_timebaseToNS are not yet set at this point).

Maybe it's OK to compute once every cycle when caching time_interval (see below).

k, had to recheck on the actual initialization semantics here. Would take a second (bool) variable to act as lazy initialization guard to calculate this constant on first entry.

But as both values are set in Platform_init we can just initialize that scaling factor there.

cgzones · 2020-12-13T13:20:19Z

darwin/DarwinProcess.c

- if (0 != proc->utime || 0 != proc->stime) {
- uint64_t diff = (pti.pti_total_system - proc->stime)
- + (pti.pti_total_user - proc->utime);
+ double time_interval = ticksToNanoseconds(dpl->global_diff) / (double) dpl->super.cpuCount;


double time_interval = ticksToNanoseconds(dpl->global_diff) / (double) dpl->super.cpuCount; is constant for all processes of a single update cycle.
Maybe pre-compute it in DarwinProcessList.c::ProcessList_goThroughEntries()

Then this should be passed in as parameter to DarwinProcess_setFromLibprocPidinfo to avoid depending on global state.

Division by 100000.0 worked because `sysconf(_SC_CLK_TCK)` happened to be 100. By unhardcoding: 1) It becomes more clear what this 100000.0 figure comes from. 2) It protects against bugs in the case `sysconf(_SC_CLK_TCK)` ever changes.

darwin/Platform.c

cgzones · 2020-12-19T20:31:46Z

Applied via 67ccd6b

Thanks for the contribution!

natoscott reviewed Dec 11, 2020

View reviewed changes

amomchilov force-pushed the darwin-cpu-tick-to-s branch from d5fbf7a to e830ea1 Compare December 11, 2020 03:21

amomchilov mentioned this pull request Dec 11, 2020

Linux: read sysconf(_SC_CLK_TCK) on startup and fail loudly #384

Closed

amomchilov requested a review from natoscott December 11, 2020 14:09

amomchilov force-pushed the darwin-cpu-tick-to-s branch from e830ea1 to 0adfecb Compare December 11, 2020 14:23

BenBE reviewed Dec 11, 2020

View reviewed changes

darwin/Platform.c Outdated Show resolved Hide resolved

amomchilov force-pushed the darwin-cpu-tick-to-s branch 2 times, most recently from 13945a1 to b5c13dd Compare December 11, 2020 17:18

amomchilov requested a review from BenBE December 11, 2020 17:18

BenBE requested changes Dec 11, 2020

View reviewed changes

amomchilov force-pushed the darwin-cpu-tick-to-s branch from dcb4a07 to 5d84b87 Compare December 12, 2020 04:46

amomchilov requested a review from BenBE December 12, 2020 04:47

BenBE requested changes Dec 12, 2020

View reviewed changes

amomchilov force-pushed the darwin-cpu-tick-to-s branch from 5d84b87 to dccca07 Compare December 13, 2020 01:17

amomchilov requested a review from BenBE December 13, 2020 01:18

BenBE force-pushed the darwin-cpu-tick-to-s branch from dccca07 to 27a3afc Compare December 13, 2020 11:30

BenBE approved these changes Dec 13, 2020

View reviewed changes

cgzones reviewed Dec 13, 2020

View reviewed changes

cgzones added this to the 3.0.4 milestone Dec 13, 2020

Unhardcode tick-to-ms conversion

56557fc

Division by 100000.0 worked because `sysconf(_SC_CLK_TCK)` happened to be 100. By unhardcoding: 1) It becomes more clear what this 100000.0 figure comes from. 2) It protects against bugs in the case `sysconf(_SC_CLK_TCK)` ever changes.

BenBE force-pushed the darwin-cpu-tick-to-s branch from 27a3afc to 56557fc Compare December 13, 2020 19:14

natoscott reviewed Dec 14, 2020

View reviewed changes

darwin/Platform.c Show resolved Hide resolved

natoscott reviewed Dec 14, 2020

View reviewed changes

darwin/Platform.c Show resolved Hide resolved

cgzones mentioned this pull request Dec 19, 2020

Unhardcode tick-to-ms conversion - rebased #415

Merged

cgzones closed this Dec 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unhardcode tick-to-ms conversion #381

Unhardcode tick-to-ms conversion #381

amomchilov commented Dec 11, 2020

natoscott Dec 11, 2020

amomchilov Dec 11, 2020

natoscott Dec 11, 2020

amomchilov Dec 11, 2020

natoscott Dec 11, 2020

amomchilov Dec 11, 2020 •

edited

Loading

amomchilov Dec 12, 2020

BenBE commented Dec 11, 2020

BenBE left a comment

BenBE Dec 11, 2020

amomchilov Dec 12, 2020

BenBE Dec 12, 2020 •

edited

Loading

BenBE Dec 12, 2020

amomchilov Dec 13, 2020

BenBE Dec 12, 2020

amomchilov Dec 13, 2020

BenBE Dec 13, 2020

BenBE commented Dec 13, 2020

cgzones Dec 13, 2020

BenBE Dec 13, 2020

cgzones Dec 13, 2020 •

edited

Loading

BenBE Dec 13, 2020

cgzones Dec 13, 2020

BenBE Dec 13, 2020

cgzones commented Dec 19, 2020

		@@ -31,6 +31,8 @@ extern const unsigned int Platform_numberOfSignals;

		extern const MeterClass* const Platform_meterTypes[];

		extern long Platform_clockTicksPerSec;

Unhardcode tick-to-ms conversion #381

Unhardcode tick-to-ms conversion #381

Conversation

amomchilov commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenBE commented Dec 11, 2020

BenBE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenBE Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenBE commented Dec 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgzones Dec 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgzones commented Dec 19, 2020

amomchilov Dec 11, 2020 •

edited

Loading

BenBE Dec 12, 2020 •

edited

Loading

cgzones Dec 13, 2020 •

edited

Loading