Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node.js leaks memory when creating classes in a loop. #22229

Closed
hashseed opened this issue Aug 10, 2018 · 6 comments
Closed

Node.js leaks memory when creating classes in a loop. #22229

hashseed opened this issue Aug 10, 2018 · 6 comments
Labels
perf_hooks Issues and PRs related to the implementation of the Performance Timing API.

Comments

@hashseed
Copy link
Member

hashseed commented Aug 10, 2018

Steps to reproduce:

$ cat test.js
'use strict';
for (var i = 0;; ++i) {
  if (i % 100 == 0) gc();
  new class{};
}

$ node --expose-gc test.js & top -p $! && kill -9 $!  # use q to quit

Observe reserved memory slowly grow. This will eventually cause out-of-memory.

This was first observed here. Note that this does not reproduce with d8 (V8's test shell), where the memory use stabilizes fairly quickly.

@bounc3-paradise-on-e
Copy link

Thanks for filing this issue.

Just a small correction (to avoid possible confusion): if (i % 100 == 0) - there's no variable i. If we want to call gc every 100th iteration, we can use for (var i = 0;; i = -~i) instead of for (;;)

@hashseed
Copy link
Member Author

Yes, you are right. I fixed the test.

@addaleax
Copy link
Member

addaleax commented Aug 10, 2018

Any chance the RSS increase is caused by running the GC, rather than the new class part? That’s the only part I see where some Node-specific behaviour kicks in, and the RSS grows slows down if I decrease the GC frequency…

(edit: confirmed.)

@addaleax addaleax added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Aug 10, 2018
@addaleax
Copy link
Member

This could do the trick here:

diff --git a/src/node_perf.cc b/src/node_perf.cc
index 5a50223ed593..8aad8ca1a705 100644
--- a/src/node_perf.cc
+++ b/src/node_perf.cc
@@ -272,6 +272,8 @@ void MarkGarbageCollectionEnd(Isolate* isolate,
                               v8::GCCallbackFlags flags,
                               void* data) {
   Environment* env = static_cast<Environment*>(data);
+  if (!env->performance_state()->observers[NODE_PERFORMANCE_ENTRY_TYPE_GC])
+    return;
   GCPerformanceEntry* entry =
       new GCPerformanceEntry(env,
                              static_cast<PerformanceGCKind>(type),

@hashseed
Copy link
Member Author

That might be right :)

@jasnell
Copy link
Member

jasnell commented Aug 10, 2018

I'll open PR

jasnell added a commit to jasnell/node that referenced this issue Aug 10, 2018
@gdams gdams closed this as completed in f6eab1a Aug 13, 2018
rvagg pushed a commit that referenced this issue Aug 15, 2018
Fixes: #22229

PR-URL: #22241
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf_hooks Issues and PRs related to the implementation of the Performance Timing API.
Projects
None yet
Development

No branches or pull requests

4 participants