Kill nsRecyclingAllocator

RESOLVED FIXED in mozilla12

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2][clownshoes])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Attachment #586327 - Flags: review?(cbiesinger)
(Assignee)

Comment 2

6 years ago
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.
Attachment #586328 - Flags: review?(taras.mozilla)
(Assignee)

Comment 3

6 years ago
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(-)
Attachment #586329 - Flags: review?(khuey)
Comment on attachment 586327 [details] [diff] [review]
patch 1: remove necko buffer cache

watching talos sounds good to me
Attachment #586327 - Flags: review?(cbiesinger) → review+
Attachment #586329 - Flags: review?(khuey) → review?(benjamin)
(Assignee)

Updated

6 years ago
Blocks: 683080

Updated

6 years ago
Attachment #586329 - Flags: review?(benjamin) → review+

Comment 5

6 years ago
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.
Attachment #586328 - Flags: review?(taras.mozilla) → feedback+

Comment 6

6 years ago
(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
(Assignee)

Updated

6 years ago
Blocks: 716323
(Assignee)

Comment 7

6 years ago
Created attachment 586879 [details] [diff] [review]
patch 2: don't use nsRecyclingAllocator in libjar (the right patch this time)
Attachment #586328 - Attachment is obsolete: true
Attachment #586879 - Flags: review?(taras.mozilla)

Comment 8

6 years ago
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.
Attachment #586879 - Flags: review?(taras.mozilla) → review-
(Assignee)

Comment 9

6 years ago
Created attachment 586926 [details] [diff] [review]
patch 2, v2
Attachment #586879 - Attachment is obsolete: true
Attachment #586926 - Flags: review?(taras.mozilla)

Updated

6 years ago
Attachment #586926 - Flags: review?(taras.mozilla) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 10

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

Mergers, there are two more patches to come, please leave this bug open for now.
Whiteboard: [MemShrink:P2] → [MemShrink:P2][leave open after merge]
Part 1 landed for Firefox 12:
https://hg.mozilla.org/mozilla-central/rev/7446734cbf34
Whiteboard: [MemShrink:P2][leave open after merge] → [MemShrink:P2]
(Assignee)

Comment 12

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

Mergers, there's still one more patch to come, please leave this open for now.
Whiteboard: [MemShrink:P2] → [MemShrink:P2][leave open after merge]
https://hg.mozilla.org/mozilla-central/rev/7ea3311b5e6c
Target Milestone: --- → mozilla12
(Assignee)

Comment 14

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

That's the last patch, please close this bug once merged.
Whiteboard: [MemShrink:P2][leave open after merge] → [MemShrink:P2][clownshoes]
https://hg.mozilla.org/mozilla-central/rev/3b8dd350dbab
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 16

6 years ago
RIP nsRecylingAllocator, it will stay in my memory for ever...
You need to log in before you can comment on or make changes to this bug.