124037 - Suggestion from NSS to decrease cert manager loading time.

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, enhancement, P2)

Description

[Stephane Saux]

From bug 121487

BTW, PMS can decrease the startup time of the CERT Dialog by 1/3 by rearranging
it's code a bit. Currently PMS calls PK11_ListCerts to extract all the certs
from the token, then filters these lists for the certs it wants. In the course
of creating the CERT Dialog it calls PK11_ListCerts 3 separate times. If the
code called PK11_ListCerts once, then sorted the certificates it would reduce
it's bringup time by about 1/3 (assuming PK11_ListCerts is a significant part of
the bringup of the dialog).

Comment 1

[John Unruh]

nsbeta1

Comment 2

[Kai Engert [:KaiE:]]

Indeed. Calling PK11_ListCerts happens now 4 times during Cert Manager startup.

My testing shows that 87% of the time is spent within PK11_ListCerts.
8% is spent in Mozilla's sorting code.

Bob, you say we call PK11_ListCerts multiple times to extract all the certs. In
fact we call it with a parameter to request only certain types of certificates,
such as PK11CertListUser or PK11CertListCA.

I can see that the time for PK11_ListCerts(PK11CertListCA) is about 6 times it
takes to call PK11_ListCerts(PK11CertListUser).

Comment 3

[Kai Engert [:KaiE:]]

> I can see that the time for PK11_ListCerts(PK11CertListCA) is about 6 times it
> takes to call PK11_ListCerts(PK11CertListUser).

That happened only once actually, and was an error in measuring. On average, it
is only about 1.5 times.

Comment 4

[Kai Engert [:KaiE:]]

PK11_ListCerts spends 99% of its time in nssToken_TraverseCertificates.

Within nssToken_TraverseCertificates, more than 80% of the time are spent in the
callback, which is nssPKIObjectCollection_AddInstanceAsObject.

Comment 5

[Robert Relyea]

Basically we are iterating over all the certs in the database to find the certs
PSM is requesting. Since PSM sorts out the certs itself anyway, you can call
PK11_ListCerts() once and get all the certs, then sort them into their various
buckets.

Also, the tip of NSS has several PK11_ListCerts() performance enhancements. They
are pretty extensive, so I doubt anyone would want to try to pull them into 7.0,
but you should see quite a gain on startup of the security manager by just
moving to the new calls.

Comment 6

[Kai Engert [:KaiE:]]

I implemented the optimization to only call PK11_ListCerts once.

By doing that, the required time went down to 48%.

While working on the code, I noticed that the sort code is very inefficient.
Whenever it needs to compare two certificates, it allocates from 2-6 strings,
each requiring a NSS allocation and a conversion to unicode.

I implemented a hash table that is used to cache the strings needed during sorting.

Now we are down to 33% of the originally needed time to open the cert manager.

On my P3 500 Mhz, and a 1.5 MB cert db, without the patch it takes 23 seconds to
open. With the patch, it takes about 7.5 seconds.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: Patch v1, down to 48%

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Patch v2, down to 33%

Comment 9

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 10

[Robert Relyea]

I think you have it down to pretty much the minimum you can get on your NSS code
base. Picking up a new version of NSS should get your time down to 1-2 seconds
on a database that size.

bob

Comment 11

[Stephane Saux]

rangan, can you review Kai's patch as Javi is away this week. Please also take a
look to make sure you use the same optimizations in your code.
Thanks,
Stephane.

Comment 12

[Kai Engert [:KaiE:]]

*** Bug 82212 has been marked as a duplicate of this bug. ***

Comment 13

[Rangan Sen]

Comment on attachment 92548 [details] [diff] [review]
Patch v2, down to 33%

seems fine.

Comment 14

[Kai Engert [:KaiE:]]

John, could you please have a look whether my pld hashtable cleanup code is
sufficient?

Comment 15

[Kai Engert [:KaiE:]]

Attachment: Patch v3

I had a chat on IRC with jkeiser. He had a suggestion to avoid some unnecessary
allocations. This does not seem to have optimized the required time, since >90%
of the time is still spent in NSS.

But anyway, its a good idea to avoid unnecessary allocations.

Comment 16

[Kai Engert [:KaiE:]]

Comment on attachment 92985 [details] [diff] [review]
Patch v3

carrying forward r=rangansen. r=jkeiser on hash table cleanup/init code.

Comment 17

[Kai Engert [:KaiE:]]

Jag, can you please review?

Comment 18

[Kai Engert [:KaiE:]]

Attachment: Patch v4

I found a problem with the previous patch.

I changed the signature of LoadCerts, but some JavaScript code still requires
the function with the old interface.

Attaching a fixed patch that supports both methods.

Comment 19

[Rangan Sen]

Comment on attachment 93155 [details] [diff] [review]
Patch v4

r=rangansen

Comment 20

[jag (Peter Annema)]

Comment on attachment 93155 [details] [diff] [review]
Patch v4

sr=jag

Comment 21

[Kai Engert [:KaiE:]]

Fixed on trunk.

Comment 22

[John Unruh]

Verified on Win2000 8/8/02 trunk. I have 11 personal certs, and opening the cert 
manager takes about 1-2 seconds compared with the 8/5 build which takes about 
3-4 seconds.