src: port coverage serialization to C++ #26874

joyeecheung · 2019-03-23T01:28:45Z

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

nodejs-github-bot · 2019-03-23T01:28:47Z

@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

joyeecheung · 2019-03-23T01:29:19Z

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

bcoe · 2019-03-25T01:34:19Z

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

joyeecheung · 2019-04-01T00:03:08Z

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

nodejs-github-bot · 2019-04-01T00:05:01Z

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

nodejs-github-bot · 2019-04-01T00:26:26Z

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

nodejs-github-bot · 2019-04-01T03:51:39Z

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

nodejs-github-bot · 2019-04-02T00:19:33Z

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

nodejs-github-bot · 2019-04-03T22:35:08Z

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

joyeecheung · 2019-04-03T22:36:31Z

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

bcoe

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

bcoe · 2019-04-03T23:07:20Z

src/env.h

+      std::unique_ptr<profiler::V8CoverageConnection> connection);
+  profiler::V8CoverageConnection* coverage_connection();
+
+  inline void set_coverage_directory(const char* directory);


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.

ofrobots · 2019-04-04T00:06:38Z

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

joyeecheung · 2019-04-04T00:14:19Z

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.

nodejs-github-bot · 2019-04-04T21:34:16Z

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

nodejs-github-bot · 2019-04-04T23:28:33Z

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

bcoe

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.

joyeecheung · 2019-04-06T04:02:31Z

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>

addaleax · 2019-06-21T16:31:56Z

src/node_process_methods.cc

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


@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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: port coverage serialization to C++ #26874

src: port coverage serialization to C++ #26874

joyeecheung commented Mar 23, 2019 •

edited

Loading

nodejs-github-bot commented Mar 23, 2019

joyeecheung commented Mar 23, 2019 •

edited

Loading

bcoe commented Mar 25, 2019

joyeecheung commented Apr 1, 2019

nodejs-github-bot commented Apr 1, 2019

nodejs-github-bot commented Apr 1, 2019

nodejs-github-bot commented Apr 1, 2019

nodejs-github-bot commented Apr 2, 2019

nodejs-github-bot commented Apr 3, 2019

joyeecheung commented Apr 3, 2019 •

edited

Loading

bcoe left a comment •

edited

Loading

bcoe Apr 3, 2019

joyeecheung Apr 4, 2019 •

edited

Loading

ofrobots commented Apr 4, 2019 •

edited

Loading

joyeecheung commented Apr 4, 2019 •

edited

Loading

nodejs-github-bot commented Apr 4, 2019

nodejs-github-bot commented Apr 4, 2019

bcoe left a comment

joyeecheung commented Apr 6, 2019

addaleax Jun 21, 2019

joyeecheung Jun 25, 2019

addaleax Jun 25, 2019

joyeecheung Jun 25, 2019

src: port coverage serialization to C++ #26874

src: port coverage serialization to C++ #26874

Conversation

joyeecheung commented Mar 23, 2019 • edited Loading

Checklist

nodejs-github-bot commented Mar 23, 2019

joyeecheung commented Mar 23, 2019 • edited Loading

bcoe commented Mar 25, 2019

joyeecheung commented Apr 1, 2019

nodejs-github-bot commented Apr 1, 2019

nodejs-github-bot commented Apr 1, 2019

nodejs-github-bot commented Apr 1, 2019

nodejs-github-bot commented Apr 2, 2019

nodejs-github-bot commented Apr 3, 2019

joyeecheung commented Apr 3, 2019 • edited Loading

bcoe left a comment • edited Loading

Choose a reason for hiding this comment

bcoe Apr 3, 2019

Choose a reason for hiding this comment

joyeecheung Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

ofrobots commented Apr 4, 2019 • edited Loading

joyeecheung commented Apr 4, 2019 • edited Loading

nodejs-github-bot commented Apr 4, 2019

nodejs-github-bot commented Apr 4, 2019

bcoe left a comment

Choose a reason for hiding this comment

joyeecheung commented Apr 6, 2019

addaleax Jun 21, 2019

Choose a reason for hiding this comment

joyeecheung Jun 25, 2019

Choose a reason for hiding this comment

addaleax Jun 25, 2019

Choose a reason for hiding this comment

joyeecheung Jun 25, 2019

Choose a reason for hiding this comment

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