1303013 - Very large 14,269.36 MB ── vsize

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[alex_mayorga]

Attachment: 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

Comment 1

[alex_mayorga]

Attachment: memory-report-safe-mode.json.gz

¡Hola!

Safe mode doesn't seem to make any difference.

¡Gracias!
Alex

Comment 2

[alex_mayorga]

Attachment: 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

Comment 3

[Eric Rahm [:erahm]]

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.

Comment 4

[Eric Rahm [: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.

Comment 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Mike Hommey [:glandium]]

Benjamin, Luke, can you take a look at comment 5 and see what can be done to make that appear in about:memory somehow?

Comment 7

[Luke Wagner [:luke]]

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?

Comment 8

[Eric Rahm [:erahm]]

(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?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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)

Comment 10

[Luke Wagner [: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.

Comment 11

[Luke Wagner [:luke]]

(Thanks btw!)

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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!

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 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).

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 32bits.png

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

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

(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 16

[Nicholas Nethercote [inactive]]

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.

Comment 17

[Nicholas Nethercote [inactive]]

Thank you for taking this on, BTW. It's good to avoid mysteriously high numbers in about:memory :)

Comment 18

[Luke Wagner [:luke]]

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.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wasmguardpages.v2.patch

Comment 21

[Luke Wagner [: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 ...".

Comment 22

[Nicholas Nethercote [inactive]]

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.

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3b7a0422b6
Account memory for wasm guard pages; r=njn, r=luke

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e3b7a0422b6

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e3b7a0422b6

Comment 27

[Gerry Chang [:gchang]]

Mark 51 as fix-optional. If it's worth uplifting to 51, feel free to nominate it.

Comment 28

[Jukka Jylänki]

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