206971 - selfserv should not OptimizeSpace

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Kirk Erickson]

Selfserv uses NSS_Init().  With the latest session bucketing
implementation, this means selfserv is acting like a client.
Servers should use NSS_Initialize().

Selfserv initializes thusly:

    rv = NSS_Init(dir);

The session bucketing code introduced an optimizeSpace flag,
which defaults to PR_TRUE.  As a result, selfserv is a token
object and session hash sizes of 32 rather than 1024.

NSS_Init() is used by clients (setting optimizeSpace).
NSS_Initialize() is used by servers.  It contains:

((flags & NSS_INIT_OPTIMIZESPACE) == NSS_INIT_OPTIMIZESPACE));

selfserv should be calling NSS_Initialize() directly to avoid
setting the NSS_INIT_OPTIMIZESPACE flag, or supplying PR_FALSE
for the optimizeSpace argument to NSS_Init().

Comment 1

[Kirk Erickson]

P2, targeting 3.9.

Comment 2

[Kirk Erickson]

Attachment: Proposed patch for the TIP

Correction - NSS_Init() is hardcoded to set PR_TRUE.
Selfserv must call NSS_Initialize().

Comment 3

[Kirk Erickson]

Attachment: selfserv stress with and without the proposed patch

3.2% gain on fullhandshakes.
12.5% gain on restarts.

Comment 4

[Kirk Erickson]

Should strsclnt be allocating this way too?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

The name "secmod.db" is not correct for all platforms.  
Whenever the NSS libraries use it internally, they define the symbol SECMOD_DB
as follows (in nss.c and in pkcs11i.h):

#ifdef macintosh
#define SECMOD_DB "Security Modules"
#else
#define SECMOD_DB "secmod.db"
#endif

I think we should move the above code to nss.h, a public header file, so that
all callers of the public function NSS_Initialize will have access to it.

Then the patch to selfserv becomes the following one-line change:

-    rv = NSS_Init(dir);
+    rv = NSS_Initialize(dir, "", "", SECMOD_DB, NSS_INIT_READONLY);

And, yes, I think strsclnt should do this, too.

Comment 6

[Wan-Teh Chang]

Nelson's suggestion is good.

I agree that strsclnt should call do this, too.
Although strsclnt is a client program, we use it
for performance benchmarking so it should optimize
speed.

Comment 7

[Kirk Erickson]

Attachment: Proposed patch for the TIP

This patch fixes both selfserv and strsclnt to OptimizeSpeed.
Note, removing the SECMOD_DB defines from pkcs11i.h made compilation
of pk11db.c fail, so this proposal leaves the defines in pkcs11i.h.

Comment 8

[Wan-Teh Chang]

Comment on attachment 124343 [details] [diff] [review]
Proposed patch for the TIP

Kirk, could you fix the indentation of the NSS_Initialize
line in selfserv.c?  I think you just need to replace the
tab at the beginning of the line by four spaces.

The pkcs11i.h problem can be solved by having pk11db.c
include nss.h to get the definition of SECMOD_DB.  Bob,
is it okay for a softoken source file to include nss.h?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Re: Comment 8 
nss.h does NOT presently define SECMOD_DB.  It should.  That should be fixed.
THEN the one-line patch I suggested above will work.

Comment 10

[Kirk Erickson]

Attachment: Proposed patch for the TIP

Corrected indentation at selfserv.c:1641.
Made nss/lib/softoken/pk11db.c include nss.h.
Removed SECMOD_DB defines from nss/lib/softoken/pkcs11i.h.

Comment 11

[Wan-Teh Chang]

Comment on attachment 124417 [details] [diff] [review]
Proposed patch for the TIP

r=wtc.

Question for Bob and Nelson: would secmod.h or secmodt.h be
a better place than nss.h to define the SECMOD_DB macro?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

I'm not sure where isthe best place for this #define symbol, but I feel
strongly that softoken should not be dependent on nss.h.  
I'd give review- to the two patches of 2003-05-28 for that reason.

Comment 13

[Wan-Teh Chang]

Comment on attachment 124417 [details] [diff] [review]
Proposed patch for the TIP

Kirk,

I'm sorry that I need to ask you to revise this
patch again.  Bob and Nelson explained to me that
the softoken should not depend on anything in NSS
above the PKCS#11 interface, so softoken should
not include nss.h.

Could you remove the changes to softoken/pk11db.c
and softoken/pkcs11i.h from this patch?  Thanks.

Comment 14

[Kirk Erickson]

Attachment: Proposed patch for the TIP

Backed out of changes to pk11db.c and pkcs11i.h.

Comment 15

[Wan-Teh Chang]

Comment on attachment 124559 [details] [diff] [review]
Proposed patch for the TIP

r=wtc.	Thanks, Kirk.

Comment 16

[Kirk Erickson]

Attachment: selfserv stress with and without the proposed patch

Benchmarked selfserv of this mornings TIP (full, restart),
and after applying this patch (full-speed, restart-speed).

13.5% gain on restarts

Also ran a strsclnt locally against selfserv on dsame2 for
awhile to verify strsclnt works.

Comment 17

[Kirk Erickson]

Checked into the TIP:

Resolves bug 206971 - selfserv should OptimizeSpeed (use NSS_Initialize).
Both selfserv and strsclnt no longer OptimizeSpace.  Moved SECMOD_DB
defines from nssinit.c to nss.h, make it availble for public use with
NSS_Initialize().

Checking in mozilla/security/nss/cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v	 <--  selfserv.c
new revision: 1.52; previous revision: 1.51
done
Checking in mozilla/security/nss/cmd/strsclnt/strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v	 <--  strsclnt.c
new revision: 1.33; previous revision: 1.32
done
Checking in mozilla/security/nss/lib/nss/nss.h;
/cvsroot/mozilla/security/nss/lib/nss/nss.h,v  <--  nss.h
new revision: 1.28; previous revision: 1.27
done
Checking in mozilla/security/nss/lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--	nssinit.c
new revision: 1.63; previous revision: 1.62
done