715770 - Kill nsRecyclingAllocator

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

Spun off from bug 683080.

nsRecyclingAllocator is an allocator that holds onto up to N free()'d blocks in the hope that they may be recycled again soon, where N is determined when it's created.  Each allocator also has a time-out, T.  If no allocations are done for T seconds, then all the held blocks are freed at once.

We have two uses of nsRecyclingAllocator, AFAICT:
- One in nsIOService which has the 15 minute(!) time-out;  this is the necko buffer cache.
- One in nsZipArchive.cpp which has the default 10 second time-out.

I want to get rid of nsRecyclingAllocator.  Given that a good allocator should handle this kind of allocation pattern well, it wouldn't surprise me at all if nsRecyclingAllocator isn't helping performance, and may even be hurting it.  Especially if we're doing stupid things like using 15 minute time-outs for large (800KB+) allocations.

(Custom allocators often do make things worse, see http://www.mendeley.com/research/reconsidering-custom-memory-allocation/ for examples.)

Another downside of using nsRecyclingAllocator is that it clownshoe-ishly adds an extra an extra word to every allocation.  For the necko buffer cache, this causes each 32KB request to become (32KB + 1 word), which jemalloc rounds up to 36KB.  With 24 chunks in the necko cache (the default) this wastes almost 24*4KB == 96KB of memory.

For the necko buffer cache, when starting the browser and loading Gmail and TechCrunch there are ~900 allocations, ~870 of which are recycled.  That's few enough that jemalloc is likely to do a fine job by itself.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch 1: remove necko buffer cache

This patch just removes gBufferCache.  Pretty simple, the only controversial part is whether it affects performance.  I haven't done any Talos runs;  any other suggestions on how to measure this are welcome.

Oh, actually, the "network.buffer.cache.count" and "network.buffer.cache.size" options are misnamed after this patch is applied.  Better names would be "network.segment.count" and "network.segment.size", but I don't know if changing them would cause any problems.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch 2: don't use nsRecyclingAllocator in libjar

Also pretty simple.  Taras, in bug 523065 comment 1 you asked if the nsRecyclingAllocator had any benefit in libjar.  Bug 523065 comment 2 gave a trace as evidence that it did, but most of the entries in that trace are for the necko buffer cache -- less than 100 are for libjar.  Peanuts!  But again, any suggestions you have for verifying that this doesn't hurt perf would be welcome.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch 3: remove nsRecyclingAllocator

Just a bunch of mopping up.  diffstat for the three patches combined says:

 17 files changed, 9 insertions(+), 658 deletions(-)

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 586327 [details] [diff] [review]
patch 1: remove necko buffer cache

watching talos sounds good to me

Comment 5

[(dormant account)]

Comment on attachment 586328 [details] [diff] [review]
patch 2: don't use nsRecyclingAllocator in libjar

I'm happy to see placebos go away, but I don't see any libjar code here.

Comment 6

[(dormant account)]

(In reply to Taras Glek (:taras) from comment #5)
> Comment on attachment 586328 [details] [diff] [review]
> patch 2: don't use nsRecyclingAllocator in libjar
> 
> I'm happy to see placebos go away, but I don't see any libjar code here.

looks like you uploaded the same patch twice

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: patch 2: don't use nsRecyclingAllocator in libjar (the right patch this time)

Comment 8

[(dormant account)]

Comment on attachment 586879 [details] [diff] [review]
patch 2: don't use nsRecyclingAllocator in libjar (the right patch this time)

please get rid of the malloc/free wrappers.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: patch 2, v2

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7446734cbf34

Mergers, there are two more patches to come, please leave this bug open for now.

Comment 11

[Matt Brubeck (:mbrubeck)]

Part 1 landed for Firefox 12:
https://hg.mozilla.org/mozilla-central/rev/7446734cbf34

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea3311b5e6c

Mergers, there's still one more patch to come, please leave this open for now.

Comment 13

[Justin Wood (:Callek)]

https://hg.mozilla.org/mozilla-central/rev/7ea3311b5e6c

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8dd350dbab

That's the last patch, please close this bug once merged.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/3b8dd350dbab

Comment 16

[Alfred Kayser]

RIP nsRecylingAllocator, it will stay in my memory for ever...