src: port coverage serialization to C++ by joyeecheung · Pull Request #26874 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation23 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

joyeecheung

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

@nodejs-github-bot

@joyeecheung

@bcoe

@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

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

@nodejs-github-bot

@nodejs-github-bot

@nodejs-github-bot

This was referenced

Apr 2, 2019

@nodejs-github-bot

@joyeecheung

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

Contributor

@bcoe bcoe left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

make coverage -j4

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

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

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

std::unique_ptrprofiler::V8CoverageConnection connection);
profiler::V8CoverageConnection* coverage_connection();
inline void set_coverage_directory(const char* directory);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@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

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.

@joyeecheung

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

@nodejs-github-bot

bcoe

bcoe approved these changes Apr 6, 2019

Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

joyeecheung added a commit that referenced this pull request

Apr 6, 2019

@joyeecheung

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

This was referenced

Apr 23, 2019

addaleax

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Labels

lib / src

Issues and PRs related to general changes in the lib or src directory.