From 19727d9096bfcf5b07e7a9481732f003f7740a20 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 8 Mar 2019 09:34:46 +0100 Subject: [PATCH 1/2] src: refactor coverage connection - Refactor the C++ class to be resuable for other types of profiles - Move the try-catch block around coverage collection callback to be inside the callback to silence potential JSON or write errors. - Use Function::Call instead of MakeCallback to call the coverage message callback since it does not actually need async hook handling. This way we no longer needs to disable the async hooks when writing the coverage results. - Renames `lib/internal/coverage-gen/with_profiler.js` to `lib/internal/profiler.js` because it is now the only way to generate coverage. --- lib/internal/bootstrap/pre_execution.js | 2 +- lib/internal/process/per_thread.js | 2 +- .../with_profiler.js => profiler.js} | 19 +- node.gyp | 2 +- src/env.h | 2 +- src/inspector/node_inspector.gypi | 2 +- src/inspector_coverage.cc | 168 -------------- src/inspector_profiler.cc | 214 ++++++++++++++++++ src/node.cc | 4 +- src/node_binding.cc | 6 +- src/node_internals.h | 4 +- 11 files changed, 231 insertions(+), 194 deletions(-) rename lib/internal/{coverage-gen/with_profiler.js => profiler.js} (73%) delete mode 100644 src/inspector_coverage.cc create mode 100644 src/inspector_profiler.cc diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 936dc4673c1c2a..79bb5cc29160fa 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -67,7 +67,7 @@ function setupCoverageHooks(dir) { const { writeCoverage, setCoverageDirectory - } = require('internal/coverage-gen/with_profiler'); + } = require('internal/profiler'); setCoverageDirectory(coverageDirectory); process.on('exit', writeCoverage); process.reallyExit = (code) => { diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js index 85772aafd8cee9..a0d6b325960bd6 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -158,7 +158,7 @@ function wrapProcessMethods(binding) { function kill(pid, sig) { var err; if (process.env.NODE_V8_COVERAGE) { - const { writeCoverage } = require('internal/coverage-gen/with_profiler'); + const { writeCoverage } = require('internal/profiler'); writeCoverage(); } diff --git a/lib/internal/coverage-gen/with_profiler.js b/lib/internal/profiler.js similarity index 73% rename from lib/internal/coverage-gen/with_profiler.js rename to lib/internal/profiler.js index 573d2c3712b281..1042f126dd6e25 100644 --- a/lib/internal/coverage-gen/with_profiler.js +++ b/lib/internal/profiler.js @@ -21,23 +21,16 @@ function writeCoverage() { } const target = join(coverageDirectory, filename); - try { - disableAllAsyncHooks(); - internalBinding('coverage').end((msg) => { + internalBinding('profiler').endCoverage((msg) => { + try { const coverageInfo = JSON.parse(msg).result; if (coverageInfo) { writeFileSync(target, JSON.stringify(coverageInfo)); } - }); - } catch (err) { - console.error(err); - } -} - -function disableAllAsyncHooks() { - const { getHookArrays } = require('internal/async_hooks'); - const [hooks_array] = getHookArrays(); - hooks_array.forEach((hook) => { hook.disable(); }); + } catch (err) { + console.error(err); + } + }); } function setCoverageDirectory(dir) { diff --git a/node.gyp b/node.gyp index abae8592f42402..13fa0d01e17cdb 100644 --- a/node.gyp +++ b/node.gyp @@ -99,7 +99,6 @@ 'lib/internal/cluster/worker.js', 'lib/internal/console/constructor.js', 'lib/internal/console/global.js', - 'lib/internal/coverage-gen/with_profiler.js', 'lib/internal/crypto/certificate.js', 'lib/internal/crypto/cipher.js', 'lib/internal/crypto/diffiehellman.js', @@ -168,6 +167,7 @@ 'lib/internal/process/worker_thread_only.js', 'lib/internal/process/report.js', 'lib/internal/process/task_queues.js', + 'lib/internal/profiler.js', 'lib/internal/querystring.js', 'lib/internal/readline.js', 'lib/internal/repl.js', diff --git a/src/env.h b/src/env.h index b269982a0ba84d..e78015cd34ad33 100644 --- a/src/env.h +++ b/src/env.h @@ -468,7 +468,7 @@ struct CompileFnEntry { #define DEBUG_CATEGORY_NAMES(V) \ NODE_ASYNC_PROVIDER_TYPES(V) \ V(INSPECTOR_SERVER) \ - V(COVERAGE) + V(INSPECTOR_PROFILER) enum class DebugCategory { #define V(name) name, diff --git a/src/inspector/node_inspector.gypi b/src/inspector/node_inspector.gypi index a59727f191e95b..5191543ab73450 100644 --- a/src/inspector/node_inspector.gypi +++ b/src/inspector/node_inspector.gypi @@ -45,7 +45,7 @@ '../../src/inspector_io.cc', '../../src/inspector_agent.h', '../../src/inspector_io.h', - '../../src/inspector_coverage.cc', + '../../src/inspector_profiler.cc', '../../src/inspector_js_api.cc', '../../src/inspector_socket.cc', '../../src/inspector_socket.h', diff --git a/src/inspector_coverage.cc b/src/inspector_coverage.cc deleted file mode 100644 index 8cbec586fba718..00000000000000 --- a/src/inspector_coverage.cc +++ /dev/null @@ -1,168 +0,0 @@ -#include "base_object-inl.h" -#include "debug_utils.h" -#include "inspector_agent.h" -#include "node_internals.h" -#include "v8-inspector.h" - -namespace node { -namespace coverage { - -using v8::Context; -using v8::Function; -using v8::FunctionCallbackInfo; -using v8::HandleScope; -using v8::Isolate; -using v8::Local; -using v8::MaybeLocal; -using v8::NewStringType; -using v8::Object; -using v8::ObjectTemplate; -using v8::String; -using v8::Value; - -using v8_inspector::StringBuffer; -using v8_inspector::StringView; - -std::unique_ptr ToProtocolString(Isolate* isolate, - Local value) { - TwoByteValue buffer(isolate, value); - return StringBuffer::create(StringView(*buffer, buffer.length())); -} - -class V8CoverageConnection : public BaseObject { - public: - class V8CoverageSessionDelegate : public inspector::InspectorSessionDelegate { - public: - explicit V8CoverageSessionDelegate(V8CoverageConnection* connection) - : connection_(connection) {} - - void SendMessageToFrontend( - const v8_inspector::StringView& message) override { - Environment* env = connection_->env(); - Local fn = connection_->env()->on_coverage_message_function(); - bool ending = !fn.IsEmpty(); - Debug(env, - DebugCategory::COVERAGE, - "Sending message to frontend, ending = %s\n", - ending ? "true" : "false"); - if (!ending) { - return; - } - Isolate* isolate = env->isolate(); - - HandleScope handle_scope(isolate); - Context::Scope context_scope(env->context()); - MaybeLocal v8string = - String::NewFromTwoByte(isolate, - message.characters16(), - NewStringType::kNormal, - message.length()); - Local args[] = {v8string.ToLocalChecked().As()}; - USE(MakeCallback(isolate, - connection_->object(), - fn, - arraysize(args), - args, - async_context{0, 0})); - } - - private: - V8CoverageConnection* connection_; - }; - - SET_MEMORY_INFO_NAME(V8CoverageConnection) - SET_SELF_SIZE(V8CoverageConnection) - - void MemoryInfo(MemoryTracker* tracker) const override { - tracker->TrackFieldWithSize( - "session", sizeof(*session_), "InspectorSession"); - } - - explicit V8CoverageConnection(Environment* env) - : BaseObject(env, env->coverage_connection()), session_(nullptr) { - inspector::Agent* inspector = env->inspector_agent(); - std::unique_ptr session = inspector->Connect( - std::make_unique(this), false); - session_ = std::move(session); - MakeWeak(); - } - - void Start() { - Debug(this->env(), - DebugCategory::COVERAGE, - "Sending Profiler.startPreciseCoverage\n"); - Isolate* isolate = this->env()->isolate(); - Local enable = FIXED_ONE_BYTE_STRING( - isolate, "{\"id\": 1, \"method\": \"Profiler.enable\"}"); - Local start = FIXED_ONE_BYTE_STRING( - isolate, - "{" - "\"id\": 2," - "\"method\": \"Profiler.startPreciseCoverage\"," - "\"params\": {\"callCount\": true, \"detailed\": true}" - "}"); - session_->Dispatch(ToProtocolString(isolate, enable)->string()); - session_->Dispatch(ToProtocolString(isolate, start)->string()); - } - - void End() { - Debug(this->env(), - DebugCategory::COVERAGE, - "Sending Profiler.takePreciseCoverage\n"); - Isolate* isolate = this->env()->isolate(); - Local end = - FIXED_ONE_BYTE_STRING(isolate, - "{" - "\"id\": 3," - "\"method\": \"Profiler.takePreciseCoverage\"" - "}"); - session_->Dispatch(ToProtocolString(isolate, end)->string()); - } - - friend class V8CoverageSessionDelegate; - - private: - std::unique_ptr session_; -}; - -bool StartCoverageCollection(Environment* env) { - HandleScope scope(env->isolate()); - - Local t = ObjectTemplate::New(env->isolate()); - t->SetInternalFieldCount(1); - Local obj; - if (!t->NewInstance(env->context()).ToLocal(&obj)) { - return false; - } - - obj->SetAlignedPointerInInternalField(0, nullptr); - - CHECK(env->coverage_connection().IsEmpty()); - env->set_coverage_connection(obj); - V8CoverageConnection* connection = new V8CoverageConnection(env); - connection->Start(); - return true; -} - -static void EndCoverageCollection(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - CHECK(args[0]->IsFunction()); - Debug(env, DebugCategory::COVERAGE, "Ending coverage collection\n"); - env->set_on_coverage_message_function(args[0].As()); - V8CoverageConnection* connection = - Unwrap(env->coverage_connection()); - CHECK_NOT_NULL(connection); - connection->End(); -} - -static void Initialize(Local target, - Local unused, - Local context, - void* priv) { - Environment* env = Environment::GetCurrent(context); - env->SetMethod(target, "end", EndCoverageCollection); -} -} // namespace coverage -} // namespace node - -NODE_MODULE_CONTEXT_AWARE_INTERNAL(coverage, node::coverage::Initialize) diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc new file mode 100644 index 00000000000000..942a1d148a2f79 --- /dev/null +++ b/src/inspector_profiler.cc @@ -0,0 +1,214 @@ +#include "base_object-inl.h" +#include "debug_utils.h" +#include "inspector_agent.h" +#include "node_internals.h" +#include "v8-inspector.h" + +namespace node { +namespace profiler { + +using v8::Context; +using v8::EscapableHandleScope; +using v8::Function; +using v8::FunctionCallbackInfo; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::MaybeLocal; +using v8::NewStringType; +using v8::Object; +using v8::ObjectTemplate; +using v8::String; +using v8::Value; + +using v8_inspector::StringBuffer; +using v8_inspector::StringView; + +std::unique_ptr ToProtocolString(Isolate* isolate, + Local value) { + TwoByteValue buffer(isolate, value); + return StringBuffer::create(StringView(*buffer, buffer.length())); +} + +class V8ProfilerConnection : public BaseObject { + public: + class V8ProfilerSessionDelegate + : public inspector::InspectorSessionDelegate { + public: + explicit V8ProfilerSessionDelegate(V8ProfilerConnection* connection) + : connection_(connection) {} + + void SendMessageToFrontend( + const v8_inspector::StringView& message) override { + Environment* env = connection_->env(); + + Local fn = connection_->GetMessageCallback(); + bool ending = !fn.IsEmpty(); + Debug(env, + DebugCategory::INSPECTOR_PROFILER, + "Sending message to frontend, ending = %s\n", + ending ? "true" : "false"); + if (!ending) { + return; + } + Isolate* isolate = env->isolate(); + + HandleScope handle_scope(isolate); + Context::Scope context_scope(env->context()); + MaybeLocal v8string = + String::NewFromTwoByte(isolate, + message.characters16(), + NewStringType::kNormal, + message.length()); + Local args[] = {v8string.ToLocalChecked().As()}; + USE(fn->Call( + env->context(), connection_->object(), arraysize(args), args)); + } + + private: + V8ProfilerConnection* connection_; + }; + + SET_MEMORY_INFO_NAME(V8ProfilerConnection) + SET_SELF_SIZE(V8ProfilerConnection) + + void MemoryInfo(MemoryTracker* tracker) const override { + tracker->TrackFieldWithSize( + "session", sizeof(*session_), "InspectorSession"); + } + + explicit V8ProfilerConnection(Environment* env, Local obj) + : BaseObject(env, obj), session_(nullptr) { + inspector::Agent* inspector = env->inspector_agent(); + std::unique_ptr session = inspector->Connect( + std::make_unique(this), false); + session_ = std::move(session); + MakeWeak(); + } + + void DispatchMessage(Isolate* isolate, Local message) { + session_->Dispatch(ToProtocolString(isolate, message)->string()); + } + + static MaybeLocal CreateConnectionObject(Environment* env) { + Isolate* isolate = env->isolate(); + Local context = env->context(); + EscapableHandleScope scope(isolate); + + Local t = ObjectTemplate::New(isolate); + t->SetInternalFieldCount(1); + Local obj; + if (!t->NewInstance(context).ToLocal(&obj)) { + return MaybeLocal(); + } + + obj->SetAlignedPointerInInternalField(0, nullptr); + return scope.Escape(obj); + } + + void Start() { + SetConnection(object()); + StartProfiling(); + } + + void End(Local callback) { + SetMessageCallback(callback); + EndProfiling(); + } + + // Override this to return a JS function that gets called with the message + // sent from the session. + virtual Local GetMessageCallback() = 0; + virtual void SetMessageCallback(Local callback) = 0; + // Use DispatchMessage() to dispatch necessary inspector messages + virtual void StartProfiling() = 0; + virtual void EndProfiling() = 0; + virtual void SetConnection(Local connection) = 0; + + private: + std::unique_ptr session_; +}; + +class V8CoverageConnection : public V8ProfilerConnection { + public: + explicit V8CoverageConnection(Environment* env) + : V8ProfilerConnection(env, + CreateConnectionObject(env).ToLocalChecked()) {} + + Local GetMessageCallback() override { + return env()->on_coverage_message_function(); + } + + void SetMessageCallback(Local callback) override { + return env()->set_on_coverage_message_function(callback); + } + + static V8ProfilerConnection* GetConnection(Environment* env) { + return Unwrap(env->coverage_connection()); + } + + void SetConnection(Local obj) override { + env()->set_coverage_connection(obj); + } + + void StartProfiling() override { + Debug(env(), + DebugCategory::INSPECTOR_PROFILER, + "Sending Profiler.startPreciseCoverage\n"); + Isolate* isolate = env()->isolate(); + Local enable = FIXED_ONE_BYTE_STRING( + isolate, "{\"id\": 1, \"method\": \"Profiler.enable\"}"); + Local start = FIXED_ONE_BYTE_STRING( + isolate, + "{" + "\"id\": 2," + "\"method\": \"Profiler.startPreciseCoverage\"," + "\"params\": {\"callCount\": true, \"detailed\": true}" + "}"); + DispatchMessage(isolate, enable); + DispatchMessage(isolate, start); + } + + void EndProfiling() override { + Debug(env(), + DebugCategory::INSPECTOR_PROFILER, + "Sending Profiler.takePreciseCoverage\n"); + Isolate* isolate = env()->isolate(); + Local end = + FIXED_ONE_BYTE_STRING(isolate, + "{" + "\"id\": 3," + "\"method\": \"Profiler.takePreciseCoverage\"" + "}"); + DispatchMessage(isolate, end); + } + + private: + std::unique_ptr session_; +}; + +void StartCoverageCollection(Environment* env) { + V8CoverageConnection* connection = new V8CoverageConnection(env); + connection->Start(); +} + +static void EndCoverageCollection(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + CHECK(args[0]->IsFunction()); + Debug(env, DebugCategory::INSPECTOR_PROFILER, "Ending coverage collection\n"); + V8ProfilerConnection* connection = V8CoverageConnection::GetConnection(env); + CHECK_NOT_NULL(connection); + connection->End(args[0].As()); +} + +static void Initialize(Local target, + Local unused, + Local context, + void* priv) { + Environment* env = Environment::GetCurrent(context); + env->SetMethod(target, "endCoverage", EndCoverageCollection); +} +} // namespace profiler +} // namespace node + +NODE_MODULE_CONTEXT_AWARE_INTERNAL(profiler, node::profiler::Initialize) diff --git a/src/node.cc b/src/node.cc index 94947cb34a3132..d9ca771804f7f4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -235,9 +235,7 @@ MaybeLocal RunBootstrapping(Environment* env) { bool rc = credentials::SafeGetenv("NODE_V8_COVERAGE", &coverage); if (rc && !coverage.empty()) { #if HAVE_INSPECTOR - if (!coverage::StartCoverageCollection(env)) { - return MaybeLocal(); - } + profiler::StartCoverageCollection(env); #else fprintf(stderr, "NODE_V8_COVERAGE cannot be used without inspector"); #endif // HAVE_INSPECTOR diff --git a/src/node_binding.cc b/src/node_binding.cc index 7b87ba6bffd2e7..9599cf956b9b7c 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -23,9 +23,9 @@ #endif #if HAVE_INSPECTOR -#define NODE_BUILTIN_COVERAGE_MODULES(V) V(coverage) +#define NODE_BUILTIN_PROFILER_MODULES(V) V(profiler) #else -#define NODE_BUILTIN_COVERAGE_MODULES(V) +#define NODE_BUILTIN_PROFILER_MODULES(V) #endif // A list of built-in modules. In order to do module registration @@ -85,7 +85,7 @@ NODE_BUILTIN_OPENSSL_MODULES(V) \ NODE_BUILTIN_ICU_MODULES(V) \ NODE_BUILTIN_REPORT_MODULES(V) \ - NODE_BUILTIN_COVERAGE_MODULES(V) + NODE_BUILTIN_PROFILER_MODULES(V) // This is used to load built-in modules. Instead of using // __attribute__((constructor)), we call the _register_ diff --git a/src/node_internals.h b/src/node_internals.h index 212da7af28107c..2e492727bbc5fc 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -294,8 +294,8 @@ void DefineZlibConstants(v8::Local target); v8::MaybeLocal RunBootstrapping(Environment* env); v8::MaybeLocal StartExecution(Environment* env, const char* main_script_id); -namespace coverage { -bool StartCoverageCollection(Environment* env); +namespace profiler { +void StartCoverageCollection(Environment* env); } } // namespace node From a7099875a405b6b11f62ebfaf2f59f09133232c7 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 8 Mar 2019 11:53:15 +0100 Subject: [PATCH 2/2] test: show stderr on v8 coverage test failures --- test/parallel/test-v8-coverage.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/test/parallel/test-v8-coverage.js b/test/parallel/test-v8-coverage.js index 5c3db7af2cfea3..f002604c48e594 100644 --- a/test/parallel/test-v8-coverage.js +++ b/test/parallel/test-v8-coverage.js @@ -22,6 +22,9 @@ function nextdir() { const output = spawnSync(process.execPath, [ require.resolve('../fixtures/v8-coverage/basic') ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + if (output.status !== 0) { + console.log(output.stderr.toString()); + } assert.strictEqual(output.status, 0); assert.strictEqual(output.stderr.toString(), ''); const fixtureCoverage = getFixtureCoverage('basic.js', coverageDirectory); @@ -38,6 +41,9 @@ function nextdir() { const output = spawnSync(process.execPath, [ require.resolve('../fixtures/v8-coverage/exit-1') ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + if (output.status !== 1) { + console.log(output.stderr.toString()); + } assert.strictEqual(output.status, 1); assert.strictEqual(output.stderr.toString(), ''); const fixtureCoverage = getFixtureCoverage('exit-1.js', coverageDirectory); @@ -55,6 +61,9 @@ function nextdir() { require.resolve('../fixtures/v8-coverage/sigint') ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); if (!common.isWindows) { + if (output.signal !== 'SIGINT') { + console.log(output.stderr.toString()); + } assert.strictEqual(output.signal, 'SIGINT'); } assert.strictEqual(output.stderr.toString(), ''); @@ -72,6 +81,9 @@ function nextdir() { const output = spawnSync(process.execPath, [ require.resolve('../fixtures/v8-coverage/spawn-subprocess') ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + if (output.status !== 0) { + console.log(output.stderr.toString()); + } assert.strictEqual(output.status, 0); assert.strictEqual(output.stderr.toString(), ''); const fixtureCoverage = getFixtureCoverage('subprocess.js', @@ -89,6 +101,9 @@ function nextdir() { const output = spawnSync(process.execPath, [ require.resolve('../fixtures/v8-coverage/worker') ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + if (output.status !== 0) { + console.log(output.stderr.toString()); + } assert.strictEqual(output.status, 0); assert.strictEqual(output.stderr.toString(), ''); const fixtureCoverage = getFixtureCoverage('subprocess.js', @@ -106,6 +121,9 @@ function nextdir() { const output = spawnSync(process.execPath, [ require.resolve('../fixtures/v8-coverage/spawn-subprocess-no-cov') ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + if (output.status !== 0) { + console.log(output.stderr.toString()); + } assert.strictEqual(output.status, 0); assert.strictEqual(output.stderr.toString(), ''); const fixtureCoverage = getFixtureCoverage('subprocess.js', @@ -119,6 +137,9 @@ function nextdir() { const output = spawnSync(process.execPath, [ require.resolve('../fixtures/v8-coverage/async-hooks') ], { env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + if (output.status !== 0) { + console.log(output.stderr.toString()); + } assert.strictEqual(output.status, 0); assert.strictEqual(output.stderr.toString(), ''); const fixtureCoverage = getFixtureCoverage('async-hooks.js', @@ -138,6 +159,9 @@ function nextdir() { cwd: tmpdir.path, env: { ...process.env, NODE_V8_COVERAGE: coverageDirectory } }); + if (output.status !== 0) { + console.log(output.stderr.toString()); + } assert.strictEqual(output.status, 0); assert.strictEqual(output.stderr.toString(), ''); const fixtureCoverage = getFixtureCoverage('basic.js',