1540101 - JS memory is not garbage collected after using Promise.all()

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Jukka Jylänki]

When using Promise.all(), Firefox is unable to garbage collect memory that is pinned down by temporary JS scope. This affects all Emscripten compiled applications in Emscripten's new "minimal runtime", which utilizes Promise.all() to maximize parallel downloads.

STR:

1. (For good measure) Open a fresh instance of Firefox with no other tabs open.
2. Visit http://clb.confined.space/dump/firefox_promise_memory_leak/method_3.html
3. After the page loads and the cube is spinning, open a second tab and navigate it to about:memory
4. Tap on the buttons GC, CC, Minimize Memory usage a couple of times
5. Close the about:memory tab
6. Back in the original tab with the cube spinning, open Right Click -> Inspect Element -> Memory, and click on Take snapshot.

Observed:

The amount of memory used is reported 51.89MB. When opening the Dominators view, there are two ArrayBuffers of size 6385728 and 4710576 bytes that correspond to downloaded files release_15.wasm and release_15.data, which should have gotten collected, but were not.

Expected:

The amount of memory used should be 40.78MB. The typed arrays for temporary files release_15.wasm and release_15.data should have gotten freed.

In Chrome the memory leak does not occur, but Chrome does properly release the temporary ArrayBuffers.

This bug critically affects content deployed from Unity3D and Unity's new "Tiny Unity"/Dots engine, causing higher memory usage, since compiled WebAssembly code resides twice in memory (in raw bytes + compiled bytes)

Related STRs:

In step 2, replace the URL part "method_3.html" with "method_2.html" or "method_1.html" to find a variant of the code that does avoid the leak (although in those cases Firefox's Memory devtool gives a false report until next Garbage Collect occurs)

The difference between methods 1,2 and 3 is:

Method 1:

// Method 1: works ok, temporary memory gets garbage collected
s('release_15.js').then((js) => {
  b('release_15.wasm').then((wasm) => {
    b('release_15.data').then((data) => {
      run(js, wasm, data);
    })
  })
})

Method 2:

// Method 2: works ok, temporary memory gets garbage collected
Promise.all([s('release_15.js'), b('release_15.wasm'), b('release_15.data')]).then((r) => {
  run(r[0], r[1], r[2]);
});

Method 3:

// Method 3: buggy, temporary memory is not released even when forcibly GCing
Promise.all([s('release_15.js'), b('release_15.wasm'), b('release_15.data')]).then((r) => {
  ModuleImports = {
    wasmBinary: r[1],
    canvas: document.getElementById('canvas'),
    preRun: [loadData]
  };
  Module = r[0](ModuleImports);
  Module.data = r[2];
});

To examine the issue locally, download the relevant files from http://clb.confined.space/dump/firefox_promise_memory_leak/release_15.zip

Tested in Firefox Nightly 68.0a1 (2019-03-27) (64-bit) on Windows 10.

Comment 1

[André Bargull [:anba]]

It's interesting that this doesn't leak in Chrome, because the way "Method 3" is written, a leak has to be expected due to how ECMAScript works:

r[0](ModuleImports) is equivalent to r[0].call(r, ModuleImports), so the variable r is explicitly set as the this argument for the r[0] function (here: UnityModule). And because r is the array containing the ArrayBuffers, the buffers won't be released because they're kept alive through the UnityModule function invocation. This also explains why thers's no leak in "Method 2", because the call in "Method 2" is basically equivalent to r[0].call(undefined, ModuleImports), which means the this argument for the UnityModule function invocation is set to undefined [1].

A possible explanation why there's no leak in Chrome, could be that V8 has an extra optimisation in place to ignore any explicit this arguments for function calls if the function itself doesn't use this.

[1] Actually: undefind resp. the global object for non-strict functions.

Comment 2

[André Bargull [:anba]]

The debugger statement in asm2wasmImports prevents SpiderMonkey from omitting the this binding for UnityModule:

var asm2wasmImports = {
    "f64-rem": function(x, y) {
        return x % y
    },
    "debugger": function() {
        debugger
    }
};

Added some more in-detail infos to bug 1538375 comment 3 and also set bug 1538375 as a blocker for this issue.

Comment 3

[Jukka Jylänki]

Thanks for the details and diagnosis, very informative!

I can confirm that if I do

var js = r[0];
Module = js(ModuleImports);

then the memory leak does not occur. This works for now, although the above kind of transformation is something that Closure minifier is really keen to undo. (we are saved by the fact that we do not yet Closure minify JS code present in the main HTML file, though that is in the works)

How wide is the extent of deopts occurring by having the debugger; statement present in there? We can easily remove that from release builds if that helps codegen at present.

Comment 4

[André Bargull [:anba]]

(In reply to Jukka Jylänki from comment #3)

How wide is the extent of deopts occurring by having the debugger; statement present in there? We can easily remove that from release builds if that helps codegen at present.

From what I can tell, there are currently at least the following deoptimisations present when debugger is used.

For the function containing the debugger statement and transitively all functions containing that function (*):

• this binding can't be omitted (this bug).
• arguments object will always be created for function calls.
• All bindings are treated as being closed-over.
• CallObjects are created for each function call.

(*) That means for the test case: asm2wasmImports.debugger, UnityModule, and the IIFE creating UnityModule.

Comment 5

[Jason Orendorff [:jorendorff]]

Bug 1538375 landed, improving performance around the debugger statement, so comment 4 is stale.

I think the changes there cover the situation in this bug, but I'm not sure.