Closed Bug 206971 Opened 21 years ago Closed 21 years ago

selfserv should not OptimizeSpace

Categories

(NSS :: Tools, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kirk.erickson, Assigned: kirk.erickson)

Details

Attachments

(2 files, 4 obsolete files)

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().
P2, targeting 3.9.
Priority: -- → P2
Summary: selfserv should OptimizeSpeed (use NSS_Initialize) → selfserv should not OptimizeSpace
Target Milestone: --- → 3.9
Attached patch Proposed patch for the TIP (obsolete) — Splinter Review
Correction - NSS_Init() is hardcoded to set PR_TRUE.
Selfserv must call NSS_Initialize().
3.2% gain on fullhandshakes.
12.5% gain on restarts.
Should strsclnt be allocating this way too?
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.
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.
Attached patch Proposed patch for the TIP (obsolete) — Splinter Review
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.
Attachment #124128 - Attachment is obsolete: true
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?
Attachment #124343 - Flags: review-
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.
Attached patch Proposed patch for the TIP (obsolete) — Splinter Review
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.
Attachment #124343 - Attachment is obsolete: true
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?
Attachment #124417 - Flags: review+
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 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.
Attachment #124417 - Flags: review+ → review-
Backed out of changes to pk11db.c and pkcs11i.h.
Attachment #124417 - Attachment is obsolete: true
Comment on attachment 124559 [details] [diff] [review]
Proposed patch for the TIP

r=wtc.	Thanks, Kirk.
Attachment #124559 - Flags: review+
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.
Attachment #124129 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: