Last Comment Bug 715770 - Kill nsRecyclingAllocator
: Kill nsRecyclingAllocator
Status: RESOLVED FIXED
[MemShrink:P2][clownshoes]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks: 683080 716323
  Show dependency treegraph
 
Reported: 2012-01-05 18:48 PST by Nicholas Nethercote [:njn]
Modified: 2012-01-18 14:03 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: remove necko buffer cache (11.04 KB, patch)
2012-01-05 19:34 PST, Nicholas Nethercote [:njn]
cbiesinger: review+
Details | Diff | Splinter Review
patch 2: don't use nsRecyclingAllocator in libjar (23.16 KB, patch)
2012-01-05 19:38 PST, Nicholas Nethercote [:njn]
taras.mozilla: feedback+
Details | Diff | Splinter Review
patch 3: remove nsRecyclingAllocator (23.16 KB, patch)
2012-01-05 19:39 PST, Nicholas Nethercote [:njn]
benjamin: review+
Details | Diff | Splinter Review
patch 2: don't use nsRecyclingAllocator in libjar (the right patch this time) (3.94 KB, patch)
2012-01-08 21:31 PST, Nicholas Nethercote [:njn]
taras.mozilla: review-
Details | Diff | Splinter Review
patch 2, v2 (4.06 KB, patch)
2012-01-09 01:29 PST, Nicholas Nethercote [:njn]
taras.mozilla: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-01-05 18:48:03 PST
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 [:njn] 2012-01-05 19:34:54 PST
Created attachment 586327 [details] [diff] [review]
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 [:njn] 2012-01-05 19:38:13 PST
Created attachment 586328 [details] [diff] [review]
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 [:njn] 2012-01-05 19:39:25 PST
Created attachment 586329 [details] [diff] [review]
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) 2012-01-05 19:43:36 PST
Comment on attachment 586327 [details] [diff] [review]
patch 1: remove necko buffer cache

watching talos sounds good to me
Comment 5 (dormant account) 2012-01-06 10:38:45 PST
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) 2012-01-06 10:39:43 PST
(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 [:njn] 2012-01-08 21:31:40 PST
Created attachment 586879 [details] [diff] [review]
patch 2: don't use nsRecyclingAllocator in libjar (the right patch this time)
Comment 8 (dormant account) 2012-01-08 22:48:52 PST
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 [:njn] 2012-01-09 01:29:26 PST
Created attachment 586926 [details] [diff] [review]
patch 2, v2
Comment 10 Nicholas Nethercote [:njn] 2012-01-11 15:17:18 PST
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) 2012-01-12 11:00:30 PST
Part 1 landed for Firefox 12:
https://hg.mozilla.org/mozilla-central/rev/7446734cbf34
Comment 12 Nicholas Nethercote [:njn] 2012-01-16 14:55:31 PST
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) 2012-01-16 20:02:59 PST
https://hg.mozilla.org/mozilla-central/rev/7ea3311b5e6c
Comment 14 Nicholas Nethercote [:njn] 2012-01-17 17:33:31 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8dd350dbab

That's the last patch, please close this bug once merged.
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2012-01-18 02:54:48 PST
https://hg.mozilla.org/mozilla-central/rev/3b8dd350dbab
Comment 16 Alfred Kayser 2012-01-18 14:03:27 PST
RIP nsRecylingAllocator, it will stay in my memory for ever...

Note You need to log in before you can comment on or make changes to this bug.