1252639 - Remove the shared PLArena freelist

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Nicholas Nethercote [inactive]]

plarena.c has a shared freelist for arenas. When an arena is freed via PL_ArenaRelease() or PL_FreeArenaPool() (but *not* via PL_FinishArenaPool()) it gets put on the freelist, which requires getting a lock. Subsequent arena allocations are obtained from the freelist when possible.

This adds complexity. It makes things confusing when investigating pool usage, because pools can end up with arenas larger or smaller than they're "supposed" to get. (This has tripped me up more than once.)

The freelist almost certainly doesn't help performance, but it may increase memory usage, because it's making decisions that the heap allocator is likely to do a better job at, given that heap allocator has a global view of things.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove the shared PLArena freelist

glandium: I'm not expecting you to review this until you are feeling better :)

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Move contents of plarenas.h into plarena.h

Because the plarenas.h/plarena.h is strange and confusing. But the patch keeps
plarenas.h around in a way that the API doesn't change -- any code that
included plarena.h or plarenas.h should not need changing.

Comment 3

[Nicholas Nethercote [inactive]]

Note to self: there are some tweaks in NSS that can be made as a result of this, such as removing NSS_DISABLE_ARENA_FREE_LIST.

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8725433 [details] [diff] [review]
(part 2) - Move contents of plarenas.h into plarena.h

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

::: nsprpub/lib/ds/plarenas.h
@@ +5,5 @@
>  
> +// PLArena-related declarations used to be split between plarenas.h and
> +// plarena.h. That split wasn't useful, so now all the declarations are in
> +// plarena.h. However, this file still exists so that any old code that
> +// includes it will still work.

You probably want to use /* */ here, we've had complaints about // comments.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Move contents of plarenas.h into plarena.h

Now with C-style comments only.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8725431 [details] [diff] [review]
(part 1) - Remove the shared PLArena freelist

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

::: nsprpub/lib/ds/plarena.c
@@ -280,5 @@
> -#endif
> -
> -    if (reallyFree) {
> -        do {
> -            *ap = a->next;

On the first iteration, this would do:
  head->next = a->next;
effectively removing a from the linked list. Combined with the loop, all in all this means we do:
  head->next = NULL;
Although the loop kinda sorta makes things a little bit safer while the loop is still running.

I don't see you doing this in the new code, effectively opening the door for use-after-free.

Comment 7

[Nicholas Nethercote [inactive]]

> I don't see you doing this in the new code, effectively opening the door for
> use-after-free.

Ugh, good catch. New patch coming shortly.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove the shared PLArena freelist

This version explicitly nulls head->next before the loop in FreeArenaList().

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c661f2c5cd878c26e25abc0256b4893d8a249b49
Bug 1252639 (part 1) - Remove the shared PLArena freelist. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf22fcece06e85cd195d61e37382ffc0084e4d05
Bug 1252639 (part 2) - Move contents of plarenas.h into plarena.h. r=glandium.

Comment 10

[:Cykesiopka]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> c661f2c5cd878c26e25abc0256b4893d8a249b49
> Bug 1252639 (part 1) - Remove the shared PLArena freelist. r=glandium.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> bf22fcece06e85cd195d61e37382ffc0084e4d05
> Bug 1252639 (part 2) - Move contents of plarenas.h into plarena.h.
> r=glandium.

Shouldn't this have landed upstream at https://hg.mozilla.org/projects/nspr first?

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce12c108f9c4294c51ccb9df1a1b6b124e1ce003
Backout c661f2c5cd87 and bf22fcece06e (from bug 1252639) because NSPR changes aren't supposed to land on mozilla-inbound.

Comment 12

[Nicholas Nethercote [inactive]]

> Shouldn't this have landed upstream at https://hg.mozilla.org/projects/nspr
> first?

Sorry, yes. My mistake. I've backed it out and I will land to the NSPR repo instead.

Comment 13

[Nicholas Nethercote [inactive]]

> I will land to the NSPR repo instead.

Except I don't have permission to push to the NSPR repo! Argh.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove the shared PLArena freelist

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Move contents of plarenas.h into plarena.h

Comment 16

[Nicholas Nethercote [inactive]]

ted: I've updated the patches so they apply cleanly to the NSPR repo, and carried over glandium's r+s. I don't have permission to push patches to NSPR. Could you please do it for me?

Comment 17

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/projects/nspr/rev/936da2a62c9949b033682df5086a22fbf6e68172
Bug 1252639 (part 1) - Remove the shared PLArena freelist. r=glandium.

https://hg.mozilla.org/projects/nspr/rev/a738c6ec6475537abc88f94478c677c8fe6d5b81
Bug 1252639 (part 2) - Move contents of plarenas.h into plarena.h. r=glandium.

Comment 18

[(not currently active) Ted Mielczarek]

FYI, I am amenable to simultaneous landing of patches to mozilla-central and NSPR, as long as you've done something to ensure someone will land them in the NSPR repo (your needinfo? here was perfect). Also I should find out if glandium has the right bits set to push to the nspr repo, what with him being a peer. :)

Comment 19

[Nicholas Nethercote [inactive]]

I'm happy to wait for the next NSPR merge. Thank you for landing the patches.