Node: Memory leak in simple test case involving WeakSet

[Ginden]

Version: 4.2.1, 5.1.10 confirmed.
Platform: at least Windows 10 and Linux

`Following code reproduces error:

'use strict';

const s = new WeakSet();
let j = 0;
function f(){
    for (let i = 0 ; i<100000; i++) {
        s.add({});  
    }
    console.log(`${j++} :${0|(process.memoryUsage().heapUsed/(1024*1024))}MB`);
    return Promise.resolve(null).then(f);
}
f();

It ends with following output

...
110 :853MB

<--- Last few GCs --->

   10993 ms: Scavenge 825.2 (856.1) -> 823.2 (863.1) MB, 49.7 / 0 ms [allocation failure].
   11126 ms: Scavenge 832.1 (863.1) -> 830.1 (870.1) MB, 43.4 / 0 ms [allocation failure].
   11255 ms: Scavenge 839.0 (870.1) -> 837.0 (877.1) MB, 44.6 / 0 ms [allocation failure].
   11390 ms: Scavenge 845.8 (877.1) -> 843.9 (884.1) MB, 47.5 / 0 ms [allocation failure].
   11517 ms: Scavenge 852.7 (884.1) -> 850.8 (891.1) MB, 42.5 / 0 ms [allocation failure].


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0000029D869E3AD1 <JS Object>
    1: add [native weak-collection.js:~92] [pc=0000036347B55F9B] (this=000003208D656691 <JS WeakSet>,k=00000167E7F946E9 <an Object with map 0000027C0AC172F1>)
    2: f(aka f) [C:\Users\michal.wadas\Documents\test_gc.js:7] [pc=0000036347B536C1] (this=0000029D86904189 <undefined>)
    3: arguments adaptor frame: 1->0
    4: /* anonymous */(aka /* anonymous */) [native promise.js:221] [pc=000003...

Objects are properly garbage collected in Chrome 49 (process.memoryUsage().heapUsed is changed to performance.memory.usedJSHeapSize).

Source: StackOverflow question.

[ChALkeR]

The problem is that somewhy only scavenges are triggered here, but mark-sweeps aren't.
And scavenges can't collect those elements (for some reason).

In most real-world apps this wouldn't be a problem, because there are many things that push mark-sweep runs.

This is most likely already fixed in v8, if Chrome 49 is fine.
We should check how master is doing, I will be able to check that a bit later.

[vkurchatkin]

@ChALkeR is there write-up on which heuristics are used by v8 for different types of GC? It seems that in node v5 and earlier WeakMap and WeakSet don't apply any pressure on garbage collector at all

[ChALkeR]

@vkurchatkin That, I don't know.

/cc @jeisinger

[dnalborczyk]

This is most likely already fixed in v8, if Chrome 49 is fine.

No garbage collection in master with v8 4.9.385.35 and in vee-eight-5.0 with v8 5.0.71.25 on OSX El Capitan 10.11.5 Beta 1, XCode v7.3

Daniel

[ChALkeR]

@dnalborczyk Same as in it gets garbage collected or not?

[jeisinger]

Are you running with 64 bit or 32 bit?

[dnalborczyk]

@ChALkeR Oh, sorry, same as in the original issue description. No garbage collection. I corrected the above.

[dnalborczyk]

Btw, if it helps, Chrome 51.0.2700.0 dev is eventually smoking up as well - though the heap size seems to remain the same ( _if the output happens to be correct_ ). Firefox 47, Edge 14 (37), and Safari 9.1.1 keep running.

[jeisinger]

filed https://bugs.chromium.org/p/v8/issues/detail?id=4909 to track this.

[jeisinger]

btw, can you share a non-artificial example? I.e. something where you manage to fill up a weak set in a meaningful way without generating enough garbage anyways to trigger a full gc?

[benjamingr]

@ChALkeR dumb question - but can't/should't node trigger a full GC before throwing an out of memory error? I'd actually expect v8 to do that.

[ChALkeR]

@benjamingr That's v8 job and somewhy it doesn't do it here. It's mentioned in the issue that @jeisinger filed to v8 issue tracker:

Maybe we should try an emergency GC before aborting in the case of weakmaps and weaksets

[jeisinger]

landed a fix in v8. turns out there was no code to actually free up GC'd slots in a weak collection... gives you an idea how much this feature is used on the web.

anyways, should probably be backmerged to node's lts version of v8

[targos]

Link to the fix: https://codereview.chromium.org/1877233005

[jeisinger]

turns out that at least one unit test, doesn't like the fix :)... investigating

[ChALkeR]

https://github.com/nodejs/node/pull/6375#issuecomment-214672366 by @jeisinger:

The WeakMap issue should be fixed here: https://codereview.chromium.org/1890123002

[benjamingr]

Any chance the fix could be backported? @ChALkeR

[ChALkeR]

/cc @bnoordhuis, @ofrobots