Closed Bug 1253160 Opened 6 years ago Closed 5 years ago

Reduce the number of heap allocations related to arenas

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox47 affected)

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(4 files, 2 obsolete files)

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.
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.)
Attachment #8726079 - Flags: feedback?(martin.thomson)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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?
Attachment #8726079 - Flags: feedback?(martin.thomson) → feedback+
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.
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?
(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).
> 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?
Flags: needinfo?(martin.thomson)
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.
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.
Flags: needinfo?(martin.thomson)
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/
Attachment #8726489 - Flags: review?(martin.thomson)
Attachment #8726079 - Attachment is obsolete: true
Attachment #8726488 - Flags: review?(martin.thomson)
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.
Attachment #8726487 - Flags: review?(martin.thomson) → review+
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?
Attachment #8726488 - Flags: review?(martin.thomson) → review+
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.
(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 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
Attachment #8726489 - Flags: review?(martin.thomson)
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 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 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/
Attachment #8726489 - Attachment description: MozReview Request: Bug 1253160 (part 3) - Introduce PORTStackArenaPool and init/deinit functions. r=mt. → MozReview Request: Bug 1253160 (part 3) - Introduce PORTStackArenaPool and init/deinit functions. r?mt.
Attachment #8726489 - Flags: review?(martin.thomson)
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/
Attachment #8726488 - Attachment description: MozReview Request: Bug 1253160 (part 2) - Remove four unused function declarations from secport.h. → MozReview Request: Bug 1253160 (part 2) - Remove four unused function declarations from secport.h. r=mt.
Attachment #8726489 - Attachment description: MozReview Request: Bug 1253160 (part 3) - Introduce PORTStackArenaPool and init/deinit functions. r?mt. → MozReview Request: Bug 1253160 (part 3) - Introduce PORTStackArenaPool and init/deinit functions. r=mt.
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 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/
Attachment #8726566 - Attachment description: MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r?mt. → MozReview Request: Bug 1253160 (part 4) - Add PORT_{Init,Destroy}StackArena to nss.symbols. r=mt.
Attachment #8726489 - Flags: review?(martin.thomson) → review+
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 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
Attachment #8726566 - Flags: review?(martin.thomson) → review+
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 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 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/
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?
Flags: needinfo?(martin.thomson)
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.
Flags: needinfo?(martin.thomson)
Depends on: 1253525
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.
> 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.
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?
>     /* 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.
Flags: needinfo?(martin.thomson)
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.
(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 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/
Attachment #8726489 - Attachment description: MozReview Request: Bug 1253160 (part 3) - Introduce PORTStackArenaPool and init/deinit functions. r=mt. → MozReview Request: Bug 1253160 (part 3) - Introduce PORTCheapArenaPool and init/deinit functions. r=mt.
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/
mt: I think parts 1--3 are ready to land in the NSS repo. Are you able to do it?
Flags: needinfo?(martin.thomson)
Flags: needinfo?(martin.thomson)
Attached patch bug1253160p3.patch (obsolete) — Splinter Review
This didn't compile for me.  Can you check that I've fixed PORT_DestroyCheapArena correctly?
Flags: needinfo?(martin.thomson)
Attachment #8728853 - Flags: superreview?(n.nethercote)
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 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 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/
The updated patches address both mt's and wtc's comments.
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 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 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/
Attachment #8728853 - Attachment is obsolete: true
Attachment #8728853 - Flags: superreview?(n.nethercote)
Try again?
Flags: needinfo?(martin.thomson)
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.
Flags: needinfo?(martin.thomson)
Thank you!
Landed a followup to suppress some coverity issues: https://hg.mozilla.org/projects/nss/rev/1c0562988595
mt, do you want to leave this open until the part 4 is landed?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(Sorry, I didn't mean to resolve this bug.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Closing this now that we have 3.24 in Firefox proper.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.