281868 - workaround inefficient reallocation of PL arenas

Status: NEW

(NSS :: Libraries, enhancement, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

In bug 281866, I explain why NSPR's method of recycling arenas wastes
memory, and that it is a problem when CRLs (which tend to be large, 
on the order of several hundred KB) are used in NSS.  

There are several places in NSS that use a PLArenaPool to hold a CRL,
then free it shortly after using it.  The problem of mismatched arena
sizes would be greatly mitigated if NSS would ensure that these large
arenas are always destroyed, not recycled on the free list, regardless
of whether the arena free list is in use or not.

To that end, I propose to create a new function,  PORT_DestroyArena()
that takes the same arguments as PORT_FreeArena() but always calls 
PL_FinishArenaPool() to destroy the arenapool without recycling any
of the arenas in that pool.  Then I propose to replace certain calls 
to PORT_FreeArena with calls to PORT_DestroyArena, particularly for
those that hold CRLs.  

Are there any objections to this plan?

Comment 1

[Robert Relyea]

Sounds good to me...

bob

Comment 2

[Julien Pierre]

Yes, it's a good idea .
Besides the CRL decoder, I think most of the code that holds CRLs in arenas is
in softoken. I think it might benefit from other improvements as well. I have
seen it do things like copy the entire CRL value from one arena to another, just
over the course of a find objects call.

Comment 3

[Wan-Teh Chang]

Added the father of PLArena to the cc list.
Perhaps other copies of PLArena could also
use this improvement.  jsarena.c doesn't
seem to address this issue.

Comment 4

[Wan-Teh Chang]

Sorry, added Brendan to the wrong bug.

We might want to name this function
PORT_FinishArena instead to mirror the
PLArena function names.  (I admit PLArena's
function names are not good, but at least
the PORT Arena function names will be
consistent with PLArena's.)

Comment 5

[Wan-Teh Chang]

While we are at it, we can simplify the following
code in PORT_FreeArena:

290     if (!pvd) {
291         /* Each of NSPR's DLLs has a function libVersionPoint().
292         ** We could do a lot of extra work to be sure we're calling the
293         ** one in the DLL that holds PR_FreeArenaPool, but instead we
294         ** rely on the fact that ALL NSPR DLLs in the same directory
295         ** must be from the same release, and we call which ever one we get. 
296         */
297         /* no need for thread protection here */
298         pvd = libVersionPoint();
299         if ((pvd->vMajor > 4) || 
300             (pvd->vMajor == 4 && pvd->vMinor > 1) ||
301             (pvd->vMajor == 4 && pvd->vMinor == 1 && pvd->vPatch >= 1)) {
302             const char *ev = PR_GetEnv("NSS_DISABLE_ARENA_FREE_LIST");
303             if (!ev) doFreeArenaPool = PR_TRUE;
304         }
305     }

It's actually wrong to call libVersionPoint()
directly like that.  But since current versions
of NSS can't possibly be used with NSPR releases
before 4.1.1, the NSPR version check can be
simply deleted.

Comment 6

[Wan-Teh Chang]

How about something like PORT_FreeArenaNoRecycle
or PORT_ReallyFreeArena?  I won't be able to figure
out the difference between FreeArena, FinishArena,
and DestroyArena just by looking at their names.
So I think my previous suggestion in comment 4 is
a bad idea.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Putting this bug on my  NSS 3.10 radar.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Maybe this will happen in 3.11.  
For now, we're finding the best performance by disabling NSS's use of NSPR's
arena free list alltogether and using NSPR's "zone allocator" instead.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

remove target milestone, since the target was missed.