svn commit: r249334 - head/usr.bin/ctlstat

[Bruce Evans]

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.

Bruce