Last Comment Bug 158242 - PK11_PutCRL is very memory inefficient
: PK11_PutCRL is very memory inefficient
Status: RESOLVED FIXED
: perf
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.5
: All All
: P2 normal (vote)
: 3.12
Assigned To: Alexei Volkov
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-07-18 20:28 PDT by Julien Pierre
Modified: 2008-03-14 17:10 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
the fix (1.23 KB, patch)
2005-03-18 10:22 PST, Alexei Volkov
no flags Details | Diff | Splinter Review
skip extra data copy (3.40 KB, patch)
2006-10-09 15:53 PDT, Alexei Volkov
nelson: review+
julien.pierre: review+
Details | Diff | Splinter Review
skipping crl entry decoding decoding (987 bytes, patch)
2007-02-22 15:59 PST, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review

Description Julien Pierre 2002-07-18 20:28:05 PDT
When storing a new CRL to a token, the memory usage grows significantly.
Tasks manager shows the process memory for crlutil going from 170 MB to 270 MB
at the time of importing a 25 MB DER CRL to the softoken. This is roughly 4x the
size of the DER CRL. I have not yet traced where the successive copies of the
DER are being made exactly.

The call to PK11_PutCRL is being made from crl_storeCRL in crl.c .
Comment 1 Wan-Teh Chang 2002-07-18 21:03:51 PDT
Assigned the bug to Julien.
Comment 2 Julien Pierre 2002-09-19 19:07:39 PDT
With the current version of the tip, the peak memory for this test is 157 MB.
This is still too much, but a lot less than when the bug was first reported.
Comment 3 Wan-Teh Chang 2002-10-29 15:05:01 PST
Julien, could you set some criteria for this bug
to be considered fixed?
Comment 4 Julien Pierre 2002-10-29 15:29:15 PST
Wan-Teh,

Ideally, the process shouldn't grow to more than 2x or 3x the size of the CRL to
be stored. That would amount to somewhere between 55 and 80 MB.
Comment 5 Wan-Teh Chang 2002-12-06 15:45:54 PST
Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2003-05-09 21:20:12 PDT
Remove target milestone of 3.8, since these bugs didn't get into that release.
Comment 7 Julien Pierre 2003-05-12 16:38:34 PDT
Performance enhancement. Reprioritizing to P3.
Comment 8 Alexei Volkov 2005-03-17 13:56:14 PST
The additional copies of der crl are done in two functions, NewDBCrlEntry and
WriteDBCrlEntry, that get called from nsslowcert_UpdateCrl.

First one creates certDBEntryRevocation object and copies derCrl.data into its
derCrl. Then in the next line of nsslowcert_UpdateCrl we call WriteDBCrlEntry
which makes another copy by calling EncodeDBCrlEntry function.

The problem can be fixed by introducing a copy flag, similar to
CRL_DECODE_DONT_COPY_DER, like it is done in
CERT_DecodeDERCrlWithFlags.
Comment 9 Julien Pierre 2005-03-17 14:23:37 PST
Alexei,

The code you mentioned is in the softoken. Where would that new option be
getting passed from/to ? Are you sure a new option is required ? I think we
should try to get the code optimized without a new option, if possible.
Comment 10 Alexei Volkov 2005-03-17 14:48:14 PST
Sorry for not clarifying it earlier.

I propose the modification of NewDBCrlEntry function parameters. But you are
right: this is a static function and it is called only from one place. We can
modify the body of the function instead of modification of parameters and
use the same memory derCrl.data is pointing to for certDBEntryRevocation
object.

*** pcertdb.c.~1.48.~	2004-04-25 08:03:16.000000000 -0700
--- pcertdb.c	2005-03-17 14:44:42.000000000 -0800
***************
*** 1268,1274 ****
  
  	
      entry->derCrl.len = derCrl->len;
!     PORT_Memcpy(entry->derCrl.data, derCrl->data, derCrl->len);
  
      return(entry);
  
--- 1268,1274 ----
  
  	
      entry->derCrl.len = derCrl->len;
!     entry->derCrl.data = derCrl->data;
  
      return(entry);
  
Comment 11 Julien Pierre 2005-03-17 15:19:54 PST
Alexei,

Doesn't this change create a memory leak ?
If there was a memcpy, presumably entry->derCrl.data was already allocated. So,
just assigning the variable would leak the buffer.
Please show more of the code in future diffs. 10 lines of context (cvs -q diff
-u10) would be good.
Comment 12 Nelson Bolyard (seldom reads bugmail) 2005-03-17 23:50:01 PST
Even for tiny little diff patches like the one in comment 10, it's a good 
idea to attach them as patches to the bug.  That way, the reviewer can 
get bugzilla to display as much or as little context as he wants.  
Comment 13 Alexei Volkov 2005-03-18 10:21:22 PST
I didn't mean to include whole fix into the report. Memory allocation 
for derCrl.data will be removed as well. The patch is attached.
Comment 14 Alexei Volkov 2005-03-18 10:22:50 PST
Created attachment 177895 [details] [diff] [review]
the fix
Comment 15 Robert Relyea 2005-03-22 15:01:52 PST
Comment on attachment 177895 [details] [diff] [review]
the fix

We need to add a comment to NewDBCrlEntry and nsslowcert_UpdateCrl that the
entry must be destroyed before the derCRL goes out of scope. 

It might be better to make 'NewDBCrlEntry' a function at takes a static
CrlEntry and initializes all the fields (setting arena to NULL).
Comment 16 Julien Pierre 2005-07-07 21:47:22 PDT
Reassigning this bug to Alexei.
Comment 17 Alexei Volkov 2006-10-09 15:53:58 PDT
Created attachment 241761 [details] [diff] [review]
skip extra data copy

avoid memory allocation for certDBEntryRevocation data
Comment 18 Nelson Bolyard (seldom reads bugmail) 2006-10-09 16:53:42 PDT
Comment on attachment 241761 [details] [diff] [review]
skip extra data copy

This eliminates one allocation and copy.  
Julien, is this sufficient?
Comment 19 Julien Pierre 2006-10-12 19:38:49 PDT
Comment on attachment 241761 [details] [diff] [review]
skip extra data copy

This fix is good. I can't answer whether this is sufficient. When I filed the bug, I had been given a very large CRL (26 MB) that took CMS 4 hours to generate. I had then imported with crlutil, and noted that the process memory exploded - it allocated it about 6 times. PK11_PutCRL was one of the offenders.
I would suggest creating a large CRL again now that crlutil can do so, in much less time. Then observe the process VM size and see how many times the CRL size it is, and how much it grows from the time of before the call to PK11_PutCRL to after it returns.
I think ideally there should be zero copy in that function and thus little process growth. I'm not sure if the PKCS#11 layering actually allows that, but I think so.
Comment 20 Alexei Volkov 2007-01-04 12:28:40 PST
/cvsroot/mozilla/security/nss/lib/softoken/pcertdb.c,v  <--  pcertdb.c
new revision: 1.64; previous revision: 1.63

The patch eliminates one out of four in memory copies of importing CRL. 
Comment 21 Alexei Volkov 2007-02-22 15:59:44 PST
Created attachment 256105 [details] [diff] [review]
skipping crl entry decoding decoding

skipping entry decoding will be more efficient.
Comment 22 Nelson Bolyard (seldom reads bugmail) 2007-02-24 01:50:12 PST
Comment on attachment 256105 [details] [diff] [review]
skipping crl entry decoding decoding

Alexei, have you committed the previously reviewed patch yet?
Comment 23 Alexei Volkov 2007-02-27 10:40:27 PST
previous patch was committed into trunk (see comment #20).
Comment 24 Alexei Volkov 2008-03-14 17:10:28 PDT
No additional work is foreseen for this bug. Closing..

Note You need to log in before you can comment on or make changes to this bug.