Bug 828844 added the "system-heap-allocated" memory report on Linux. It would be nice to have it on other platforms too: - Mac: not sure how. - Android: not sure how. Is it different from Linux? - Windows: I don't know. Well, you can enumerate a heap and measure each block individually, but that sounds slow and awful: https://msdn.microsoft.com/en-us/library/windows/desktop/ee175819%28v=vs.85%29.aspx

(In reply to Nicholas Nethercote [:njn] from comment #0) > - Android: not sure how. Is it different from Linux? Yes an no, Android uses a different system allocator, but that allocator exposes mallinfo... except when it might not. Recent bionic has the option to build with jemalloc as the system allocator, and I don't know if devices in the wild do use that (b2g does, btw). Jemalloc doesn't expose mallinfo. I don't know if bionic exposes it nevertheless in this case with a proper wrapper.

I said earlier that heap enumeration sounded slow, but because the system allocator heap shouldn't actually be used much, I decided it wouldn't be so bad. In a 64-bit build I see it at about 11--12 MiB at start-up, and then later on it bounces around between ~0.5 MiB and ~10 MiB. The number of allocated blocks varies between ~2000 and ~7000. Currently it's only in about:memory's "Other measurements" section. I guess I could put it in the "explicit" tree as well. I'm undecided about that.

> In a 64-bit build I see it at about 11--12 MiB at start-up, and then later on > it bounces around between ~0.5 MiB and ~10 MiB. The number of allocated > blocks varies between ~2000 and ~7000. The world looks so nice from our dev machines! I'd be curious to see how long an enumeration takes when there are dual graphics drivers, an antivirus, and various binary hook addons all eating up heap. Note that reports can be collected unwittingly through the OOM-analysis feature. Do/can we have telemetry, or even a measurement on the report itself, of how long collection takes?

Comment on attachment 8647892 [details] [diff] [review] Implement "system-heap-allocated" reporter for Windows Review of attachment 8647892 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/nsMemoryReporterManager.cpp @@ +571,5 @@ > > +// njn: cull these > +#include <windows.h> > +#include <tchar.h> > +#include <stdio.h> njn: cull these :) @@ +577,5 @@ > +#define HAVE_SYSTEM_HEAP_REPORTER 1 > +nsresult > +SystemHeapSize(int64_t* aSizeOut) > +{ > + HANDLE heap = GetProcessHeap(); This will get you the default heap, used by the CRT and most others, but it's also possible for code to create other heaps. Should we be doing GetProcessHeaps (plural) here? @@ +588,5 @@ > + PROCESS_HEAP_ENTRY entry; > + entry.lpData = nullptr; > + while (HeapWalk(heap, &entry)) { > + if (entry.wFlags & PROCESS_HEAP_ENTRY_BUSY) { > + size += entry.cbData; Should we account for entry.cbOverhead somehow? @@ +593,5 @@ > + } > + } > + > + if (GetLastError() != ERROR_NO_MORE_ITEMS) { > + return NS_ERROR_FAILURE; This will leave the heap locked!

> Should we account for entry.cbOverhead somehow? No. AFAICT, that's overhead due in the allocators own data structures. Neither "heap-allocated" and "system-heap-allocated" include that; they just include the memory that Firefox has access to.

I'm iterating over all the heaps in the way recommended by https://msdn.microsoft.com/en-us/library/windows/desktop/ee175820%28v=vs.85%29.aspx. I hope heaps are ref-counted. If they're not, the following sequence might cause problems. - We get a HANDLE to a heap and stick it in our array. - Another thread destroys the heap. - We call HeapWalk() on the heap.

Comment on attachment 8650227 [details] [diff] [review] Implement "system-heap-allocated" reporter for Windows Review of attachment 8650227 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/nsMemoryReporterManager.cpp @@ +580,5 @@ > + DWORD nHeaps = GetProcessHeaps(0, nullptr); > + NS_ENSURE_TRUE(nHeaps != 0, NS_ERROR_FAILURE); > + > + // Allocate space for a handle to each heap. > + UniquePtr<HANDLE[]> heaps(new HANDLE[nHeaps]); Neat! Yay for a modern codebase. @@ +581,5 @@ > + NS_ENSURE_TRUE(nHeaps != 0, NS_ERROR_FAILURE); > + > + // Allocate space for a handle to each heap. > + UniquePtr<HANDLE[]> heaps(new HANDLE[nHeaps]); > + NS_ENSURE_TRUE(heaps, NS_ERROR_OUT_OF_MEMORY); Isn't operator new infallible? @@ +586,5 @@ > + > + // Get handles to all heaps, checking that no new heaps have been created in > + // the meantime. > + DWORD nHeaps2 = GetProcessHeaps(nHeaps, heaps.get()); > + NS_ENSURE_TRUE(nHeaps2 != 0 && nHeaps2 <= nHeaps, NS_ERROR_FAILURE); It might be simpler just to demand that nHeaps2 == nHeaps. I don't expect it ever to fail, in practice. In theory though, your comment 6 is correct: somebody could destroy a heap while we hold a pointer. If it's really a big concern, then I guess we'd want to HeapLock all of them before we start HeapWalk'ing (and even _that_ would still have a tiny window where a heap could disappear).

> Isn't operator new infallible? I thought that |operator new[]| was fallible, but looking at mozalloc.h it turns out it's infallible. Huh.