Closed
Bug 206971
Opened 21 years ago
Closed 21 years ago
selfserv should not OptimizeSpace
Categories
(NSS :: Tools, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.9
People
(Reporter: kirk.erickson, Assigned: kirk.erickson)
Details
Attachments
(2 files, 4 obsolete files)
2.73 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
2.15 KB,
text/html
|
Details |
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().
Assignee | ||
Comment 1•21 years ago
|
||
P2, targeting 3.9.
Priority: -- → P2
Summary: selfserv should OptimizeSpeed (use NSS_Initialize) → selfserv should not OptimizeSpace
Target Milestone: --- → 3.9
Assignee | ||
Comment 2•21 years ago
|
||
Correction - NSS_Init() is hardcoded to set PR_TRUE. Selfserv must call NSS_Initialize().
Assignee | ||
Comment 3•21 years ago
|
||
3.2% gain on fullhandshakes. 12.5% gain on restarts.
Assignee | ||
Comment 4•21 years ago
|
||
Should strsclnt be allocating this way too?
Comment 5•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Comment 7•21 years ago
|
||
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 8•21 years ago
|
||
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-
Comment 9•21 years ago
|
||
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.
Assignee | ||
Comment 10•21 years ago
|
||
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 11•21 years ago
|
||
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+
Comment 12•21 years ago
|
||
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•21 years ago
|
||
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-
Assignee | ||
Comment 14•21 years ago
|
||
Backed out of changes to pk11db.c and pkcs11i.h.
Attachment #124417 -
Attachment is obsolete: true
Comment 15•21 years ago
|
||
Comment on attachment 124559 [details] [diff] [review] Proposed patch for the TIP r=wtc. Thanks, Kirk.
Attachment #124559 -
Flags: review+
Assignee | ||
Comment 16•21 years ago
|
||
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
Assignee | ||
Comment 17•21 years ago
|
||
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.
Description
•