process: start coverage collection before bootstrap #26006

joyeecheung · 2019-02-08T12:53:28Z

This patch moves the dispatch of Profiler.takePreciseCoverage
to a point before the bootstrap scripts are run to ensure that
we can collect coverage data for all the scripts run after
the inspector agent is ready.

Before this patch lib/internal/bootstrap/primordials.js was not
covered by make coverage, after this patch it is.

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

This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is.

nodejs-github-bot · 2019-02-08T12:53:30Z

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

joyeecheung · 2019-02-08T12:55:38Z

This also seems to fix the async-hooks/test-callback-error error in make coverage for me - probably because it no longer triggers the creation of an additional JSBindingsConnection (AsyncWrap) anymore.

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

src/inspector_coverage.cc

joyeecheung · 2019-02-08T14:36:52Z

Addressed reviews from @addaleax and fixed the quotes in the warning. Also I notice that sometimes undefined could be written to the file, so I've restored to an approach similar to the previous one and always overwrite the file in the callback once the callback is set.

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

targos · 2019-02-08T15:41:14Z

lib/internal/coverage-gen/with_profiler.js

-    }));
-    const coverageInfo = JSON.parse(msg).result;
-    writeFileSync(target, JSON.stringify(coverageInfo));
+    internalBinding('coverage').end((msg) => {


this seems to be async now? should the try/catch be moved to the callback?

bcoe

this looks good to me, and I'm excited that we'll be able to collect coverage for a few of the internal libraries we're missing right now.

Before we land this, I'd like to test the branch with a few user-land modules, paying special attention to how subprocesses are handled. I will have cycles to do so this weekend.

bcoe · 2019-02-08T21:55:36Z

src/inspector_coverage.cc

+    explicit V8CoverageSessionDelegate(V8CoverageConnection* connection)
+        : connection_(connection) {}
+
+    void SendMessageToFrontend(


nice, seems like a huge benefit to move coverage standup and teardown into C++.

addaleax · 2019-02-09T18:39:23Z

Looks like there are related failures in CI:

Windows:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

134 !== 0

    at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-v8-coverage.js:92:10)

--without-intl:

/home/iojs/build/workspace/node-test-commit-linux-containered/out/Release/obj.target/node_lib/src/node_binding.o: In function `node::binding::RegisterBuiltinModules()':
node_binding.cc:(.text+0x14ae): undefined reference to `_register_coverage()'
collect2: error: ld returned 1 exit status

joyeecheung · 2019-02-12T03:24:49Z

The failures on Windows:

C:\Users\joyee\projects\node>C:\Users\joyee\projects\node\Release\node.exe C:\Users\joyee\projects\node\test\parallel\test-v8-coverage.js
uv loop at [000001D549520AD0] has 2 active handles
[000001D54AF20CE0] async
        Close callback: 00007FF6CDFC2C40 <lambda_1e91a1e10c4ec4799db2c780f02b0099>::<lambda_invoker_cdecl> [c:\users\joyee\projects\node\src\node_platform.cc]:L275
        Data: 000001D54AEE2830
        (First field): 00019A740001A116
[000001D54AF21730] async
        Close callback: 00007FF6CDF4D1A0 node::inspector::`anonymous namespace'::DisposePairCallback [c:\users\joyee\projects\node\src\inspector\main_thread_interface.cc]:L99
        Data: 0000000000000000
Command Prompt - C:\Users\joyee\projects\node\Release\node.exe  C:\Users\joyee\projects\node\test\parallel\test-v8-coverage.js[7760]: c:\users\joyee\projects\node\src\debug_utils.cc:290: Assertion `0 && "uv_loop_close() while having open handles"' failed.
 1: 00007FF6CE0A723F node::DumpBacktrace+143 [c:\users\joyee\projects\node\src\debug_utils.cc]:L276
 2: 00007FF6CE047BD6 node::Abort+22 [c:\users\joyee\projects\node\src\node_errors.cc]:L166
 3: 00007FF6CE0481A3 node::Assert+131 [c:\users\joyee\projects\node\src\node_errors.cc]:L183
 4: 00007FF6CE0A6E28 node::CheckedUvLoopClose+216 [c:\users\joyee\projects\node\src\debug_utils.cc]:L290
 5: 00007FF6CDFA251E node::worker::Worker::`scalar deleting destructor'+126
 6: 00007FF6CE0A008D node::Environment::RunCleanup+701 [c:\users\joyee\projects\node\src\env.cc]:L524
 7: 00007FF6CE073836 node::Start+1334 [c:\users\joyee\projects\node\src\node.cc]:L831
 8: 00007FF6CE0731EE node::Start+382 [c:\users\joyee\projects\node\src\node.cc]:L877
 9: 00007FF6CE072D30 node::Start+1040 [c:\users\joyee\projects\node\src\node.cc]:L937
10: 00007FF6CDECC58C wmain+444 [c:\users\joyee\projects\node\src\node_main.cc]:L72
11: 00007FF6CEFE819C __scrt_common_main_seh+268 [d:\agent\_work\3\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl]:L288
12: 00007FF8B5643034 BaseThreadInitThunk+20
13: 00007FF8B7F13691 RtlUserThreadStart+33

I'll try looking into it later. (Meanwhile the test probably needs to reverse the order of checks to print something more helpful on failures..)

bcoe · 2019-02-12T07:15:21Z

quick update, haven't tested on any user-land modules yet; compiling the branch right now and will test tomorrow.

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: nodejs#26089 Refs: nodejs#26006

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: nodejs#26089 Refs: nodejs#26006

addaleax · 2019-02-15T18:57:04Z

I haven’t tried it for this PR myself (yet), but I think #26138 and #26137 should take care of the Windows failures?

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

addaleax · 2019-02-17T23:07:47Z

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

bcoe · 2019-02-18T02:25:06Z

@addaleax @joyeecheung I apologize for the slow turnaround, I've tested this change on a few user-land modules and it's working great 👍

as soon as tests pass, +1 from me.

joyeecheung · 2019-02-18T05:41:06Z

Added HAVE_INSPECTOR guard for the coverage binding.

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

joyeecheung · 2019-02-18T09:43:33Z

Landed in 2ae45d3

This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is. PR-URL: #26006 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: James M Snell <[email protected]>

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is. PR-URL: #26006 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: James M Snell <[email protected]>

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is. PR-URL: #26006 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: James M Snell <[email protected]>

joyeecheung requested a review from bcoe February 8, 2019 12:53

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 8, 2019

addaleax approved these changes Feb 8, 2019

View reviewed changes

src/inspector_coverage.cc Outdated Show resolved Hide resolved

src/inspector_coverage.cc Outdated Show resolved Hide resolved

src/inspector_coverage.cc Outdated Show resolved Hide resolved

joyeecheung added 2 commits February 8, 2019 22:24

fixup! process: start coverage collection before bootstrap

0bf9ae9

fixup! fixup! process: start coverage collection before bootstrap

44ff18b

targos reviewed Feb 8, 2019

View reviewed changes

bcoe approved these changes Feb 8, 2019

View reviewed changes

jasnell approved these changes Feb 12, 2019

View reviewed changes

addaleax mentioned this pull request Feb 15, 2019

test,inspector: add heap allocation tracker test #26089

Closed

3 tasks

addaleax mentioned this pull request Feb 15, 2019

src: remove inspector main_thread_request_ field #26137

Closed

2 tasks

addaleax added a commit to addaleax/node that referenced this pull request Feb 15, 2019

worker: spin uv_run twice before closing loop

06e09f7

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: nodejs#26089 Refs: nodejs#26006

addaleax mentioned this pull request Feb 15, 2019

worker: spin uv_run twice before closing loop #26138

Closed

2 tasks

fixup! fixup! fixup! process: start coverage collection before bootstrap

de2845b

joyeecheung closed this Feb 18, 2019

BridgeAR mentioned this pull request Feb 26, 2019

v11.11.0 proposal #26322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: start coverage collection before bootstrap #26006

process: start coverage collection before bootstrap #26006

joyeecheung commented Feb 8, 2019

nodejs-github-bot commented Feb 8, 2019

joyeecheung commented Feb 8, 2019

joyeecheung commented Feb 8, 2019

targos Feb 8, 2019

bcoe left a comment

bcoe Feb 8, 2019

addaleax commented Feb 9, 2019

joyeecheung commented Feb 12, 2019 •

edited

Loading

bcoe commented Feb 12, 2019

addaleax commented Feb 15, 2019

addaleax commented Feb 17, 2019

bcoe commented Feb 18, 2019

joyeecheung commented Feb 18, 2019 •

edited

Loading

joyeecheung commented Feb 18, 2019

process: start coverage collection before bootstrap #26006

process: start coverage collection before bootstrap #26006

Conversation

joyeecheung commented Feb 8, 2019

Checklist

nodejs-github-bot commented Feb 8, 2019

joyeecheung commented Feb 8, 2019

joyeecheung commented Feb 8, 2019

targos Feb 8, 2019

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

bcoe Feb 8, 2019

Choose a reason for hiding this comment

addaleax commented Feb 9, 2019

joyeecheung commented Feb 12, 2019 • edited Loading

bcoe commented Feb 12, 2019

addaleax commented Feb 15, 2019

addaleax commented Feb 17, 2019

bcoe commented Feb 18, 2019

joyeecheung commented Feb 18, 2019 • edited Loading

joyeecheung commented Feb 18, 2019

joyeecheung commented Feb 12, 2019 •

edited

Loading

joyeecheung commented Feb 18, 2019 •

edited

Loading