Sitelet https://github.com/nodejs/node/issues/6180
Skip to content

Memory leak in simple test case involving WeakSet #6180

Closed
Closed
Memory leak in simple test case involving WeakSet#6180
Labels
memoryIssues and PRs related to Node.js memory management or memory footprint.v8 engineIssues and PRs related to the V8 dependency.

Description

@Ginden
Ginden
opened on Apr 13, 2016

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.

Activity

  1. ChALkeR commented on Apr 13, 2016

    @ChALkeR
    ChALkeR
    on Apr 13, 2016
    Member

    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.

  2. ChALkeR
    added
    v8 engineIssues and PRs related to the V8 dependency.
    memoryIssues and PRs related to Node.js memory management or memory footprint.
    on Apr 13, 2016

  3. vkurchatkin commented on Apr 13, 2016

    @vkurchatkin
    vkurchatkin
    on Apr 13, 2016
    Contributor

    @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

  4. ChALkeR commented on Apr 13, 2016

    @ChALkeR
    ChALkeR
    on Apr 13, 2016
    Member

    @vkurchatkin That, I don't know.

    /cc @jeisinger

  5. dnalborczyk commented on Apr 13, 2016

    @dnalborczyk
    dnalborczyk
    on Apr 13, 2016
    Contributor

    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

  6. ChALkeR commented on Apr 13, 2016

    @ChALkeR
    ChALkeR
    on Apr 13, 2016
    Member

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

  7. jeisinger commented on Apr 13, 2016

    @jeisinger
    jeisinger
    on Apr 13, 2016
    Contributor

    Are you running with 64 bit or 32 bit?

  8. dnalborczyk commented on Apr 13, 2016

    @dnalborczyk
    dnalborczyk
    on Apr 13, 2016
    Contributor

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

  9. dnalborczyk commented on Apr 13, 2016

    @dnalborczyk
    dnalborczyk
    on Apr 13, 2016
    Contributor

    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.

  10. jeisinger commented on Apr 13, 2016

    @jeisinger
    jeisinger
    on Apr 13, 2016
    Contributor

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

  11. jeisinger commented on Apr 14, 2016

    @jeisinger
    jeisinger
    on Apr 14, 2016
    Contributor

    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?

  12. benjamingr commented on Apr 15, 2016

    @benjamingr
    benjamingr
    on Apr 15, 2016
    Member

    @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.

  13. ChALkeR commented on Apr 15, 2016

    @ChALkeR
    ChALkeR
    on Apr 15, 2016
    Member

    @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

  14. jeisinger commented on Apr 15, 2016

    @jeisinger
    jeisinger
    on Apr 15, 2016
    Contributor

    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

  15. 31 remaining items

  16. rvagg
    added 3 commits that reference this issue on Oct 18, 2016

    deps: backport e093a04, 09db540 from upstream V8

    74d37d0

    deps: cherry-pick b93c80a from v8 upstream

    492baa9

    deps: backport a76d133 from v8 upstream

    93e5381
  17. MylesBorins
    added 3 commits that reference this issue on Oct 26, 2016

    deps: backport e093a04, 09db540 from upstream V8

    2d07fd7

    deps: cherry-pick b93c80a from v8 upstream

    e1f12fb

    deps: backport a76d133 from v8 upstream

    2b01bc8
  18. BethGriggs
    added 3 commits that reference this issue on Nov 9, 2016

    deps: backport e093a04, 09db540 from upstream V8

    b03629e

    deps: cherry-pick b93c80a from v8 upstream

    af2da0f

    deps: backport a76d133 from v8 upstream

    20aef57
  19. bernardmcmanus
    added 2 commits that reference this issue on May 28, 2017

    use mock weakset until nodejs/node#6180 is fixed

    a0b9da2

    Merge pull request #2 from BitClock/feature

    04591d7
  20. abernix
    added a commit that references this issue on Aug 14, 2017

    deps: cherry-pick 09db540,686558d from V8 upstream

    5f18fb2
  21. MylesBorins
    added a commit that references this issue on Aug 16, 2017

    deps: cherry-pick 09db540,686558d from V8 upstream

    5d7f68a
  22. MylesBorins
    added a commit that references this issue on Oct 25, 2017

    deps: cherry-pick 09db540,686558d from V8 upstream

    71f9cdf
  23. gibfahn
    added a commit that references this issue on Nov 24, 2017

    deps: cherry-pick 09db540,686558d from V8 upstream

    b77def2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    memoryIssues and PRs related to Node.js memory management or memory footprint.v8 engineIssues and PRs related to the V8 dependency.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions