Date: Thu, 11 Apr 2013 16:46:34 -0600 From: "Kenneth D. Merry" <ken@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r249334 - head/usr.bin/ctlstat Message-ID: <20130411224634.GA56177@nargothrond.kdm.org> In-Reply-To: <20130411163712.Y1200@besplex.bde.org> References: <201304101601.r3AG1jZq083572@svn.freebsd.org> <20130411163712.Y1200@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Apr 11, 2013 at 17:06:44 +1000, Bruce Evans wrote: > On Wed, 10 Apr 2013, Kenneth D. Merry wrote: > > >Log: > > Fix a time calculation error in ctlstat_standard(). > > > > ctlstat.c: When converting a timeval to a floating point > > number in ctlstat_standard(), cast the nanoseconds > > calculation to a long double, so we don't lose > > precision. Without the cast, we wind up with a > > time in whole seconds only. > >... > >Modified: head/usr.bin/ctlstat/ctlstat.c > >============================================================================== > >--- head/usr.bin/ctlstat/ctlstat.c Wed Apr 10 11:26:30 2013 (r249333) > >+++ head/usr.bin/ctlstat/ctlstat.c Wed Apr 10 16:01:45 2013 (r249334) > >@@ -416,9 +416,10 @@ ctlstat_standard(struct ctlstat_context > > if (F_CPU(ctx) && (getcpu(&ctx->cur_cpu) != 0)) > > errx(1, "error returned from getcpu()"); > > > >- cur_secs = ctx->cur_time.tv_sec + (ctx->cur_time.tv_nsec / > >1000000000); > >+ cur_secs = ctx->cur_time.tv_sec + > >+ ((long double)ctx->cur_time.tv_nsec / 1000000000); > > prev_secs = ctx->prev_time.tv_sec + > >- (ctx->prev_time.tv_nsec / 1000000000); > >+ ((long double)ctx->prev_time.tv_nsec / 1000000000); > > > > etime = cur_secs - prev_secs; > > long double is rarely necessary. It mainly asks for slowness (10-50%) > on i386 and extreme slowness (hundreds of times slower) on space64. > Double precision is plenty. Many arches in FreeBSD have long double == > double, so you can't depend on long double being more precise than double, > and statistics utilities are especially not in need of much precision. > (Float precision would be enough here. It would be accurate to 1/16 > of a microsecond. Not to 1 nanosecond, but you don't need that. The > integer division was only accurate to 1/4 second, so ist error was > noticeable.) > > There is no need for any casts. There is no need for any divisions. > Simply multiply by 1 nanosecond. This must be in floating point, since > integers can't represent 1 nanosecond (neither can floating, but the > error of ~2**-53 nanosecnds for double precision is neglogible). When > 1 nanosecond is in a floating point literal, the whole expression is > automatically promoted correctly. > > Other style bugs in the above: > - non-KNF indentation (1 tab) for the newly split line > - different non-KNF indentation (5 spaces) for the previously split line > - exessive parentheses around the division operation > - bogus blank line which splits up the etime initialization > - general verboseness from the above. > > Fixing these gives: > > cur_secs = ctx->cur_time.tv_sec + ctx->cur_time.tv_nsec * 1e-9; > prev_secs = ctx->prev_time.tv_sec + ctx->prev_time.tv_nsec * 1e-9 > etime = cur_secs - prev_secs; > > It is now clear that this is still too verbose, since cur_secs and prev_secs > are not used except to initialize etime. Simplifying this gives: > > etime = ctx->cur_time.tv_sec - ctx->prev_time.tv_sec + > (ctx->prev_time.tv_nsec - ctx->cur_time.tv_nsec) * 1e-9; > > This might need casting to double of ctx->cur_time.tv_sec, in case time_t > is unsigned and the time went backwards. Otherwise, this should be the > usual expression for subtracting timespecs. The time can't go backwards in this case, because it is the system uptime. Using wall clock time causes problems measuring performance when NTP decides that the system time needs to change. I have a local patch to dd to fix instances of bogus performance numbers due to its using wall clock time to measure elapsed time. This should be fixed now, thanks for the review! Ken -- Kenneth Merry ken@FreeBSD.ORG
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130411224634.GA56177>