287498 - Add option for rsaperf to run for a fixed duration, and display ops/s

Status: RESOLVED FIXED

(NSS :: Test, defect, P2)

Description

[Julien Pierre]

As summary suggests.

Comment 1

[Julien Pierre]

Attachment: add -p period option, and display ops/s

Comment 2

[Julien Pierre]

I felt the need to add this option because doing a particular number of
iterations has widely different results depending on the machine and can often
be too short or too long to be useful.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 178443 [details] [diff] [review]
add -p period option, and display ops/s

This patch was mostly OK, but I had two issues with it, one 
stylistic and one substantive.


>+PRUint32 TimingSeconds(TimingContext *ctx) {
>+    return ctx->millisecs/1000 + ctx->seconds + ctx->minutes*60 +
>+        ctx->hours*3600 + ctx->days*86400;
>+}
>+

First, the value ctx->interval is already the elapsed time in 
microseconds.  The values you used (seconds, milliseconds, minutes,
etc) are derived from it in the function immediate below.  
So, just use the ctx->interval value instead.  

Second, I'm concerned about the loss of precision by truncating
the fraction of a second here.	Rounding would be better, but the
best thing, IMO, is to compute the ops/second using the fractions
of a second also, and then dislpay the result in whatever 
precision you desire.  

> static void timingUpdate(TimingContext *ctx) {
>     int64 tmp, remaining;
>     int64 L1000,L60,L24;
> 
>     LL_I2L(L1000,1000);
>     LL_I2L(L60,60);
>     LL_I2L(L24,24);
> 
>     LL_DIV(remaining, ctx->interval, L1000);
>     LL_MOD(tmp, remaining, L1000);



Stylistic comment.  You added a new if-then-else whose style doesn't 
match the rest of the file.  E.g.

>+    if (doIters)
>+    {

>+    }
>+    else
>+    {

>     }

Comment 4

[Julien Pierre]

Attachment: address Nelson's issues

Comment 5

[Julien Pierre]

I checked in the updated patch to the tip .

Checking in rsaperf.c;
/cvsroot/mozilla/security/nss/cmd/rsaperf/rsaperf.c,v  <--  rsaperf.c
new revision: 1.10; previous revision: 1.9

Comment 6

[Julien Pierre]

Reopening to change assigned field.

Comment 7

[Julien Pierre]

Marking fixed.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 178520 [details] [diff] [review]
address Nelson's issues

There is one potential problem with this patch.


>     TimingEnd(timeCtx);
>     printf("%ld iterations in %s\n",
> 	   iters, TimingGenerateString(timeCtx));
>+    printf("%.2f operations/s .\n", (double)(iters*1000000) / (double)timeCtx->interval );

On a 32-bit machine, if iters > 2147, this expression will experience 
an integer overflow, and produce an incorrect number.  
I suggest this:

>+    printf("%.2f operations/s .\n", 
 +	     ((double)(iters) * (double)1000000.0) / (double)timeCtx->interval
);

This code is only correct on machines where PRInt64 is a primite integer
type, not on those where it is a struct of two PRUint32s.  But I think 
that all supported platforms now have a primite integer type for PRInt64,
so it's probably safe.	

Does printf need a different format specifier for double than for float?
(I seem to recall that it did not in K&R v1, because all floats got 
promoted to doubles when passed as function arguments.	But now with 
ANSI c, I'm not sure (don't have K&R 2 handy to check).

>     TimingDivide(timeCtx, iters);
>     printf("one operation every %s\n", TimingGenerateString(timeCtx));

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

reopening for resolution to integer overflow issue with previous patch.

Comment 10

[Julien Pierre]

The %f specified is fine for both double and float, as far as I can tell from
the man page on printf(3C) in Solaris.

I verified your proposed change fixed the problem, and checked it in .

Checking in rsaperf.c;
/cvsroot/mozilla/security/nss/cmd/rsaperf/rsaperf.c,v  <--  rsaperf.c
new revision: 1.11; previous revision: 1.10
done

Marking fixed again.