Closed Bug 715770 Opened 8 years ago Closed 8 years ago

Kill nsRecyclingAllocator

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

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

Attachments

(3 files, 2 obsolete files)

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.
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)
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)
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)
Blocks: 683080
Attachment #586329 - Flags: review?(benjamin) → review+
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+
(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
Blocks: 716323
Attachment #586328 - Attachment is obsolete: true
Attachment #586879 - Flags: review?(taras.mozilla)
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-
Attached patch patch 2, v2Splinter Review
Attachment #586879 - Attachment is obsolete: true
Attachment #586926 - Flags: review?(taras.mozilla)
Attachment #586926 - Flags: review?(taras.mozilla) → review+
Whiteboard: [MemShrink] → [MemShrink:P2]
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]
Whiteboard: [MemShrink:P2][leave open after merge] → [MemShrink:P2]
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/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
Closed: 8 years ago
Resolution: --- → FIXED
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.