Closed Bug 682195 Opened 8 years ago Closed 6 years ago

Use mallocSizeOf in decoded-{audio,video} memory reporters

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 961441

People

(Reporter: kinetik, Unassigned)

Details

(Whiteboard: [MemShrink:P2][see comment 11])

Larger video frame allocations are subject to allocation slop on the order of one page (4kB) to |chunksize| (1MB in our jemalloc), depending on the base allocation size.

The existing memory reports should be modified to account for this, as it can contribute a significant amount to heap-unclassified.

See bug 680358 for details.
Whiteboard: [MemShrink]
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [MemShrink] → [MemShrink:P2]
BTW, I'm pretty sure the maximum slop is one page.  We round up the *virtual* size to 1M, but jemalloc decommits / madvise(DONT_NEED)'s pages it doesn't need.
(In reply to Justin Lebar [:jlebar] from comment #1)
> BTW, I'm pretty sure the maximum slop is one page.  We round up the
> *virtual* size to 1M, but jemalloc decommits / madvise(DONT_NEED)'s pages it
> doesn't need.

But heap memory reporters don't take that into account, they just report on what was requested/allocated.  So using moz_malloc_usable_size is the right thing to do here, IMO (that's what I've been doing in the JS engine).
Sure, using malloc_usable_size (and falling back to the requested size when it's not available) is reasonable, since when we don't have jemalloc, we don't know how malloc rounds up anyway.

malloc_usable_size rounds big allocations up to the nearest page, not the nearest 1MB, right?  (It's possible that it'd round up to the nearest MB, since without DECOMMIT, the whole MB is usable, just MADV_FREE'd.)
(gdb) set $ptr = moz_malloc((1 << 20) + 1)
(gdb) p moz_malloc_usable_size($ptr)
$1 = 2097152

But that makes sense, right?  The entire chunk may not be mapped, but it's still wasted address space.
(In reply to Justin Lebar [:jlebar] from comment #3)
> 
> malloc_usable_size rounds big allocations up to the nearest page, not the
> nearest 1MB, right?  (It's possible that it'd round up to the nearest MB,
> since without DECOMMIT, the whole MB is usable, just MADV_FREE'd.)

"Large" allocations (4KB..1MB-1) are rounded up to the nearest 4KB.  "Huge" allocations (1MB+) are rounded up to the nearest MB.
> "Large" allocations (4KB..1MB-1) are rounded up to the nearest 4KB.  "Huge" allocations 
> (1MB+) are rounded up to the nearest MB.

That's what I used to think, too.  But the OS has our back here.  If you allocate 1MB + epsilon, but you never touch the pages outside the range you requested, the OS won't assign physical pages there.  So effectively the memory used is the requested size rounded up to the nearest page.

I think this is pretty clearly the intent of the code (or, as clearly as anything is in jemalloc), because in huge_malloc, when MALLOC_DECOMMIT is defined, it decommits the slop pages.  If the slop pages were taking up memory without DECOMMIT, surely they'd be MADV_DONTNEED'ed.

> But that makes sense, right?  The entire chunk may not be mapped, but it's still wasted 
> address space.

It makes sense for malloc_usable_size to report that, yes, because it is usable.  But if you touch all 1MB + 1 byte of memory you requested, you'll be using 1MB + 4kb of memory, not 2MB of memory.
A problem here is that we've been comparing 'explicit' to 'heap-allocated' and also to 'resident'.  But if heap-allocated counts a 1MB + 1B allocation as 2MB, but resident only counts it as 1MB + 4KB, then...we have a bit of a problem.
(In reply to Justin Lebar [:jlebar] from comment #7)
> A problem here is that we've been comparing 'explicit' to 'heap-allocated'
> and also to 'resident'.  But if heap-allocated counts a 1MB + 1B allocation
> as 2MB, but resident only counts it as 1MB + 4KB, then...we have a bit of a
> problem.

There's no problem.  'explicit' and 'heap-allocated' are directly comparable.  'resident' isn't.  Heap memory reporters should report what malloc_usable_size says, so 2MB in this case.
You don't think it's a problem that we're over-reporting how much memory these allocations actually use?  Most of the other allocations in explicit are small, so those allocations correspond directly to RSS.  Reporting 2MB here instead of 1MB + 4KB will make the audio/video system look like it's using more memory than it actually is.

Put another way, why do I care at all about the 2MB number?  It's much less important than the 1MB + 4KB number, since I care much more about RSS than vsize.

This isn't the bug for it, but my preference would be to modify jemalloc so the heap-allocated number doesn't include this slop.
Filed bug 683597 on changing jemalloc's accounting.  Until that lands, this should round up to the next MB (as is done in malloc_usable_size).
jemalloc's accounting has been changed.  So the only thing left to do here is to convert these memory reporters to the new style (https://wiki.mozilla.org/Memory_Reporting), as we've been doing with other memory reporters.
Summary: Account for allocation slop in decoded-{audio,video} memory reporters → Use mallocSizeOf in decoded-{audio,video} memory reporters
Whiteboard: [MemShrink:P2] → [MemShrink:P2][see comment 11]
Assignee: kinetik → nobody
Status: ASSIGNED → NEW
Depends on: 961441, 962154
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 961441
No longer depends on: 961441, 962154
You need to log in before you can comment on or make changes to this bug.