src: port coverage serialization to C++ by joyeecheung · Pull Request #26874 · nodejs/node · GitHub

This patch moves the serialization of coverage profiles into
C++. With this we no longer need to patch process.reallyExit
and hook into the exit events, but instead hook into relevant
places in C++ which are safe from user manipulation. This also
makes the code easier to reuse for other types of profiles.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3018/pipeline

CI: https://ci.nodejs.org/job/node-test-pull-request/21818/
CI: https://ci.nodejs.org/job/node-test-pull-request/21819/

@joyeecheung fascinating, I'll provide a thorough code review sometime tomorrow (including checking out the branch and testing). Assuming that collecting coverage continues to work as expected for the Node.js codebase and user-land modules, this refactor seems like a no-brainer.

Great work.

I updated this patch to accommodate #26544 - we now must use env->env_vars() to query NODE_V8_COVERAGE for workers. In addition I removed the inheritance of BaseObject from V8InspectorConnection, since now the entire functionality is implemented in C++, we can just manage the lifetime of the connections along with Environment.

cc @addaleax

CI: https://ci.nodejs.org/job/node-test-pull-request/22083/

CI: https://ci.nodejs.org/job/node-test-pull-request/22084/

CI: https://ci.nodejs.org/job/node-test-pull-request/22085/

CI: https://ci.nodejs.org/job/node-test-pull-request/22106/

CI: https://ci.nodejs.org/job/node-test-pull-request/22183/

Ping @bcoe have you gotten around to this?

also trying to get more reviews... I want to land this sooner than later to unblock #26878

@nodejs/v8-inspector @nodejs/process @nodejs/fs

I'm running into some issues attempting to run with:

make coverage -j4

the test sequential/test-inspector-contexts seems to hang indefinitely.

☝️ this seems to be happening to me on master too.

running the c8 reporting library seems to fail due to memory issues:

Security context: 0x298904f1a281 <JSObject>
    0: builtin exit frame: parse(this=0x298904f0ab61 <Object map = 0x298980501449>,0x298977288139 <Very long string[204639]>,0x298904f0ab61 <Object map = 0x298980501449>)

    1: /* anonymous */(aka /* anonymous */) [0x29893a080139] [/Users/bencoe/oss/node/node_modules/c8/lib/report.js:102] [bytecode=0x2989343531b9 offset=59](this=0x29896de004d1 <undefined>,0x29893a0e4ef1 <String[34]: coverage-8...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0x100068dc2 node::Abort() [/Users/bencoe/oss/node/./node]
 2: 0x10006943a node::OnFatalError(char const*, char const*) [/Users/bencoe/oss/node/./node]

are you bumping into this issue with make coverage; I can try on my home computer as well (It's worth noting this also happened to me on master).

will the directory passed in to set_coverage_directory have been resolved? I think this could protect against some issues that crop up if the cwd is changed by a subprocess.

This is set as usual during pre-execution (but it's called from JS to persist states in C++ now), so yes it is resolved.

@bcoe The OOM is probably related to the heap V8 has allocated rather than physical memory size. Try a bigger heap e.g. --max-old-space-size=2048.

are you bumping into this issue with make coverage; I can try on my home computer as well (It's worth noting this also happened to me on master).

I ran into this before, try cleaning up the .coverage your in build directory - if you run the command multiple times, it tries to combine all the results from previous runs so if you don't clean them up, the coverage data will grow forever. The default heap size only happens to be able to accommodate the coverage data generated from one single run of Node.js core coverage.

This patch moves the serialization of coverage profiles into C++. With this we no longer need to patch `process.reallyExit` and hook into the exit events, but instead hook into relevant places in C++ which are safe from user manipulation. This also makes the code easier to reuse for other types of profiles.

CI: https://ci.nodejs.org/job/node-test-pull-request/22205/

CI: https://ci.nodejs.org/job/node-test-pull-request/22210/

You were correct that the underlying issue was not cleaning up Release/.coverage doh.

I've tested locally on yargs, and things seem to work better than ever 👍

Thanks for the hard work.

Landed in 864860e

This patch moves the serialization of coverage profiles into C++. With this we no longer need to patch `process.reallyExit` and hook into the exit events, but instead hook into relevant places in C++ which are safe from user manipulation. This also makes the code easier to reuse for other types of profiles. PR-URL: #26874 Reviewed-By: Ben Coe <bencoe@gmail.com>

@joyeecheung Should this be guarded to only be run if sig > 0 and pid is one of 0, -1, getpid(), -getpid() or something similar?

I think we should only write the profiles when we are sure the kill is initiated to terminate the process, are those conditions enough for us to be sure about that?

@joyeecheung I mean, some signals that would usually terminate the process can be caught (like SIGINT), so we cannot be sure of whether this is going to terminate the process or not no matter what…

Maybe we can only take care of signals that don't have a handler attached through Node.js's APIs, though even then it would get complicated because of workers. Or we could just not guard this at all, or let the user decide somehow, but then no one has reported any issues with this so I don't know what the real-world use cases would be.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 23, 2019

joyeecheung requested a review from bcoe March 23, 2019 01:29

joyeecheung mentioned this pull request Mar 23, 2019

RFC: add a flag similar to --prof, but based on v8 CPU profiler #26878

Closed

joyeecheung force-pushed the coverage-serialize branch from 7a88639 to 3768e82 Compare April 1, 2019 00:00

joyeecheung force-pushed the coverage-serialize branch from 3768e82 to 14147bb Compare April 1, 2019 00:25

This was referenced Apr 2, 2019

CI failures: 20190330-20190402 nodejs/reliability#22

Closed

Investigate flaky test-fs-readfile-tostring-fail #16601

Closed

joyeecheung force-pushed the coverage-serialize branch from 29b45ea to 8a8d45a Compare April 3, 2019 22:34

bcoe reviewed Apr 4, 2019

View reviewed changes

joyeecheung force-pushed the coverage-serialize branch from 8a8d45a to a63c7f2 Compare April 4, 2019 21:29

bcoe approved these changes Apr 6, 2019

View reviewed changes

joyeecheung closed this Apr 6, 2019

beevelop mentioned this pull request Apr 23, 2019

2019-04-23 Version 12.0.0 (Current) @BethGriggs beevelop/docker-android-nodejs#302

Closed

addaleax reviewed Jun 21, 2019

View reviewed changes

shobhitchittora mentioned this pull request Sep 13, 2019

NODE_V8_COVERAGE does not fail gracefully when inspector is disabled #29542

Closed

+                  // TODO(joyeecheung): white list the signals?
+              #if HAVE_INSPECTOR
+                profiler::EndStartedProfilers(env);

src: port coverage serialization to C++ by joyeecheung · Pull Request #26874 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: port coverage serialization to C++ - #26874

src: port coverage serialization to C++#26874joyeecheung wants to merge 1 commit intonodejs:masternodejs/node:masterfrom joyeecheung:coverage-serializejoyeecheung/node:coverage-serializeCopy head branch name to clipboard

Conversation

joyeecheung commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Mar 23, 2019

Uh oh!

joyeecheung commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe commented Mar 25, 2019

Uh oh!

joyeecheung commented Apr 1, 2019

Uh oh!

nodejs-github-bot commented Apr 1, 2019

Uh oh!

nodejs-github-bot commented Apr 1, 2019

Uh oh!

nodejs-github-bot commented Apr 1, 2019

Uh oh!

nodejs-github-bot commented Apr 2, 2019

Uh oh!

nodejs-github-bot commented Apr 3, 2019

Uh oh!

joyeecheung commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoe Apr 3, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofrobots commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 4, 2019

Uh oh!

nodejs-github-bot commented Apr 4, 2019

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Apr 6, 2019

Uh oh!

addaleax Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

src: port coverage serialization to C++#26874
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:coverage-serialize

joyeecheung commented Mar 23, 2019 •
edited
Loading

joyeecheung commented Mar 23, 2019 •
edited
Loading

joyeecheung commented Apr 3, 2019 •
edited
Loading

bcoe left a comment •
edited
Loading

joyeecheung Apr 4, 2019 •
edited
Loading

ofrobots commented Apr 4, 2019 •
edited
Loading

joyeecheung commented Apr 4, 2019 •
edited
Loading