Open Bug 683080 Opened 13 years ago Updated 1 year ago

Add memory reporters for pipes and storage streams

Categories

(Core :: Networking, defect, P5)

defect

Tracking

()

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-backlog])

That includes both in-use buffers and whatever has been recycled back to the recycling allocator, possibly as separate numbers.

This came up in bug 126212.

I can especially see pipes and storage streams using a bunch of memory when extensions get involved.
Here's some relevant output from DMD:

 Unreported: 24 block(s) in record 4 of 18137
  884,736 bytes (786,624 requested / 98,112 slop)
  0.78% of the heap (5.39% cumulative unreported)
    at 0x402A063: malloc (vg_replace_malloc.c:263)
    by 0x403C0A4: moz_malloc (mozalloc.cpp:113)
    by 0x7A60AE4: nsRecyclingAllocator::Malloc(unsigned long, bool) (nsRecyclingAllocator.cpp:164)
    by 0x7A60E15: nsRecyclingAllocatorImpl::Alloc(unsigned long) (nsRecyclingAllocator.cpp:295)
    by 0x7A7E7A4: nsSegmentedBuffer::AppendNewSegment() (nsSegmentedBuffer.cpp:103)
    by 0x7A79B13: nsPipe::GetWriteSegment(char*&, unsigned int&) (nsPipe3.cpp:488)
    by 0x7A7B483: nsPipeOutputStream::WriteSegments(unsigned int (*)(nsIOutputStream*, void*, char*, unsigned int, unsigned int, unsigned int*)
, void*, unsigned int, unsigned int*) (nsPipe3.cpp:1110)
    by 0x7A7D6D3: nsStreamCopierOB::DoCopy(unsigned int*, unsigned int*) (nsStreamUtils.cpp:579)
    by 0x7A7CAF2: nsAStreamCopier::Process() (nsStreamUtils.cpp:319)
    by 0x7A7D094: nsAStreamCopier::Run() (nsStreamUtils.cpp:435)
Pipe memory usage should be transitory (right?) so it may be worth investigating why these blocks haven't been deallocated.  I'd start with figuring out what's creating the nsPipe, and why it isn't dead.  (I'm assuming that there are 1 or some other small number of nsPipes here because there's only 24 blocks involved).
(An edited IRC transcript relating to this...)

<bz> nsSegmentedBuffer always uses nsMemory::GetGlobalMemoryService() as mSegAllocator,
     Which returns NS_GetMemoryManager, which afaict does
    
      return sGlobalMemory.QueryInterface(NS_GET_IID(nsIMemory), (void**) result);

     where sGlobalMemory is an nsMemoryImpl
     how are we ending up in nsRecyclingAllocator?
     ah
     someone can pass an allocator to Init()!
     and it looks like that allocator is the one that comes from nsPipe::Init
     so this recycling allocator comes from net_GetSegmentAlloc

<khuey> bz: so this is just the generic necko allocator?
        that keeps chunks around?

<bz>  yeah
      it uses a 15 minute timer to discard the chunks

<khuey> sounds like something that needs to listen for memory pressure!
We have two uses of nsRecyclingAllocator, AFAICT:
- One in nsIOService which has the 15 minute time-out.
- One in nsZipArchive.cpp which has the default 10 second time-out.

The obvious thing to do here is to add a memory reporter to nsIOService.

Another possibility is to reduce the time-out on nsIOService.

My preferred option is to get rid of nsRecyclingAllocator altogether.  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.)

If I were to remove nsRecyclingAllocator altogether, how would I determine if performance has improved/worsened?  Is just normal Talos good enough?

This stuff was last touched in bug 545869, though the 15 minute time-out predates that.  I've CC'd some mobile guys who were involved with that bug.
Another downside of using nsRecyclingAllocator is that we allocate 32KB chunks, but nsRecyclingAllocator clownshoe-ishly adds an extra word, and so jemalloc rounds the request up to 36KB.  With 24 chunks in the necko cache this wastes almost 24*4KB == 96KB of memory.
> If I were to remove nsRecyclingAllocator altogether, how would I determine if performance has 
> improved/worsened?  Is just normal Talos good enough?

If you want to be really paranoid, you could run a microbenchmark which thrashes the cache.  The performance regression there (if any) should be an upper bound on the real-world perf regression.

But on IRC, njn mentioned that the number of allocations here when loading techcrunch and gmail is on the order of 1000.  In that case, it seems really unlikely switching to jemalloc would make things measurably slower.
I spun off bug 715770 for killing nsRecycleAllocator.
Depends on: 715770
With bug 715770 fixed, nsRecyclingAllocator is gone, and this is no longer necessary.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Wait, why?  This bug was about detecting and reporting long-lived full pipes and storage streams.  The recycling allocator bit was a sideline...  See comment 0.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I think njn's measurements showed that we don't have long-lived full pipes and storage streams in the browser during his workloads.
Was testing done on a vanilla install, or with extensions?

In a vanilla install, that shouldn't happen.  Again, see comment 0.
(In reply to Boris Zbarsky (:bz) from comment #11)
> Was testing done on a vanilla install, or with extensions?
> 
> In a vanilla install, that shouldn't happen.  Again, see comment 0.

My testing was without any add-ons, where I haven't seen pipes outside the nsRecyclingAllocator being significant.

In comment 0 you said:

> I can especially see pipes and storage streams using a bunch of memory
> when extensions get involved.

By that do you mean "I have seen this in profiles" or "I can imagine this happening"?

I went back to bug 126212, the only reference to pipes and storage streams I could see was bug 126212 comment 41, which said:

> 7) We probably need a reporter for the pipe recycling allocator,
> as well as for the segments of live pipes!

So the former is now moot and we're talking about the latter.  I didn't see any mention of add-ons in that bug.  If you tell me which add-on(s) I should test to see lots of live pipes I'm happy to do that.
> By that do you mean "I have seen this in profiles" or "I can imagine this happening"?

More like "I've seen this in extension code, and I bet they don't clean up their stuff very well".

I'd start testing with Firebug.  And maybe any other extensions that try to show network data.  If it's not an issue with those, it's probably not an issue in general.

Sorry I didn't make this clear earlier.....
Whiteboard: [necko-backlog]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3

Bulk-downgrade of unassigned, >=3 years untouched DOM/Storage bug's priority.

If you have reason to believe this is wrong, please write a comment and ni :jstutte.

Severity: normal → S4
Priority: P3 → P5

This might be WONTFIX

Status: REOPENED → NEW
OS: macOS → Unspecified
Hardware: x86 → Unspecified
You need to log in before you can comment on or make changes to this bug.