8000 fix!: re-create CPU profiler each time a CPU profile is collected to work around V8 CPU profiler memory leak. by nolanmar511 · Pull Request #142 · google/pprof-nodejs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix!: re-create CPU profiler each time a CPU profile is collected to work around V8 CPU profiler memory leak. #142

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 29, 2020
Merged

fix!: re-create CPU profiler each time a CPU profile is collected to work around V8 CPU profiler memory leak. #142

merged 2 commits into from
Oct 29, 2020

Conversation

nolanmar511
Copy link
Contributor
@nolanmar511 nolanmar511 commented Oct 28, 2020 •
edited
Loading

Work around memory leak in V8 CPU profiler (https://bugs.chromium.org/p/v8/issues/detail?id=11051) by creating and destroying the CPU profiler each time a CPU profile is collected.

BREAKING:
With this change, an error will now occur when one attempts to collect 2 CPU profiles in parallel.

TESTED: Confirmed pprof-nodejs no longer had memory leak by running small express application with a version of pprof-nodejs without this change and with the currently release version. When running this application with Node 14 overnight, a memory leak was no longer apparent. I added log statements to confirm the fix is included when this native portion is compiled for Node 12.

Other potential consequences of this change:

  • I expect both the overhead of CPU profiling and the time spent in calls to start and stop CPU profiling (these calls block the event loop) to increase.
  • Native code for CPU profiling is no longer thread safe. This should not be an issue, because Node.js is single threaded and calls to start/stop CPU profiling will block the event loop.

Measurements from this change:

  • Node 12.14.1; collecting profiles w/ currently released version of pprof-nodejs.
  • Node 12.14.1; collecting profiles w/ version of pprof-nodejs including this fix.
  • Node 12.14.1; pprof-nodejs required, but no profile collection.
  • Node 12.19.0; collecting profiles w/ currently released version of pprof-nodejs.
  • Node 12.19.0; collecting profiles w/ version of pprof-nodejs including this fix.
  • Node 12.19.0; pprof-nodejs required, but no profile collection.

Applications used attached (using an express application which returns between the 20th and 30th value in the Fibonacci's sequence on each request, and application sends queries to that app).

Results (RSS sampled ~200 times throughout run of each application; all applications run at same time):

  • Node 12.14.1 (version of Node.js w/o leak)

    |                                                             |12.14.1; no pprof|12.14.1;  original pprof|12.14.1; pprof with fix|
|-------------------------------------------------------------|-----------------|------------------------|-----------------------|
|Avg RSS (MiB)                                                |108.1552407      |118.7069561             |119.2503501            |
|T-test (unpaired; two tailed), Compare against no pprof      |1                |0                       |0                      |
|T-test (unpaired; two tailed), Compare against original pprof|0                |1                       |0.07586016786          |

    |                              |    12.14.1; no pprof|12.14.1;  original pprof|12.14.1; pprof with fix|
|------------------------------|---------------------|------------------------|-----------------------|
|Avg QPS                       |1.630026753          |1.605802218             |1.627270883            |
|Compare against no pprof      |1                    |0.000007524927747       |0.0113245978           |
|Compare against original pprof|0.007518199978       |1                       |0.04419195895          |

  • Node 12.19.0 (version of Node.js w/ leak):

    |                              |    12.19.0; no pprof|    12.19.0;  original pprof|    12.19.0; pprof with fix|
|------------------------------|---------------------|----------------------------|---------------------------|
|Avg RSS (MiB)                |106.7471331          |131.2334727                 |110.398869                 |
|Compare against no pprof      |1                    |0                           |0                          |
|Compare against original pprof|0                    |1                           |0                          |

    |                              |    12.19.0; no pprof|    12.19.0;  original pprof|    12.19.0; pprof with fix|
|------------------------------|---------------------|----------------------------|---------------------------|
|Avg QPS                       |1.602044023          |1.602805889                 |1.606838462                |
|Compare against no pprof      |1                    |0.8827788704                |0.5445208298               |
|Compare against original pprof|0.8827788704         |1                           |0.4504291645               |

Looking at the charts above, it doesn't seem like performance w/ and w/o fix is very different.

express_app.js.txt
express_app-no-pprof.js.txt

@nolanmar511 nolanmar511 changed the title fix!: work around V8 memory leak fix!: re-create CPU profiler each time a CPU profile is collected to work around V8 CPU profiler memory leak. Oct 28, 2020
@kalyanac
Copy link

If we are publishing this, we should at least measure the overhead of repeated creation and deletion of the CPU profiler.

  1. Run an app with Node.js which does not have the memory leak issue.
  2. Run same as (1) but with Profiler.
  3. Run same as (1) with a Node.js version that has the memory leak.
  4. Run same as (3) but with Profiler which includes this workaround.
  5. Compare timings across all. We can still publish it but document the overhead, if any.

Otherwise, this PR LGTM.

@nolanmar511 nolanmar511 requested review from kalyanac and removed request for kalyanac October 28, 2020 20:55
@nolanmar511
Copy link
Contributor Author

@kalyanac -- I've updated description with testing I've done.

RSS is slightly higher w/ version of pprof-nodejs with the fix (relative to current pprof release). QPS is also slightly higher w/ version of pprof-nodejs (relative to current pprof release); but even statistically significant differences seem slight.

@kalyanac
Copy link

Thank you for the additional data.

All the values could be rounded down to fewer significant digits for readability. For 12.19.0, the first row is just AVERAGE, it should be Avg RSS (MiB).

kalyanac
kalyanac approved these changes Oct 28, 2020
View reviewed changes
@nolanmar511 nolanmar511 force-pushed the v8-workaround branch 3 times, most recently from f3436dd to d0504ca Compare October 29, 2020 19:04
@codecov-io
Copy link
codecov-io commented Oct 29, 2020 •
edited
Loading

Codecov Report

Merging #142 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #142   +/-   ##
=======================================
  Coverage   42.21%   42.21%           
=======================================
  Files          14       14           
  Lines        2061     2061           
  Branches       42       42           
=======================================
  Hits          870      870           
  Misses       1173     1173           
  Partials       18       18

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b83cf4...9612cc8. Read the comment docs.

@nolanmar511 nolanmar511 merged commit 57f766f into google:master Oct 29, 2020
@nolanmar511 nolanmar511 deleted the v8-workaround branch October 29, 2020 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalyanac kalyanac kalyanac approved these changes

@macieksmuga macieksmuga Awaiting requested review from macieksmuga

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nolanmar511 @kalyanac @codecov-io
0