287625 - rsaperf should run multithreaded

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

Currently, rsaperf can only run single-threaded. It would be very useful to be
able to run it multithreaded in order to measure the aggregate performance on
multiple CPU machines.

Comment 1

[Julien Pierre]

Attachment: sample code

This is sample source code of a how to start a function in multiple threads
with NSPR . The same logic could be used in rsaperf. I suggest adding a -t
<threads> command-line option. If iterations are specified with -i, each thread
should perform iterations/threads operations .

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This can't be P1 for 3.10!
It might be P1 for 3.11, and could be P2 for 3.10,
but there's no way that Sun or RedHat would stop the 3.10 release
if this bug wasn't fixed.  Please correct the priority or target
or both.

Comment 3

[Julien Pierre]

I'd like this change soon - in 3.10 - for benchmarking. Marking P2.

Comment 4

[Alexei Volkov]

Attachment: bug fix patch

One thing I'm uncertain with: when rsaperf is calculating the time it spend on
a
fixed number on interaction, should it spread given number across all threads
or
should each thread run and time itself against the whole number?

Currently second variant is implemented.

Comment 5

[Julien Pierre]

Alexei,

To answer your question, either way is fine. Your patch is good overall, except
for the following :

1) In the ThreadRunDataStr structure, I'm concerned about your use of the
"duration" variable. Its name implies a time measurement, but you are using it
for multiple purposes. 

- in ThreadExecFunction, in the case where rsaperf runs for a fixed period of
time, you are using it first to store the duration of the run, and then you are
resetting it to zero and using it to store the number of iterations performed .

- in ThreadExecFunction, in the case where rsaperf runs for a fixed number of
iterations, you are using it to store the number of iterations to perform.
However, you don't keep a count of the iterations actually performed, which
could be useful in case of errors.

I suggest you don't worry about the size of the ThreadRunDataStr structure, and
use different variables for each purpose : fixed duration of the run, number of
iterations to perform, and iterations actually performed .

2) In ThreadExecFunction, it would be better not to return abruptly in case of
error, so that tdata->stop is set . Otherwise, your ops/s calculations will be
off. I suggest you initialize tdata->status to SECSuccess at the top of the
function, and use break statements for both while loops in case fn() fails .

3) The PRTime type should be used to store return values from PR_Now() . I know
the existing code uses int64, but it is wrong. At least for your new code,
please use PRTime for the tstart and tstop fields. Feel free to change any other
erroneous int64 to PRTime if you wish.

4) Minor nit - please try not to use a goto loser statement within your cleanup
loop at the end of the main() function . Using if { } else { } should be
sufficient to accomplish what you need here .

Comment 6

[Alexei Volkov]

Attachment: includes review code fix

This patch contains fixes for all problems Julien notice in the code except
item #2:

One way or another, the calculation will be off. This is how it was done
in original code and I though it will be better not to include calculations
from threads where error had occurred. So this patch does not has the change
that counts partial results from "bad" threads.

Comment 7

[Julien Pierre]

Comment on attachment 178879 [details] [diff] [review]
includes review code fix

Looks good.

I missed one thing in my review - you are using PR_Malloc and PR_Free . In NSS,
we use the wrapper PORT_Malloc and PORT_Free . You can check in your code once
this change is made.

Thanks !

Comment 8

[Alexei Volkov]

Attachment: includes PR_ changes to PORT

In addition PR_GetError is replaced with PORT_GetError