Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: port coverage serialization to C++ #26874

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 23, 2019

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 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
Copy link
Member Author

joyeecheung commented Mar 23, 2019

@bcoe
Copy link
Contributor

bcoe commented Mar 25, 2019

@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
Copy link
Member Author

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

joyeecheung commented Apr 3, 2019

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

Copy link
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.

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_ptr<profiler::V8CoverageConnection> connection);
profiler::V8CoverageConnection* coverage_connection();

inline void set_coverage_directory(const char* directory);
Copy link
Contributor

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.

Copy link
Member Author

@joyeecheung joyeecheung Apr 4, 2019

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
Copy link
Contributor

ofrobots commented Apr 4, 2019

@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
Copy link
Member Author

joyeecheung commented Apr 4, 2019

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
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
Copy link
Member Author

Landed in 864860e

@joyeecheung joyeecheung closed this Apr 6, 2019
joyeecheung added a commit that referenced this pull request Apr 6, 2019
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>
// TODO(joyeecheung): white list the signals?

#if HAVE_INSPECTOR
profiler::EndStartedProfilers(env);
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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…

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants