From owner-svn-src-all@FreeBSD.ORG Thu Apr 11 07:06:55 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id D2BEF65E; Thu, 11 Apr 2013 07:06:55 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail35.syd.optusnet.com.au (mail35.syd.optusnet.com.au [211.29.133.51]) by mx1.freebsd.org (Postfix) with ESMTP id 7110D3F0; Thu, 11 Apr 2013 07:06:55 +0000 (UTC) Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail35.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r3B76iCs014378 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 11 Apr 2013 17:06:46 +1000 Date: Thu, 11 Apr 2013 17:06:44 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: "Kenneth D. Merry" Subject: Re: svn commit: r249334 - head/usr.bin/ctlstat In-Reply-To: <201304101601.r3AG1jZq083572@svn.freebsd.org> Message-ID: <20130411163712.Y1200@besplex.bde.org> References: <201304101601.r3AG1jZq083572@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=Ov0XUFDt c=1 sm=1 a=jrZi4plbApYA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=ns49S-B92uUA:10 a=YHl7f7u7s7nfAW9j7NkA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Apr 2013 07:06:55 -0000 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