1253160 - Reduce the number of heap allocations related to arenas

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nicholas Nethercote [inactive]]

NSS creates *many* PORTArenaPools. Each one involves two heap allocations: one
for the pool itself, and one for its lock. (I've mentioned this before in bug
1095272 comment 0.)

For example, I ran Firefox, opened cnn.com, gmail.com and
treeherder.mozilla.org, and then shut down. These allocations accounted for 14%
of all calls to malloc in the parent process! Here's a histogram that shows the
hot locations for the abovementioned workload.

> 844148 counts:
> (  1)   159658 (18.9%, 18.9%): PORT_NewArena: secname.c:651 2048
> (  2)   102399 (12.1%, 31.0%): PORT_NewArena: certdb.c:309 2048
> (  3)   102367 (12.1%, 43.2%): PORT_NewArena: certdb.c:264 2048
> (  4)    79624 ( 9.4%, 52.6%): PORT_NewArena: certxutl.c:402 2048
> (  5)    79586 ( 9.4%, 62.0%): PORT_NewArena: xbsconst.c:107 2048
> (  6)    40792 ( 4.8%, 66.9%): PORT_NewArena: pcertdb.c:2568 2048
> (  7)    40792 ( 4.8%, 71.7%): PORT_NewArena: pcertdb.c:2574 2048
> (  8)    35360 ( 4.2%, 75.9%): PORT_NewArena: certv3.c:132 2048
> (  9)    30684 ( 3.6%, 79.5%): PORT_NewArena: certdb.c:2492 2048
> ( 10)    26552 ( 3.1%, 82.7%): PORT_NewArena: certdb.c:713 2048
> ( 11)    26552 ( 3.1%, 85.8%): PORT_NewArena: alg1485.c:1401 2048
> ( 12)    26520 ( 3.1%, 89.0%): PORT_NewArena: pk11obj.c:1821 2048
> ( 13)    20565 ( 2.4%, 91.4%): PORT_NewArena: seckey.c:561 2048
> ( 14)    20520 ( 2.4%, 93.8%): PORT_NewArena: pkcs11.c:1688 2048
> ( 15)    20475 ( 2.4%, 96.2%): PORT_NewArena: seckey.c:1103 2048
> ( 16)    20430 ( 2.4%, 98.7%): PORT_NewArena: seckey.c:1421 2048
> ( 17)    10202 ( 1.2%, 99.9%): PORT_NewArena: polcyxtn.c:145 2048

It's not the memory usage that I'm concerned about -- these allocations are
short-lived and small (80 bytes for the pool and 176 bytes for the lock on my
machine) -- but the speed. Heap allocation isn't fast and it uses a global
lock.

Fortunately, in the majority of the above functions the pool's lifetime is
bounded by a function, which means it can be allocated on the stack instead
of the heap. Furthermore, in these cases the pool's use is single-threaded, so
the lock isn't necessary.

Happily, all the PORT_*Arena* functions have the ability to take either a
PORTArenaPool or a vanilla PLArenaPool; switching to the latter will let us
avoid both allocations in the function-bounded, single-threaded cases.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (DRAFT) - Use a stack-allocated PLArenaPool in CERT_DecodeAVAValue()

Here's a proof of concept. I've only converted the highest-frequency case from
comment 0. Also, the patch is against mozilla-central rather than the NSS repo.
(I'll go against the NSS repo for a later version if this one passes muster.)

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8726079 [details] [diff] [review]
(DRAFT) - Use a stack-allocated PLArenaPool in CERT_DecodeAVAValue()

Review of attachment 8726079 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry, but I made things worse by copying this pattern once.  I guess that I could claim that the code wasn't performance sensitive.

This is one case where goto would make sense, but the change you make here seems right.  I had to look into PORT_FreeArena and PR_FreeArenaPool to satisfy myself that you weren't actually changing anything, but this is good.

Do you plan to mechanically change all instances of this, or is this a manual scrub?

Comment 3

[Martin Thomson [:mt:]]

BTW, I don't care if you make a patch against m-c, especially if it means you can use mozreview.  I can land a m-c-based patch in NSS without too much trouble.  If you don't use mozreview, I will request that you push to rietveld for the big patch, which is common for NSS; context is important.

Comment 4

[Nicholas Nethercote [inactive]]

Thank you for the fast feedback.


> Do you plan to mechanically change all instances of this, or is this a
> manual scrub?

It's hard to do it mechanically because you have to look at each case to see if it meets the "function-bounded & single-threaded" criterion. So I plan to manually change the ones in the profile in comment 1, where suitable; it'll be a dozen or so.

If you know of any websites (other than gmail, CNN and Treeherder) that exercise other parts of NSS that would be good to know.


> If you don't use mozreview, I will request that you push to
> rietveld for the big patch, which is common for NSS; context is important.

How do I push to rietveld?

Comment 5

[Martin Thomson [:mt:]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> If you know of any websites (other than gmail, CNN and Treeherder) that
> exercise other parts of NSS that would be good to know.

Not really.  Sites with a lot of SSL connections are always good to look at.  Google is usually bad for that (they tend to run only a couple of fat HTTP/2 pipes.

> > If you don't use mozreview, I will request that you push to
> > rietveld for the big patch, which is common for NSS; context is important.
> 
> How do I push to rietveld?

   https://github.com/rietveld-codereview/rietveld/wiki

I recommend the --oauth2 argument for sure, my last push there was (and yes, I renamed their nasty script):

   rietveld.py --oauth2 --rev nss -i 288360043

You will need to authorize a google account (your @mozilla.com should do for that).

Comment 6

[Nicholas Nethercote [inactive]]

> I had to look into PORT_FreeArena and PR_FreeArenaPool to
> satisfy myself that you weren't actually changing anything, but this is good.

So the patch as written does change behaviour -- the old code looks at NSS_DISABLE_ARENA_FREE_LIST and does PL_FinishArenaPool() if it's set, and PL_FreeArenaPool() if it's unset. My patch just does PL_FreeArenaPool() unconditionally. It's possible this may cause trouble for one of the leak-checking tests? (That seems to be the reason for NSS_DISABLE_ARENA_FREE_LIST's existence.)

I'm actually in the process of removing the NSPR arena freelist (bug 1252639). If that happens, then PL_FreeArenaPool() and PL_FinishArenaPool() will be identical (except for some stats recording) and then we can get rid of NSS_DISABLE_ARENA_FREE_LIST.

With that in mind, I'm inclined to change the patch and use PL_FinishArenaPool() instead. Thoughts?

Comment 7

[Nicholas Nethercote [inactive]]

Perhaps a better idea is to add two thin wrapper functions:

- One for PL_InitArenaPool(), which would always use "security" as the arena name and sizeof(double) for the alignment.

- One for arena destruction, which would consult NSS_DISABLE_ARENA_FREE_LIST and do the appropriate thing.

I'll try that and see how it looks.

Comment 8

[Martin Thomson [:mt:]]

Given that you are going to free the entries in bug 1252639, I don't see why you wouldn't just land that and ditch NSS_DISABLE_ARENA_FREE_LIST.  The only purpose it serves is to expose tests to raw malloc/free rather than use the freelist and bug 1252639 does that already.

The wrappers might still be useful in the sense that it narrows the interface the bulk of NSS uses.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: MozReview Request: Bug 1253160 (part 1) - Remove port_allocFailures, which isn't used usefully. r=mt.

Review commit: https://reviewboard.mozilla.org/r/38045/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38045/

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: MozReview Request: Bug 1253160 (part 2) - Remove four unused function declarations from secport.h. r=mt.

Review commit: https://reviewboard.mozilla.org/r/38047/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38047/

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

This avoids *many* heap allocations in places where arena pools are used in a
function-bounded, single-threaded way.

Review commit: https://reviewboard.mozilla.org/r/38049/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38049/

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8726487 [details]
MozReview Request: Bug 1253160 (part 1) - Remove port_allocFailures, which isn't used usefully. r=mt.

https://reviewboard.mozilla.org/r/38045/#review34585

Yep, assigned but never read.

Comment 13

[Nicholas Nethercote [inactive]]

BTW, about part 3... It converts 9 of the 17 cases from comment 0, which
gets rid of 78% of the heap allocations.

I ended up introducing a new type, PORTStackArenaPool, and two new functions
PORT_{Init,Deinit}StackPool() for init/deinit. 

- I like that the separate type and functions make it obvious when you're
  using the special case.

- I'm ambivalent about the "Deinit" in PORT_DeinitStackPool(), but I wanted to
  avoid "Free" because PORT_Free() and PORT_FreeStackArena() are too similar.
  
- I don't much like all the stubs & _Util junk; I fully admit to cargo-culting
  all that. The fact that nss.symbols is a change outside of NSS is also
  interesting -- would it have to land separately?

Comment 14

[Martin Thomson [:mt:]]

Comment on attachment 8726488 [details]
MozReview Request: Bug 1253160 (part 2) - Remove four unused function declarations from secport.h. r=mt.

https://reviewboard.mozilla.org/r/38047/#review34589

I can't even find definitions for these.

Comment 15

[Ryan Sleevi]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> - I don't much like all the stubs & _Util junk; I fully admit to
> cargo-culting
>   all that. The fact that nss.symbols is a change outside of NSS is also
>   interesting -- would it have to land separately?

You don't have to cargo-cult this for new functions. It merely existed when libutil was created and we shuffled symbols into it, to make sure you could mix and match libraries.

You should be fine adding it to libutil and the .def file therein

Comment 16

[Martin Thomson [:mt:]]

Comment on attachment 8726489 [details]
MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

https://reviewboard.mozilla.org/r/38049/#review34591

A few gripes, but nothing major.  I marked all the places where the stubbing came into play.  You should just be able to revert those files.

I think that you might want to use `PORT_DestroyStackArena` instead of `PORT_DeinitStackArena`.  That's a convention we've used elsewhere.

For landing in NSS, I will need you to split the commit so that config/external/nss/nss.symbols is in a separate patch.  That can be landed separately once NSS is updated in m-c.

::: config/external/nss/nss.symbols:455
(Diff revision 1)
> +PORT_DeinitStackArena_Util

Remove `_Util` variant.

::: config/external/nss/nss.symbols:463
(Diff revision 1)
> +PORT_InitStackArena_Util

Remove `_Util` variant.

::: security/nss/lib/freebl/stubs.h:26
(Diff revision 1)
> +#define PORT_DeinitStackArena  PORT_DeinitStackArena_stub

remove

::: security/nss/lib/freebl/stubs.h:30
(Diff revision 1)
> +#define PORT_InitStackArena  PORT_InitStackArena_stub

remove

::: security/nss/lib/freebl/stubs.c:110
(Diff revision 1)
> +STUB_DECLARE(void,PORT_DeinitStackArena_Util,(PORTStackArenaPool *arena));

remove

::: security/nss/lib/freebl/stubs.c:114
(Diff revision 1)
> +STUB_DECLARE(void,PORT_InitStackArena_Util,(PORTStackArenaPool *arena,
> +                                            unsigned long chunksize));

remove

::: security/nss/lib/freebl/stubs.c:228
(Diff revision 1)
> +extern void
> +PORT_InitStackArena_stub(PORTStackArenaPool *arena, unsigned long chunksize)
> +{
> +    STUB_SAFE_CALL2(PORT_InitStackArena_Util, arena, chunksize);
> +    abort();
> +    return NULL;
> +}
> +

remove

::: security/nss/lib/freebl/stubs.c:262
(Diff revision 1)
> +extern void
> +PORT_DeinitStackArena_stub(PORTStackArenaPool *arena)
> +{
> +    STUB_SAFE_CALL1(PORT_DeinitStackArena_Util, arena);
> +    abort();
> +    return NULL;
> +}

remove

::: security/nss/lib/freebl/stubs.c:614
(Diff revision 1)
> +    STUB_FETCH_FUNCTION(PORT_InitStackArena_Util);
> +    STUB_FETCH_FUNCTION(PORT_DeinitStackArena_Util);

remove

::: security/nss/lib/nss/utilwrap.c:65
(Diff revision 1)
> +#undef PORT_DeinitStackArena

remove

::: security/nss/lib/nss/utilwrap.c:69
(Diff revision 1)
> +#undef PORT_InitStackArena

remove

::: security/nss/lib/nss/utilwrap.c:203
(Diff revision 1)
> +void
> +PORT_InitStackArena(PORTStackArenaPool *arena, unsigned long chunksize)
> +{
> +    PORT_InitStackArena_Util(arena, chunksize);
> +}
> +
> +void
> +PORT_DeinitStackArena(PORTStackArenaPool *arena)
> +{
> +    PORT_DeinitStackArena_Util(arena);
> +}
> +

remove

::: security/nss/lib/util/nssutil.def:70
(Diff revision 1)
> +PORT_DeinitStackArena_Util;

remove

::: security/nss/lib/util/nssutil.def:74
(Diff revision 1)
> +PORT_InitStackArena_Util;

remove

::: security/nss/lib/util/secport.c:344
(Diff revision 1)
> +    if (!checkedEnv) {
> +	doFreeArenaPool = (PR_GetEnvSecure("NSS_DISABLE_ARENA_FREE_LIST") == NULL);
> +	checkedEnv = PR_TRUE;
> +    }

I hate to do this to you, but this is super-unsafe thread-wise.  Can you use PR_CallOnce instead here?

See an example here:

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslsecur.c#927-931

(Ignore the pristine nonsense there, that probably isn't thread-safe either.)

::: security/nss/lib/util/utilrename.h:60
(Diff revision 1)
> +#define PORT_DeinitStackArena PORT_DeinitStackArena_Util

Remove this

::: security/nss/lib/util/utilrename.h:64
(Diff revision 1)
> +#define PORT_InitStackArena PORT_InitStackArena_Util

Remove this too

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8726487 [details]
MozReview Request: Bug 1253160 (part 1) - Remove port_allocFailures, which isn't used usefully. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38045/diff/1-2/

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8726488 [details]
MozReview Request: Bug 1253160 (part 2) - Remove four unused function declarations from secport.h. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38047/diff/1-2/

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8726489 [details]
MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38049/diff/1-2/

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r=mt.

Review commit: https://reviewboard.mozilla.org/r/38087/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38087/

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 8726488 [details]
MozReview Request: Bug 1253160 (part 2) - Remove four unused function declarations from secport.h. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38047/diff/2-3/

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8726489 [details]
MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38049/diff/2-3/

Comment 23

[Nicholas Nethercote [inactive]]

Comment on attachment 8726566 [details]
MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38087/diff/1-2/

Comment 24

[Martin Thomson [:mt:]]

Comment on attachment 8726489 [details]
MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

https://reviewboard.mozilla.org/r/38049/#review34617

::: security/nss/lib/util/nssutil.def:286
(Diff revision 3)
> +;+NSSUTIL_3.23 {       # NSS Utilities 3.23 release

This will be 3.24 now.

::: security/nss/lib/util/secport.c:306
(Diff revision 3)
> +setupUseFreeList()

I think that you might have to declare this as
```
static PRStatus
setupUseFreeList(void)
```
to avoid portability issues.

Comment 25

[Martin Thomson [:mt:]]

Comment on attachment 8726566 [details]
MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r=mt.

https://reviewboard.mozilla.org/r/38087/#review34621

Comment 26

[Martin Thomson [:mt:]]

Oh, the thought occurred, if you want some heavy-duty testing of NSS and TLS connections, try running our unit tests.  Those do a few hundred.

If you have the latest nspr and nss checked out in the same directory (i.e., from the NSS root, ../nspr contains an nspr release), build nss by running `USE_64=1 make nss_build_all` in the nss directory.  The unit tests can then be run from nss/test/ssl_gtest (i.e., `USE_64=1 ./ssl_gtests.sh`).  You can rerun the actual binary against the output with a debugger or profiler, e.g.,

  nss/../dist/<uname -r...>/bin/ssl_gtests -d nss/../tests_results/localhost.<number>/ssl_gtests

...or just modify the runner script, but that does a bunch of certificate generation you might not want to repeat each time.

Comment 27

[Nicholas Nethercote [inactive]]

Comment on attachment 8726489 [details]
MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38049/diff/3-4/

Comment 28

[Nicholas Nethercote [inactive]]

Comment on attachment 8726566 [details]
MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38087/diff/2-3/

Comment 29

[Nicholas Nethercote [inactive]]

Thank you for the fast reviews. I've addressed all the review comments.

mt, is there anything else I should do? Are you ok to take it from here?

Comment 30

[Martin Thomson [:mt:]]

I've sent an email to nss-dev.  If no one screams, I will land the changes.

What will likely happen is that the first three of these will be part of NSS 3.24.  Once that lands in m-c, we will need to land the last piece there.  We have a lot going in to 3.24 (mostly deleting code :), but I expect to have a beta version mid-to-early 48.

Comment 31

[Wan-Teh Chang]

This is very good work to reduce the number of heap allocations.
I understand that to work with existing NSS code such as
SEC_QuickDERDecodeItem, you cannot just use PLArenaPool directly.

I suggest some changes.

1. "StackArena" is poorly named. I thought it used alloca().
I would call it "PlainArena" or "VanillaArena". This struct
can be a member of some other struct that is usually heap
allocated. Many NSS structs don't have a lock member, and
we didn't name those structs StackFoo.

2. I hope I misunderstand your patch. If PORTStackArenaPool
is just a typedef of PLArenaPool, isn't it wrong to pass it
to PORT_ArenaAlloc()? The first thing PORT_ArenaAlloc() does
is to cast the |arena| argument to a PORTArenaPool and read
its |magic| field.  It seems that that will be an invalid
read.

Comment 32

[Nicholas Nethercote [inactive]]

> 2. I hope I misunderstand your patch. If PORTStackArenaPool
> is just a typedef of PLArenaPool, isn't it wrong to pass it
> to PORT_ArenaAlloc()? The first thing PORT_ArenaAlloc() does
> is to cast the |arena| argument to a PORTArenaPool and read
> its |magic| field.  It seems that that will be an invalid
> read.

All the PORT_Arena* functions are unusual because they can take either a vanilla PLArenaPool *or* a PORT_ArenaPool, which is an extended version of PLArenaPool that adds locking. The |magic| field is used within the PORT_Arena* functions to determine which kind of arena pool is being used. It's an unusual design, but given that it's in place I'm taking advantage of it to convert some PORT_ArenaPools to vanilla PLArenaPools.

Comment 33

[Wan-Teh Chang]

Hi Nicholas,

As far as I can tell, your patch does not modify the PORT_ArenaAlloc()
function.  That function looks like this:

void *
PORT_ArenaAlloc(PLArenaPool *arena, size_t size)
{
    void *p = NULL;

    PORTArenaPool *pool = (PORTArenaPool *)arena;

    if (size <= 0) {
        size = 1;
    }

    if (size > MAX_SIZE) {
        /* you lose. */
    } else
    /* Is it one of ours?  Assume so and check the magic */
    if (ARENAPOOL_MAGIC == pool->magic ) {
        PZ_Lock(pool->lock);

If |arena| points at a PORTStackArenaPool, isn't pool->magic
an invalid read?

Comment 34

[Nicholas Nethercote [inactive]]

>     /* Is it one of ours?  Assume so and check the magic */
>     if (ARENAPOOL_MAGIC == pool->magic ) {
>         PZ_Lock(pool->lock);
> 
> If |arena| points at a PORTStackArenaPool, isn't pool->magic
> an invalid read?

It does read four bytes past the end of the struct. Whether that's invalid depends on... what other local variable are on the stack, possibly compiler optimizations, and possibly which direction the stack grows. Not good.

So now I'm wondering how this is supposed to work. I'll take a closer look on Monday to see if there are any existing cases where the |magic| test fails, and if so, whether they exhibit this same problem or get around it somehow.

Either way, it should be possible to do something like this:

> typedef struct PORTStackArenaPool_str {
>   PLArenaPool arena;
>   PRUint32    magic;
> } PORTStackArenaPool;

to avoid reading past the end of the struct.

mt, in the meantime, please hold off from landing these patches. Thank you.

Comment 35

[Wan-Teh Chang]

https://reviewboard.mozilla.org/r/38049/#review34919

::: security/nss/lib/util/secport.h:97
(Diff revision 4)
> +extern void PORT_DestroyStackArena(PORTStackArenaPool* arena);

This comment is just a commentary, not a request for change.

1. The Init and Destroy function names match existing NSS functions (e.g., AES_InitContext and AES_DestroyContext). They also match a common naming convention used in other libraries, for example, pthread_mutex_init and pthread_mutex_destroy.

2. I think it pays to come up with a better name than PORTStackArenaPool because PORTStackArenaPool can be misinterpreted to mean an arena pool that allocates memory from the stack (as opposed to the intended meaning of a PLArenaPool struct allocated on the stack). Also, "allocated on the stack" is overly restrictive. The only requirement is that the caller rather than NSS is responsible for allocating the PLArenaPool struct. I would probably emphasize the no-lock or the no-value-add aspect and call this PORTUnlockedArenaPool or PORTPlainArenaPool

::: security/nss/lib/util/secport.c:306
(Diff revision 4)
> +setupUseFreeList(void)

Please capitalize this function name "SetupUseFreeList" so that it looks like a function.

::: security/nss/lib/util/secport.c:330
(Diff revision 4)
> -    if (!checkedEnv) {
> +    if (PR_SUCCESS == PR_CallOnce(&setupUseFreeListOnce,

PR_CallOnce can only fail if the one-time initialization function fails. (We should improve the PR_CallOnce documentation if this point is not obvious.) PR_CallOnce doesn't even check if its input arguments are null pointers. Even if PR_CallOnce starts to perform null pointer checks on its input arguments in the future, that won't affect this PR_CallOnce call.

Since setupUseFreeList() always returns PR_SUCCESS, this PR_CallOnce call always returns PR_SUCCESS. You can safely ignore the return value of this PR_CallOnce call and just use the useFreeList global variable directly.

Comment 36

[Wan-Teh Chang]

(In reply to Nicholas Nethercote [:njn] from comment #34)
> 
> ..., it should be possible to do something like this:
> 
> > typedef struct PORTStackArenaPool_str {
> >   PLArenaPool arena;
> >   PRUint32    magic;
> > } PORTStackArenaPool;
> 
> to avoid reading past the end of the struct.

Under your design constraints, I think this is the best solution.
It also seems like a good idea to use a nonzero magic value for
PORTStackPrenaPool.

P.S. I am glad I wasn't hallucinating. When two engineers I
respect didn't see the problem, it made me wonder if I missed
something subtle.

Comment 37

[Nicholas Nethercote [inactive]]

Comment on attachment 8726489 [details]
MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38049/diff/4-5/

Comment 38

[Nicholas Nethercote [inactive]]

Comment on attachment 8726566 [details]
MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38087/diff/3-4/

Comment 39

[Nicholas Nethercote [inactive]]

mt: I think parts 1--3 are ready to land in the NSS repo. Are you able to do it?

Comment 40

[Martin Thomson [:mt:]]

Attachment: bug1253160p3.patch

This didn't compile for me.  Can you check that I've fixed PORT_DestroyCheapArena correctly?

Comment 41

[Wan-Teh Chang]

Comment on attachment 8728853 [details] [diff] [review]
bug1253160p3.patch

Review of attachment 8728853 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/certdb/alg1485.c
@@ +1411,5 @@
>      PORT_Assert(maxLen);
>      if (!maxLen)
>          maxLen = 2000; /* a guess, should never happen */
>  
> +    pBuf = addrBuf = (char*)PORT_ArenaZAlloc(&tmpArena, maxLen + 1);

We need to pass &tmpArena.arena instead of &tmpArena to PORT_Arena* and
QuickDER functions.

::: lib/util/secport.h
@@ +78,5 @@
> + * All the other PORT_Arena* functions will operate safely with either
> + * subclass.
> + */
> +typedef struct PORTCheapArenaPool_str {
> +  PLArenaPool arena;

Nit: (I know you copied this member name from the PORTArenaPool struct
in the original code.) In NSPR, a PLArenaPool is usually named "pool"
because there is also a PLArena struct.

To be precise, I think this member should be named "pool", "basePool",
or "base"/"super" (which suggests "base class").

Comment 42

[Nicholas Nethercote [inactive]]

Comment on attachment 8726489 [details]
MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38049/diff/5-6/

Comment 43

[Nicholas Nethercote [inactive]]

Comment on attachment 8726566 [details]
MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38087/diff/4-5/

Comment 44

[Nicholas Nethercote [inactive]]

The updated patches address both mt's and wtc's comments.

Comment 45

[Wan-Teh Chang]

Hi Nicholas,

After looking at your new patch, I don't like my suggestion of "base".
The main reason is that it's not clear what "base" means. (Renaming
it to "baseClass" would address that issue.) Another reason is that
"base" can confuse people who are very familiar with PLArena code
because the PLArena struct has a member named "base":
22 struct PLArena {
23     PLArena     *next;          /* next arena for this lifetime */
24     PRUword     base;           /* aligned base address, follows this header */
25     PRUword     limit;          /* one beyond last byte in arena */
26     PRUword     avail;          /* points to next available byte */
27 };

I now suggest we just name that member "arena". This variable/argument
name is widespread in NSS. Even though it's not precise, I think it's
better to be consistent. Sorry about the trouble.

Comment 46

[Nicholas Nethercote [inactive]]

Comment on attachment 8726489 [details]
MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38049/diff/6-7/

Comment 47

[Nicholas Nethercote [inactive]]

Comment on attachment 8726566 [details]
MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r=mt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38087/diff/5-6/

Comment 48

[Nicholas Nethercote [inactive]]

Try again?

Comment 49

[Martin Thomson [:mt:]]

remote:   https://hg.mozilla.org/projects/nss/rev/223b3a4eb825
remote:   https://hg.mozilla.org/projects/nss/rev/396480150b3f
remote:   https://hg.mozilla.org/projects/nss/rev/eb039754d734

We'll need to wait until NSS lands in m-c before landing the last part.

Comment 50

[Nicholas Nethercote [inactive]]

Thank you!

Comment 51

[Martin Thomson [:mt:]]

Landed a followup to suppress some coverity issues: https://hg.mozilla.org/projects/nss/rev/1c0562988595

Comment 52

[Nicholas Nethercote [inactive]]

mt, do you want to leave this open until the part 4 is landed?

Comment 53

[Nicholas Nethercote [inactive]]

(Sorry, I didn't mean to resolve this bug.)

Comment 54

[Martin Thomson [:mt:]]

(In reply to Nicholas Nethercote [:njn] from comment #52)
> mt, do you want to leave this open until the part 4 is landed?

Yeah, I don't think that it's fixed until it's fixed in Firefox.  Unless you want to open a separate bug for that.

Comment 55

[Martin Thomson [:mt:]]

Closing this now that we have 3.24 in Firefox proper.