Closed Hollerberg closed 6 years ago
/cc @nodejs/v8
/cc @nodejs/diagnostics
I can confirm the issue and came to the same conclusion, that it's the ProfilerListener that is leaking the memory. A straightforward patch (see below) breaks a number of V8 tests however so this needs some more digging.
diff --git a/deps/v8/src/log.cc b/deps/v8/src/log.cc
index 7f029d8..57cd522 100644
--- a/deps/v8/src/log.cc
+++ b/deps/v8/src/log.cc
@@ -1854,6 +1854,7 @@ void Logger::SetUpProfilerListener() {
void Logger::TearDownProfilerListener() {
if (profiler_listener_->HasObservers()) return;
removeCodeEventListener(profiler_listener_.get());
+ profiler_listener_.reset();
}
sampler::Sampler* Logger::sampler() {
Turns out the ProfilerListener is still used after TearDownProfilerListener()...
I have a new patch that passes V8's and node's test suites but it's a little gross. I'll see if I can spruce it up before I send it upstream.
diff --git a/deps/v8/src/profiler/cpu-profiler.cc b/deps/v8/src/profiler/cpu-profiler.cc
index 85f9d5e..3e7c474 100644
--- a/deps/v8/src/profiler/cpu-profiler.cc
+++ b/deps/v8/src/profiler/cpu-profiler.cc
@@ -275,6 +275,8 @@ void CpuProfiler::set_sampling_interval(base::TimeDelta value) {
void CpuProfiler::ResetProfiles() {
profiles_.reset(new CpuProfilesCollection(isolate_));
profiles_->set_cpu_profiler(this);
+ Logger* logger = isolate_->logger();
+ logger->profiler_listener_.reset();
}
void CpuProfiler::CreateEntriesForRuntimeCallStats() {
@bnoordhuis thanks a lot for your effort! is there a chance that this patch will be included into one of the next Node.js releases?
I am seeing this issue too with node 7 and 8. I see that the bug is closed. Which node release is the fix present in ?
@interviewQ It was just fixed in Node.js yesterday (https://github.com/nodejs/node/pull/17512) so it will come with the next Node.js v8.x and v9.x. We no longer maintain v7.x.
Closing, since https://github.com/nodejs/node/pull/17512 fixes this.
Actually Node 9.3.0 released earlier this week should already contain the fix (please give it a shot and report back). For Node 8 the fix will be in the next release.
Thanks for the response. Would this get fixed in node 7 ?
Amaranth
On Thu, Dec 14, 2017 at 3:58 PM, Ali Ijaz Sheikh notifications@github.com wrote:
Actually Node 9.3.0 released earlier this week should already contain the fix (please give it a shot and report back). For Node 8 the fix will be in the next release.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/14894#issuecomment-351872616, or mute the thread https://github.com/notifications/unsubscribe-auth/ASSkiQg8Ok5-eaj6NZ3aUuHlrmDMaYgvks5tAbZAgaJpZM4O6Wql .
Sorry, I missed your earlier message that this will not be fixed in node 7.
On Thu, Dec 14, 2017 at 4:10 PM, Amarnath Shanbhag < amarnathshanbhag@gmail.com> wrote:
Thanks for the response. Would this get fixed in node 7 ?
Amaranth
On Thu, Dec 14, 2017 at 3:58 PM, Ali Ijaz Sheikh <notifications@github.com
wrote:
Actually Node 9.3.0 released earlier this week should already contain the fix (please give it a shot and report back). For Node 8 the fix will be in the next release.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nodejs/node/issues/14894#issuecomment-351872616, or mute the thread https://github.com/notifications/unsubscribe-auth/ASSkiQg8Ok5-eaj6NZ3aUuHlrmDMaYgvks5tAbZAgaJpZM4O6Wql .
Dear Node community, I am filing this ticket to make you aware of a memory leak in v8::CpuProfiler introduced in v8 versions used by Node 7.x / 8.x. My company uses v8::CpuProfiler APIs to render periodic samples of the running application and I noticed a continuous RSS size increase with Node.js version 7.x and 8.x. Earlier versions are not affected. I reported the issue a few weeks back in v8 monorail (https://bugs.chromium.org/p/v8/issues/detail?id=6623) but it did not receive too much attention so far.