From 1cf085235a1d0c36ea54c712a885143d1b7ddb95 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 11 Jun 2020 23:36:19 +0200 Subject: [PATCH] src: add public APIs to manage v8::TracingController MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We added a hack for this a while ago for Electron, so let’s remove that hack and make this an official API. Refs: https://github.com/nodejs/node/pull/28724 Refs: https://github.com/nodejs/node/issues/33800 PR-URL: https://github.com/nodejs/node/pull/33850 Reviewed-By: Michaël Zasso Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node.h | 9 +++++++++ src/node_platform.cc | 3 ++- src/node_platform.h | 1 - src/node_v8_platform-inl.h | 1 + src/tracing/trace_event.cc | 10 ++++++++++ src/tracing/trace_event.h | 6 ++---- test/cctest/test_platform.cc | 19 +++++++++++++++++++ 7 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/node.h b/src/node.h index 610d72ba8bf6ba..345c9d0f3ebf1b 100644 --- a/src/node.h +++ b/src/node.h @@ -510,6 +510,15 @@ NODE_EXTERN MultiIsolatePlatform* CreatePlatform( v8::TracingController* tracing_controller); NODE_EXTERN void FreePlatform(MultiIsolatePlatform* platform); +// Get/set the currently active tracing controller. Using CreatePlatform() +// will implicitly set this by default. This is global and should be initialized +// along with the v8::Platform instance that is being used. `controller` +// is allowed to be `nullptr`. +// This is used for tracing events from Node.js itself. V8 uses the tracing +// controller returned from the active `v8::Platform` instance. +NODE_EXTERN v8::TracingController* GetTracingController(); +NODE_EXTERN void SetTracingController(v8::TracingController* controller); + NODE_EXTERN void EmitBeforeExit(Environment* env); NODE_EXTERN int EmitExit(Environment* env); NODE_EXTERN void RunAtExit(Environment* env); diff --git a/src/node_platform.cc b/src/node_platform.cc index 9ca15c13e70155..4bb9b919f60a91 100644 --- a/src/node_platform.cc +++ b/src/node_platform.cc @@ -333,7 +333,8 @@ NodePlatform::NodePlatform(int thread_pool_size, // TODO(addaleax): It's a bit icky that we use global state here, but we can't // really do anything about it unless V8 starts exposing a way to access the // current v8::Platform instance. - tracing::TraceEventHelper::SetTracingController(tracing_controller_); + SetTracingController(tracing_controller_); + DCHECK_EQ(GetTracingController(), tracing_controller_); worker_thread_task_runner_ = std::make_shared(thread_pool_size); } diff --git a/src/node_platform.h b/src/node_platform.h index e428c7e716765d..dc512ddf08facf 100644 --- a/src/node_platform.h +++ b/src/node_platform.h @@ -11,7 +11,6 @@ #include "libplatform/libplatform.h" #include "node.h" #include "node_mutex.h" -#include "tracing/agent.h" #include "uv.h" namespace node { diff --git a/src/node_v8_platform-inl.h b/src/node_v8_platform-inl.h index ce6d5dd223d2cb..d79dd99bf34da6 100644 --- a/src/node_v8_platform-inl.h +++ b/src/node_v8_platform-inl.h @@ -8,6 +8,7 @@ #include "env-inl.h" #include "node.h" #include "node_metadata.h" +#include "node_platform.h" #include "node_options.h" #include "tracing/node_trace_writer.h" #include "tracing/trace_event.h" diff --git a/src/tracing/trace_event.cc b/src/tracing/trace_event.cc index da562c1bf7f8ff..3ffe6f8867cae0 100644 --- a/src/tracing/trace_event.cc +++ b/src/tracing/trace_event.cc @@ -1,4 +1,5 @@ #include "tracing/trace_event.h" +#include "node.h" namespace node { namespace tracing { @@ -24,4 +25,13 @@ void TraceEventHelper::SetTracingController(v8::TracingController* controller) { } } // namespace tracing + +v8::TracingController* GetTracingController() { + return tracing::TraceEventHelper::GetTracingController(); +} + +void SetTracingController(v8::TracingController* controller) { + tracing::TraceEventHelper::SetTracingController(controller); +} + } // namespace node diff --git a/src/tracing/trace_event.h b/src/tracing/trace_event.h index 2a79c5bc05b501..be0f55a409a71b 100644 --- a/src/tracing/trace_event.h +++ b/src/tracing/trace_event.h @@ -5,8 +5,8 @@ #ifndef SRC_TRACING_TRACE_EVENT_H_ #define SRC_TRACING_TRACE_EVENT_H_ -#include "node_platform.h" #include "v8-platform.h" +#include "tracing/agent.h" #include "trace_event_common.h" #include @@ -310,9 +310,7 @@ const int kZeroNumArgs = 0; const decltype(nullptr) kGlobalScope = nullptr; const uint64_t kNoId = 0; -// Extern (for now) because embedders need access to TraceEventHelper. -// Refs: https://github.com/nodejs/node/pull/28724 -class NODE_EXTERN TraceEventHelper { +class TraceEventHelper { public: static v8::TracingController* GetTracingController(); static void SetTracingController(v8::TracingController* controller); diff --git a/test/cctest/test_platform.cc b/test/cctest/test_platform.cc index c56766b5ec8611..07bea57b314bf0 100644 --- a/test/cctest/test_platform.cc +++ b/test/cctest/test_platform.cc @@ -104,3 +104,22 @@ TEST_F(NodeZeroIsolateTestFixture, IsolatePlatformDelegateTest) { platform->UnregisterIsolate(isolate); isolate->Dispose(); } + +TEST_F(PlatformTest, TracingControllerNullptr) { + v8::TracingController* orig_controller = node::GetTracingController(); + node::SetTracingController(nullptr); + EXPECT_EQ(node::GetTracingController(), nullptr); + + v8::Isolate::Scope isolate_scope(isolate_); + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env env {handle_scope, argv}; + + node::LoadEnvironment(*env, [&](const node::StartExecutionCallbackInfo& info) + -> v8::MaybeLocal { + return v8::Null(isolate_); + }); + + node::SetTracingController(orig_controller); + EXPECT_EQ(node::GetTracingController(), orig_controller); +}