Very large 14,269.36 MB ── vsize

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: alex_mayorga, Assigned: bbouvier)

Tracking

51 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox51 fix-optional, firefox52 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8791583 [details]
memory-report.json.gz

¡Hola!

While investigating https://bugzilla.mozilla.org/show_bug.cgi?id=1291294 I've found a very large 14,269.36 MB ── vsize, asked on #memshrink and erahm suggested to file a bug so here it is.

¡Gracias!
Alex
(Reporter)

Updated

a year ago
Flags: needinfo?(erahm)
Whiteboard: [MemShrink]
(Reporter)

Comment 1

a year ago
Created attachment 8791696 [details]
memory-report-safe-mode.json.gz

¡Hola!

Safe mode doesn't seem to make any difference.

¡Gracias!
Alex
(Reporter)

Comment 2

a year ago
Created attachment 8791704 [details]
memory-report-all-tabs-closed-safe-mode.json.gz

¡Hola!

Another one from "Safe mode" and all tabs but about:memory closed.

Closing web.telegram.org brought it down to 7,946.30 MB ── vsize 50% decrease.

https://vector.im/develop/#/room/#mozilla_#memshrink:matrix.org brings it up to 7,366.69 MB ── vsize as well.

¡Gracias!
Alex
I can confirm the STR:
  #1 - Load (no need to sign in or anything): https://vector.im/develop/#/room/#mozilla_#memshrink:matrix.org
  #2 - Check about:memory

Example result after loading:

> 17,479.97 MB ── vsize

Example after opening another tab (google.com), closing first tab:

> 9,285.45 MB ── vsize

And after minimizing:

> 1,084.18 MB ── vsize

So this seems okay but I'm a bit concerned about what happens on a 32-bit build.
Flags: needinfo?(erahm)
After discussing in IRC we came up with a theory this is ASM.js related. Similar behavior is also seen with telegram and mega.
This is where the memory comes from: a 6.4GB region with no permissions (---p)

Here is the stack trace when that region is mmapped:
#0  0x00007f1a724ef260 in __mmap (addr=0x0, len=6442520576, prot=0, flags=34, fd=-1, offset=0)
    at ../sysdeps/unix/sysv/linux/wordsize-64/mmap.c:33
#1  0x00007f1a75b52fd7 in js::WasmArrayRawBuffer::Allocate(unsigned int, mozilla::Maybe<unsigned int>) (aTag=<optimized out>, aOffset=0, aFd=-1, aFlags=34, aProt=0, aLength=6442520576, aAddr=0x0)
    at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/obj-firefox/dist/include/mozilla/TaggedAnonymousMemory.h:73
#2  0x00007f1a75b52fd7 in js::WasmArrayRawBuffer::Allocate(unsigned int, mozilla::Maybe<unsigned int>) (numBytes=numBytes@entry=16777216, maxSize=...) at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/vm/ArrayBufferObject.cpp:595
#3  0x00007f1a75b55005 in js::ArrayBufferObject::prepareForAsmJS(JSContext*, JS::Handle<js::ArrayBufferObject*>, bool) (cx=cx@entry=0x7f1a5f028000, buffer=buffer@entry=..., needGuard=needGuard@entry=true)
    at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/vm/ArrayBufferObject.cpp:733
#4  0x00007f1a75c3fb44 in InstantiateAsmJS(JSContext*, unsigned int, JS::Value*) (metadata=..., metadata=..., buffer=..., bufferVal=..., cx=0x7f1a5f028000) at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/asmjs/AsmJS.cpp:7846
#5  0x00007f1a75c3fb44 in InstantiateAsmJS(JSContext*, unsigned int, JS::Value*) (exportObj=..., instanceObj=..., 
    metadata=..., module=..., cx=0x7f1a5f028000, args=...)
    at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/asmjs/AsmJS.cpp:7926
#6  0x00007f1a75c3fb44 in InstantiateAsmJS(JSContext*, unsigned int, JS::Value*) (cx=0x7f1a5f028000, argc=3, vp=0x7fff6d189128)
    at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/asmjs/AsmJS.cpp:8054
#7  0x00007f1a7642dce9 in js::CallFromStack(JSContext*, JS::CallArgs const&) (args=..., native=0x7f1a75c3f6b9 <InstantiateAsmJS(JSContext*, unsigned int, JS::Value*)>, cx=0x7f1a5f028000)
    at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/jscntxtinlines.h:235
#8  0x00007f1a7642dce9 in js::CallFromStack(JSContext*, JS::CallArgs const&) (construct=js::NO_CONSTRUCT, args=..., cx=0x7f1a5f028000) at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/vm/Interpreter.cpp:454
#9  0x00007f1a7642dce9 in js::CallFromStack(JSContext*, JS::CallArgs const&) (args=..., cx=0x7f1a5f028000)
    at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/vm/Interpreter.cpp:499
#10 0x00007f1a7642dce9 in js::CallFromStack(JSContext*, JS::CallArgs const&) (cx=cx@entry=0x7f1a5f028000, args=...)
    at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/vm/Interpreter.cpp:505
#11 0x00007f1a765942d3 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, uint32_t, JS::Value*, JS::MutableHandleValue) (cx=0x7f1a5f028000, frame=0x7fff6d189198, stub_=0x7f1a4a03f490, argc=3, vp=0x7fff6d189128, res=...)
    at /builds/slave/m-cen-l64-ntly-000000000000000/build/src/js/src/jit/BaselineIC.cpp:5998

Which after looking around, points to this comment:
https://dxr.mozilla.org/mozilla-central/rev/8a494adbc5cced90a4edf0c98cffde906bf7f3ae/js/src/vm/ArrayBufferObject.cpp#384

So this is pretty much happening by design...

It would probably be better if about:memory knew about it.
Benjamin, Luke, can you take a look at comment 5 and see what can be done to make that appear in about:memory somehow?
Component: Memory Allocator → JavaScript Engine
Flags: needinfo?(luke)
Flags: needinfo?(bbouvier)
Sure, we can do this.  So there doesn't seem to be a more specific bucket than the list at the end of "Other Measurements" that ends with "vsize".  So would a "wasm-guard-page-vsize" have been helpful here?
(In reply to Luke Wagner [:luke] from comment #7)
> Sure, we can do this.  So there doesn't seem to be a more specific bucket
> than the list at the end of "Other Measurements" that ends with "vsize".  So
> would a "wasm-guard-page-vsize" have been helpful here?

That location is probably fine.

Perhaps vsize-wasm-guard-page would make more sense so it's next to vsize. Even then, that wording is a bit odd to someone not intimately familiar with WASM (I'd expected page size to be 64KiB, not 7GiB). 

Maybe just 'vsize-wasm-reserved' (or even just 'vsize-wasm' as I guess that memory could be in use) with a nice detailed hover text?
(Assignee)

Comment 9

a year ago
Created attachment 8792588 [details] [diff] [review]
tidyingup.patch

Here's a small patch made after reading some code:

- asm.js may use a wasm-backed buffer (calling WasmArrayRawBuffer::Allocate, mmap'd, which is not the GC heap or the Malloc heap, so should probably belong in "other"?) or a plain GC-calloc'd buffer (GC heap).
- wasm uses WasmArrayRawBuffer::Allocate all the time, so it's also mmap'd.

So none does allocate memory in the Malloc heap (this patch removes objectsMallocHeapElementsAsmJS).

In the case of mmap'd memory, should we report as:
- the actual committed range in one bin
- the rest of the mprotected memory range in another bin?

(just a random thought: there shouldn't be anything to do with respect to GrowMemory, right? since the heap measurement is just a snapshot)
Flags: needinfo?(bbouvier)
Attachment #8792588 - Flags: review?(luke)
(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> Created attachment 8792588 [details] [diff] [review]
> - asm.js may use a wasm-backed buffer (calling WasmArrayRawBuffer::Allocate,
> mmap'd, which is not the GC heap or the Malloc heap, so should probably
> belong in "other"?) or a plain GC-calloc'd buffer (GC heap).
> - wasm uses WasmArrayRawBuffer::Allocate all the time, so it's also mmap'd.
> 
> So none does allocate memory in the Malloc heap (this patch removes
> objectsMallocHeapElementsAsmJS).

Well technically the 32-bit asm.js case does (re)use the incoming malloc'd memory from the ArrayBuffer, so that could go in objectsMallocHeapElementsAsmJS, but I see that we're not even doing that right now (if so, it would be in the 'case PLAIN:' of addSizeOfExcludingThis()).

Also given that wasm memories are no longer Just ArrayBuffers, perhaps we could rename objectsNonHeapElementsWasm to objectsNonHeapWasmMemory?

> In the case of mmap'd memory, should we report as:
> - the actual committed range in one bin
> - the rest of the mprotected memory range in another bin?

I think we should report the byteLength() in objectsNonHeapElementsWasm because that's the range that is R+W.  Then we should additionally create a new category that would get reported as 'vsize-wasm-reserved' (see comment 8) that would accumulate (mappedSize() - byteLength()).  The tooltip should point out that this memory is reserved but not committed and thus cannot have RSS cost and is not included in 'explicit'.

> (just a random thought: there shouldn't be anything to do with respect to
> GrowMemory, right? since the heap measurement is just a snapshot)

Right, it's just a snapshot.
Flags: needinfo?(luke)
(Thanks btw!)
(Assignee)

Comment 12

a year ago
Created attachment 8792889 [details] [diff] [review]
memory.patch

Nicholas, selecting you as a reviewer for the XPCJSConnect.cpp parts as the last person who touched these lines before me, but feel free to redirect to anybody else.

Luke, can you review the js/src parts please?

Doing this was actually simple; I've started the wrong way adding a new unsigned integer in the nsMemoryReporter IDL and go through all the code down vm/MemoryMetrics.cpp, but it wasn't clearly needed. Thanks froydnj for proposing a simpler design on irc!
Assignee: nobody → bbouvier
Attachment #8792588 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8792588 - Flags: review?(luke)
Attachment #8792889 - Flags: review?(n.nethercote)
Attachment #8792889 - Flags: review?(luke)
(Assignee)

Comment 13

a year ago
Created attachment 8792892 [details]
wasm-guard-pages.png

Example of about:memory after opening web.telegram.org.

Note that this is the expected amount on x64 (4GB + 2GB + 64KB, aka js::wasm::HugeMappedSize http://searchfox.org/mozilla-central/search?q=symbol:_ZN2js4wasmL14HugeMappedSizeE&redirect=false ); I need to run through treeherder and test on x86 that it reports the correct values too (where we don't use the guard pages for asm.js at least).
(Assignee)

Comment 14

a year ago
Created attachment 8792998 [details]
32bits.png

And it also works on 32 bits, showing only the asm.js heap, and no vsize-wasm-guard-page.
(Assignee)

Comment 15

a year ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> Created attachment 8792892 [details]
> wasm-guard-pages.png
> 
> Example of about:memory after opening web.telegram.org.
> 
> Note that this is the expected amount on x64 (4GB + 2GB + 64KB, aka
> js::wasm::HugeMappedSize
> http://searchfox.org/mozilla-central/search?q=symbol:
> _ZN2js4wasmL14HugeMappedSizeE&redirect=false ); I need to run through
> treeherder and test on x86 that it reports the correct values too (where we
> don't use the guard pages for asm.js at least).

Actually, it shows 4 GB + 2 GB + 64 KB - 8 MB, since the array is 8 MB (and the other section under elements/non-heap/wasm show 8 MB).
Comment on attachment 8792889 [details] [diff] [review]
memory.patch

Review of attachment 8792889 [details] [diff] [review]:
-----------------------------------------------------------------

I'm giving f+ because it's heading in the right direction but I'd like to see it again before landing. Apologies about the nitpickiness below but this stuff is intricate enough that care is required.

BTW, about:memory is just text so you can copy and paste rather than doing screenshots :)

::: js/public/MemoryMetrics.h
@@ -166,5 @@
>  #define FOR_EACH_SIZE(macro) \
>      macro(Objects, GCHeapUsed, objectsGCHeap) \
>      macro(Objects, MallocHeap, objectsMallocHeapSlots) \
>      macro(Objects, MallocHeap, objectsMallocHeapElementsNormal) \
> -    macro(Objects, MallocHeap, objectsMallocHeapElementsAsmJS) \

You moved objectsMallocHeapElementsAsmJS and change it from `MallocHeap` to `NonHeap`. That's incorrect, please revert.

(Please test these changes in a debug build, BTW. There are lots of assertions to make sure the JS memory reporting fits together as intended and it's possible that this change would have made them fail.)

::: js/xpconnect/src/XPCJSContext.cpp
@@ +2168,5 @@
> +    // Although wasm guard pages aren't committed in memory, they appear in the
> +    // total memory set and should be reported accordingly. We never put them
> +    // under sundries, because (a) in practice they're almost always larger than
> +    // the sundries threshold, and (b) we'd need a third category of sundries
> +    // ("non-heap"), which would be a pain.

The way you've changed the comment mixes things up a bit. I suggest this instead:

// WebAssembly memory is always non-heap-allocated (mmap). AsmJS arrays may
// either use the same system or be allocated on the heap. We never put these
// under sundries, because (a) in practice they're almost always larger than
// the sundries threshold, and (b) we'd need a third category of sundries
// ("non-heap"), which would be a pain.
//
// Although wasm guard pages aren't committed in memory they can be very large
// and contribute greatly to vsize and so are worth reporting.

@@ +2169,5 @@
> +    // total memory set and should be reported accordingly. We never put them
> +    // under sundries, because (a) in practice they're almost always larger than
> +    // the sundries threshold, and (b) we'd need a third category of sundries
> +    // ("non-heap"), which would be a pain.
> +    size_t heapElementsAsmJS = classInfo.objectsMallocHeapElementsAsmJS;

Please leave this variable named `mallocHeapElementsAsmJS`, so it matches the classInfo fieldname and the path below.

@@ +2170,5 @@
> +    // under sundries, because (a) in practice they're almost always larger than
> +    // the sundries threshold, and (b) we'd need a third category of sundries
> +    // ("non-heap"), which would be a pain.
> +    size_t heapElementsAsmJS = classInfo.objectsMallocHeapElementsAsmJS;
> +    if (heapElementsAsmJS) {

Please leave the `> 0` there.

@@ +2178,4 @@
>      }
> +
> +    size_t mmappedGuardPagesWasm = classInfo.mmappedGuardPagesWasm;
> +    if (mmappedGuardPagesWasm) {

Add `> 0`.

@@ +2179,5 @@
> +
> +    size_t mmappedGuardPagesWasm = classInfo.mmappedGuardPagesWasm;
> +    if (mmappedGuardPagesWasm) {
> +        REPORT_BYTES(NS_LITERAL_CSTRING("vsize-wasm-guard-pages"),
> +            KIND_OTHER, mmappedGuardPagesWasm,

I know erahm suggested "vsize-wasm-guard-pages", but I'm going to veto that name, because I don't think the "vsize-" prefix makes sense. I suggest just "wasm-guard-pages". That will end up just after "vsize" due to the alphabetical ordering. No matter what name is chosen the meaning will be obscure and people will have to refer to the tool-tip to understand it anyway.

Also, please call the variable `wasmGuardPages` so it matches the path. Likewise with the ClassInfo field name.

@@ +2186,5 @@
> +            "committed, it is not in resident memory.");
> +    }
> +
> +    size_t nonHeapMemoryWasm = classInfo.objectsNonHeapMemoryWasm;
> +    if (nonHeapMemoryWasm) {

Add `> 0`.

@@ +2187,5 @@
> +    }
> +
> +    size_t nonHeapMemoryWasm = classInfo.objectsNonHeapMemoryWasm;
> +    if (nonHeapMemoryWasm) {
> +        REPORT_BYTES(path + NS_LITERAL_CSTRING("objects/non-heap/memory/wasm"),

The "memory/" sub-path is not serving any useful purpose here. Just make it "objects/non-heap/wasm", then change the variable and the ClassInfo field name accordingly.
Attachment #8792889 - Flags: review?(n.nethercote) → feedback+
Thank you for taking this on, BTW. It's good to avoid mysteriously high numbers in about:memory :)
Comment on attachment 8792889 [details] [diff] [review]
memory.patch

Review of attachment 8792889 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good from my end; I'll also be interested to see the revised version with njn's comments addressed.
Attachment #8792889 - Flags: review?(luke)
(Assignee)

Comment 19

a year ago
Thanks for the reviews!

Right about the screenshots; I don't know what I was thinking.
I've tried a debug build under x64, but an optimized non-debug build under x86, my bad.

Relaunching a try build to re-test the changes, requesting a new review in the meanwhile.
(Assignee)

Comment 20

a year ago
Created attachment 8793219 [details] [diff] [review]
wasmguardpages.v2.patch
Attachment #8792889 - Attachment is obsolete: true
Attachment #8793219 - Flags: review?(n.nethercote)
Attachment #8793219 - Flags: review?(luke)
Comment on attachment 8793219 [details] [diff] [review]
wasmguardpages.v2.patch

Review of attachment 8793219 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: js/xpconnect/src/XPCJSContext.cpp
@@ +2113,1 @@
>      if (mallocHeapElementsAsmJS > 0) {

Unless njn said/says otherwise, I'd move objectsMallocHeapElementsAsmJS up to be right under objectsMallocHeapElementsNormal and remove the above "AsmJS arrays may ... or be allocated on the heap" sentence in the above comment since this field now has a simple "always malloced" meaning and the rest of the comment doesn't apply.

@@ +2119,5 @@
> +    size_t wasmGuardPages = classInfo.wasmGuardPages;
> +    if (wasmGuardPages > 0) {
> +        REPORT_BYTES(NS_LITERAL_CSTRING("wasm-guard-pages"),
> +            KIND_OTHER, wasmGuardPages,
> +            "Memory mapped by the process allocated for wasm usage, reserved "

How about: "Guard pages mapped after the end of wasm memories, reserved for optimization tricks, but not committed and thus never contributing to RSS, only vsize."

@@ +2129,5 @@
> +    if (nonHeapWasm > 0) {
> +        REPORT_BYTES(path + NS_LITERAL_CSTRING("objects/non-heap/wasm"),
> +            KIND_NONHEAP, nonHeapWasm,
> +            "wasm/asm.js memory allocated outside both the malloc heap and the "
> +            "GC heap.");

How about "Memory allocated for wasm/asm.js memories allocated outside both the ...".
Attachment #8793219 - Flags: review?(luke) → review+
Comment on attachment 8793219 [details] [diff] [review]
wasmguardpages.v2.patch

Review of attachment 8793219 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/MemoryMetrics.h
@@ +172,2 @@
>      macro(Objects, MallocHeap, objectsMallocHeapElementsAsmJS) \
> +    macro(Objects, NonHeap,    objectsNonHeapWasm) \

In the previous review I asked you to change this from objectsNonHeapMemoryWasm to objectsNonHeapWasm. I now realize I should have asked for objectsNonHeapElementsWasm. Sorry!

@@ +172,4 @@
>      macro(Objects, MallocHeap, objectsMallocHeapElementsAsmJS) \
> +    macro(Objects, NonHeap,    objectsNonHeapWasm) \
> +    macro(Objects, Ignore,     wasmGuardPages) \
> +    macro(Objects, NonHeap,    objectsNonHeapCodeWasm) \

Please put them in this order:

> objectsGCHeap
> objectsMallocHeapSlots
> objectsMallocHeapElementsNormal
> objectsMallocHeapElementsAsmJS
> objectsMallocHeapMisc
> objectsNonHeapElementsNormal
> objectsNonHeapElementsShared
> objectsNonHeapElementsWasm
> objectsNonHeapCodeWasm
> wasmGuardPages

::: js/xpconnect/src/XPCJSContext.cpp
@@ +2112,1 @@
>      size_t mallocHeapElementsAsmJS = classInfo.objectsMallocHeapElementsAsmJS;

I realize this is pre-existing, but can you rename this objectsMallocHeapElementsAsmJS for consistency? Thanks.

@@ +2113,1 @@
>      if (mallocHeapElementsAsmJS > 0) {

Luke's comment about ordering is good. Generally the order of reporting in this file should match the order of the field declarations in MemoryMetrics.h. Feel free to reorder them to achieve that.

@@ +2119,5 @@
> +    size_t wasmGuardPages = classInfo.wasmGuardPages;
> +    if (wasmGuardPages > 0) {
> +        REPORT_BYTES(NS_LITERAL_CSTRING("wasm-guard-pages"),
> +            KIND_OTHER, wasmGuardPages,
> +            "Memory mapped by the process allocated for wasm usage, reserved "

Luke's suggestion seems good except for "memories", which sounds weird to me. I would use "memory".

@@ +2124,5 @@
> +            "for optimization tricks but not committed. Since it is not "
> +            "committed, it is not in resident memory.");
> +    }
> +
> +    size_t nonHeapWasm = classInfo.objectsNonHeapWasm;

Similar to above, please change the variable name to objectsNonHeapElementsWasm.

@@ +2126,5 @@
> +    }
> +
> +    size_t nonHeapWasm = classInfo.objectsNonHeapWasm;
> +    if (nonHeapWasm > 0) {
> +        REPORT_BYTES(path + NS_LITERAL_CSTRING("objects/non-heap/wasm"),

Similar to above, please change the path to "objects/non-heap/elements/wasm".

@@ +2129,5 @@
> +    if (nonHeapWasm > 0) {
> +        REPORT_BYTES(path + NS_LITERAL_CSTRING("objects/non-heap/wasm"),
> +            KIND_NONHEAP, nonHeapWasm,
> +            "wasm/asm.js memory allocated outside both the malloc heap and the "
> +            "GC heap.");

This is actually array elements, so "wasm/asm.js array buffer elements allocated outside both the malloc heap and the GC heap." would be good.
Attachment #8793219 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 23

a year ago
Thank you both for the reviews!

For maximal consistency, I've reordered the if/REPORT_BYTES in XPCJSContext.cpp, and removed the use of local variables nonHeapWasm, etc., since the rest of the function just does this, and the compiler can infer the value doesn't change since classInfo is marked const.

I've also removed the part of the comment relative to AsmJS (since the REPORT_BYTES is now far above this comment, and the line seems self-explanatory), and moved the part relative to guard pages further down, just above the if/REPORT_BYTES for wasm-guard-pages.

Comment 24

a year ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3b7a0422b6
Account memory for wasm guard pages; r=njn, r=luke

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e3b7a0422b6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as fix-optional. If it's worth uplifting to 51, feel free to nominate it.
status-firefox51: affected → fix-optional

Comment 28

a year ago
This commit resulted in the Memory devtool reporting wrong size for asm.js allocated pages, marked the regression down as bug https://bugzilla.mozilla.org/show_bug.cgi?id=1307768
You need to log in before you can comment on or make changes to this bug.