297015 - bltest should run multithreaded

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P2)

Description

[Alexei Volkov]

Make bltest to run in multi-threaded mode.
Also implement a mode, when bltest can run operations for a specific period
of time and utilize better reporting format to avoid data overlapping.

Currently bltest utilizes almost all low case letters for different options.
Nothing left but to take "-4 <num>" as a an option for specifying a number of
threads that should be spawned, and "-5 <num>" as a definition of a time period
for each thread to run.

The list of bltest test parameters is attached

Comment 1

[Alexei Volkov]

Attachment: bltest params

Comment 2

[Alexei Volkov]

Attachment: bug fix patch

Comment 3

[Alexei Volkov]

Attachment: output from bl test test run

Comment 4

[Julien Pierre]

Comment on attachment 185697 [details] [diff] [review]
bug fix patch

- You didn't document the -4 option for multithreading in the Usage function.

- the usage should note that the time intervals are in seconds

- you need to change the identation when you add a new logical block of code.
For example at
https://bugzilla.mozilla.org/attachment.cgi?id=185697&action=diff#mozilla/secur
ity/nss/cmd/bltest/blapitest.c_sec6 This issue happens many times throughout
the file. You need to ident all the relevant blocks .

- I think you should be checking the time for every operation, not every 10
operations

- the status of operations should be checked each time . using
"rv |= " and continuing after a failure is IMO incorrect. I realize that this
bug already existed, but it needs to be corrected. The loop should abort if
there is any error .

- you are using the repetitions field for two different purposes - either the
number of repetitions to perform, or the number of repetitions actually
performed. I think you should use two different variables for this. It makes
sense once your loop aborts on error, since the number of actually performed
iterations can be different from the number of iterations requested

- the status of DSA_SignDigest should be checked, both for the time and
iterations cases. This is a pre-existing bug, but it should be fixed ...

- same for ECDSA_SignDigest (which was checked in the iteration case, but you
didn't check it in the time case)

- same for ECDSA_VerifyDigest, (*cipherInfo->cipher.symmkeyCipher),
(*cipherInfo->cipher.pubkeyCipher),(*cipherInfo->cipher.hashCipher)

Comment 5

[Alexei Volkov]

Julien, thanks for the review.

> - you need to change the identation when you add a new logical block of code.
> For example at
> https://bugzilla.mozilla.org/attachment.cgi?id=185697&action=diff#mozilla/secur
> ity/nss/cmd/bltest/blapitest.c_sec6 This issue happens many times throughout
> the file. You need to ident all the relevant blocks .
Is there any way I can preview my changes without commiting a patch?

> - I think you should be checking the time for every operation, not every 10
> operations
I did implement with out the loop at the beginning. It appeared that time check
operations takes significant amount of time. So I've been getting 20% less
on all tests using -5 option compare to old runs with -p <num ops>. Time check
every 10 time helps a lot. I can minimize the number of iterations in the loop,
but don't think it will be the best to just remove them.

Comment 6

[Julien Pierre]

Alexei,

You should be able to spot the indentation errors by looking at your patch file
before you submit it for review.

I see your point in the high cost of timing functions. I think that's highly
dependent on the type of operations. For example, if you are doing 8192 bit RSA
key ops, you might not want to do them in chunks of 10, as that might take hours
on some machines. I don't know if there is a rule of thumb to apply here, though.

FYI, I only reviewed the first half of your patch. I submit the comments because
the network started acting funny here. I'll review the second half next.

Comment 7

[Alexei Volkov]

We can time one operation and then dicide on how many ops should be done without
time checks. 

Comment 8

[Julien Pierre]

Comment on attachment 185697 [details] [diff] [review]
bug fix patch

- In dump_performance_info, don't forget to use actual_repetitions, and not
repetitions, in the computations (see my previous review comment about separate
variables for request and actual repetitions).

- nit: the print_td label  was moved unnecessarily


- in the callers of functions that return the output of PR_smprintf, you need
to check the result for NULL . This is for all callers of getHighUnitOps and
getHighUnitBytes, as well as for other times where you call PR_smprintf
directly

- You can use PORT_ZNew instead of PORT_Alloc and PORT_Memset for allocating
cipherInfo and newCInfo

- You should not litter the code with multiple PORT_Free(cipherInfo) statements
. As much as I hate goto statements, I think this is a case where its use is
indicated . You should be able to factor all the PORT_Free(cipherInfo) /
Usage() pairs into 1, at least .

- nit : your indentation is off for the braces you added around PR_Close below
setupIO()


- Rather than having two functions named ThreadExecOthersTest and
ThreadExecMonteCarloTest, you can set a bit inside the cipherInfo structure,
and have a single function . This will make the code smaller and easier to read
and understand

- there is a pre-existing bug in the FIPS self-test . The code always returned
0, and you made it return SECSuccess . I think the right thing is to check for
CK_RV to be CKR_OK

Comment 9

[Alexei Volkov]

Attachment: the patch includes all Julien's recommendations

Comment 10

[Alexei Volkov]

Attachment: the patch includes all Julien's recommendations(-U options of diff seems to fix ident problems)

I had indentation corrected allover the places. Used diff -u -C 5. It looks
like
diff -U 5 should be used instead

Comment 11

[Julien Pierre]

Alexei,

Everything looks good, except for the following :

1) Your timeBetweenChecks variable should really be called opsBetweenChecks .

2) I agree with the logic of timing the first operation before deciding the
value of opsBetweenChecks. However, the code you wrote is very large, as for
each algorithm, you are duplicating the logic as follows :

TIMESTART();
do_op();
if (repetitionstoPerform) {
   for ( ... )
      do_op()
}
else {
   TIMEMARK()
   while (!TIMETOFINISH) {
        do_op()
   }
}
TIMEFINISH()

The problem is that you are calling "do_op()" 3 times. Only 2 are really needed.
Also, your initial timing op is not counted in the case when you are doing a
specified count of operations. The solution is to remove the separate code for
the timing op at the top, and use the first op of the while() loop as your
timing op . Your TIMETOFINISH macro should be able to handle resetting
opsBetweenChecks to a lower value if one op takes too long. Just pre-initialize
opsBetweenChecks to 10 .

Please correct this for each algorithm. This will make the code significantly
smaller and easier to read.

3) You are using the wrong test below . You need to check !filename instead of
!oufile 

                filename = PR_smprintf("%s/tests/%s/%s", testdir,              
                         mode_strings[cipherInfo->mode], filename);

                if (!outfile) {
                    fprintf(stderr, "%s: Can not allocate memory.\n", progName);
                    goto exit_point;
                }

Thanks !

Comment 12

[Julien Pierre]

Comment on attachment 186160 [details] [diff] [review]
the patch includes all Julien's recommendations(-U options of diff seems to fix ident problems)

Review comments are already in the bug. Just marking r-.

Comment 13

[Alexei Volkov]

Attachment: patch checked in as revision 1.43

Fixed items #1 and #3.

Comment 14

[Alexei Volkov]

Checking in blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.43; previous revision: 1.42

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 186171 [details] [diff] [review]
patch checked in as revision 1.43

This patch seems to have been checked in without receiving r+ first.
I haven't thoroughly reviewed it yet, but a cursory reading shows
several issues.  I'll review this more thoroughly tomorrow.

1. This patch introduces many lines that are wider than 80 columns,
some are MUCH wider than 80 columns.  That needs to be fixed.

2. Look at the following new block of code

>+    cipherInfo = cipherInfoListHead;
>+    while (cipherInfo != NULL) {
>+        if (cipherInfo->arena)
>+            PORT_FreeArena(cipherInfo->arena, PR_TRUE);
>+        bltestCipherInfo *tmpInfo = cipherInfo;

That's a c++ style declaration in the middle of a basic block.
It's a declaration after an executable line of code in the block.
It doesn't compile on Windows.	The build is broken due to this.
Fixing build breakage is a P0 issue.

>+        cipherInfo = cipherInfo->next;
>+        PORT_Free(tmpInfo);
>+    }

Shouldn't the cipherinfo be allocated from its own arena?

3. for portability accross multiple platforms, including windows,
main must always return a non-negative integer in the range 0-255.

>-    return SECSuccess;
>+    return rv;
> }

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

reopening.  Since we want to keep the trunk building at all times,
I may back this out tonight.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

I backed out that change, so that the builds should pass tonight. 

Alexei, please file a P1 bugster bug against the c compiler that 
didn't catch that c++ declaration in c code.  

Comment 18

[Alexei Volkov]

Attachment: patch includes fixes for Nelson's comments

Comment 19

[Alexei Volkov]

Attachment: patch includes fixes for Nelson's comments

wrong patch was attached previously

Comment 20

[Julien Pierre]

Nelson,

I had given a verbal r+ to Alexei for the code he checked in yesterday .
I wish we had a tinderbox to find the compiler error accross platforms. Sorry my
manual review didn't catch it.

Wrt the C++ variable declaration, I believe all compilers have flags to compile
in ANSI C mode. The NSS build system must be using the wrong flags, which allow
C++ declarations. That's the case apparently on Linux/gcc, since Alexei wrote
the code there. It may be the case on other platforms too. We should file a bug
against coreconf to use the right flags .

Comment 21

[Alexei Volkov]

Bug https://bugzilla.mozilla.org/show_bug.cgi?id=297708 is filed against nss
build system to make nss with ANSI C compiler flags.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Usually the -ansi cc command line option instructs the compiler to use 
header files that define ONLY the functions defined in ANSI's libc,
and do NOT instruct the compiler to exclude c++ syntax.  

In my experience, selecting the -ansi flag always necessitates adding additional 
command line options that tell the compiler "also include these non-ANSI 
extensions to the definition of libc's API".  For example, when you use -ansi
with gcc on Solaris, it is typically necessary to also add  -D__EXTENSIONS__  
to get anything to compile.

A c compiler's default behavior should be to compile the c language.
The ability to permit language extensions should be enabled with a flag.

Comment 23

[Alexei Volkov]

Checking in blapitest.c;
/cvsroot/mozilla/security/nss/cmd/bltest/blapitest.c,v  <--  blapitest.c
new revision: 1.44; previous revision: 1.45