Skip to content

Loading…

CPU times should be reported in ms #673

Closed
wants to merge 2 commits into from

2 participants

@tfeb

This changes CPU times to be reported in milliseconds, not ticks:

  • for Linux this is done by 38032ec: this commit has been tested reasonably well;
  • for Solaris this is done by e79d4e7: I haven't tested this as I don't currently have a Solaris VM to try it on although I'm reasonably sure it is OK.

In both cases the division is done slightly carefully to round rather than truncate (ie 9/10 should be 1 not 0 which is achieved by (9 + 10/2)/10): this probably does not matter much now but it did in an earlier version of things where I had things returning seconds.

I have also checked (by reading, not building) the Darwin and *BSD equivalents of this code and I think they all return things in ms as well.

I haven't checked the AIX code: I don't know AIX and don't even potentially have a machine to test it on, so I'm not sure what it does.

@bnoordhuis bnoordhuis commented on the diff
src/unix/sunos.c
((6 lines not shown))
knp = (kstat_named_t *) kstat_data_lookup(ksp, (char *)"cpu_ticks_user");
assert(knp->data_type == KSTAT_DATA_UINT64);
- cpu_info->cpu_times.user = knp->value.ui64;
+ cpu_info->cpu_times.user = (1000 * knp->value.ui64 + clock_ticks/2)
+ / clock_ticks;

Style error.

@tfeb
tfeb added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on the diff
src/unix/linux/linux-core.c
((4 lines not shown))
- ts.user = clock_ticks * user;
- ts.nice = clock_ticks * nice;
- ts.sys = clock_ticks * sys;
- ts.idle = clock_ticks * idle;
- ts.irq = clock_ticks * irq;
+ /* we want values in milliseconds, not clock ticks. This is the
+ * most accurate way of doing this but is more liable to overflow.
+ * I don't think this matters as anything using these values needs
+ * to cope with potential overflows.
+ */
+
+ ts.user = (1000 * user + clock_ticks/2) / clock_ticks;
+ ts.nice = (1000 * nice + clock_ticks/2) / clock_ticks;
+ ts.sys = (1000 * sys + clock_ticks/2) / clock_ticks;
+ ts.idle = (1000 * idle + clock_ticks/2) / clock_ticks;
+ ts.irq = (1000 * irq + clock_ticks/2) / clock_ticks;

I suggest you do away with banker's rounding here. As you say, it's not that relevant when counting in milliseconds; if someone else reads the source, he'll have to do a double-take to figure out what it's doing, let alone why.

@tfeb
tfeb added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis

Closing, no longer applies. Thanks though.

@bnoordhuis bnoordhuis closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 27 additions and 9 deletions.
  1. +11 −5 src/unix/linux/linux-core.c
  2. +16 −4 src/unix/sunos.c
View
16 src/unix/linux/linux-core.c
@@ -640,11 +640,17 @@ static void read_times(unsigned int numcpus, uv_cpu_info_t* ci) {
&irq))
abort();
- ts.user = clock_ticks * user;
- ts.nice = clock_ticks * nice;
- ts.sys = clock_ticks * sys;
- ts.idle = clock_ticks * idle;
- ts.irq = clock_ticks * irq;
+ /* we want values in milliseconds, not clock ticks. This is the
+ * most accurate way of doing this but is more liable to overflow.
+ * I don't think this matters as anything using these values needs
+ * to cope with potential overflows.
+ */
+
+ ts.user = (1000 * user + clock_ticks/2) / clock_ticks;
+ ts.nice = (1000 * nice + clock_ticks/2) / clock_ticks;
+ ts.sys = (1000 * sys + clock_ticks/2) / clock_ticks;
+ ts.idle = (1000 * idle + clock_ticks/2) / clock_ticks;
+ ts.irq = (1000 * irq + clock_ticks/2) / clock_ticks;

I suggest you do away with banker's rounding here. As you say, it's not that relevant when counting in milliseconds; if someone else reads the source, he'll have to do a double-take to figure out what it's doing, let alone why.

@tfeb
tfeb added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ci[num++].cpu_times = ts;
}
fclose(fp);
View
20 src/unix/sunos.c
@@ -476,6 +476,11 @@ uv_err_t uv_cpu_info(uv_cpu_info_t** cpu_infos, int* count) {
kstat_t *ksp;
kstat_named_t *knp;
uv_cpu_info_t* cpu_info;
+ unsigned long clock_ticks;
+
+ clock_ticks = sysconf(_SC_CLK_TCK);
+ assert(clock_ticks != (unsigned long) -1);
+ assert(clock_ticks != 0);
if ((kc = kstat_open()) == NULL) {
return uv__new_sys_error(errno);
@@ -528,21 +533,28 @@ uv_err_t uv_cpu_info(uv_cpu_info_t** cpu_infos, int* count) {
cpu_info->cpu_times.idle = 0;
cpu_info->cpu_times.irq = 0;
} else {
+ /* conversions to ms below have good rounding but potential
+ * overflow */
knp = (kstat_named_t *) kstat_data_lookup(ksp, (char *)"cpu_ticks_user");
assert(knp->data_type == KSTAT_DATA_UINT64);
- cpu_info->cpu_times.user = knp->value.ui64;
+ cpu_info->cpu_times.user = (1000 * knp->value.ui64 + clock_ticks/2)
+ / clock_ticks;

Style error.

@tfeb
tfeb added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
knp = (kstat_named_t *) kstat_data_lookup(ksp, (char *)"cpu_ticks_kernel");
assert(knp->data_type == KSTAT_DATA_UINT64);
- cpu_info->cpu_times.sys = knp->value.ui64;
+ cpu_info->cpu_times.sys = (1000 * knp->value.ui64 + clock_ticks/2)
+ / clock_ticks;
knp = (kstat_named_t *) kstat_data_lookup(ksp, (char *)"cpu_ticks_idle");
assert(knp->data_type == KSTAT_DATA_UINT64);
- cpu_info->cpu_times.idle = knp->value.ui64;
+ cpu_info->cpu_times.idle = (1000 * knp->value.ui64 + clock_ticks/2)
+ / clock_ticks;
knp = (kstat_named_t *) kstat_data_lookup(ksp, (char *)"intr");
assert(knp->data_type == KSTAT_DATA_UINT64);
- cpu_info->cpu_times.irq = knp->value.ui64;
+ cpu_info->cpu_times.irq = (1000 * knp->value.ui64 + clock_ticks/2)
+ / clock_ticks;
+
cpu_info->cpu_times.nice = 0;
}
Something went wrong with that request. Please try again.