From 61468b8ea6f41948ae191324e010fd0558a7f3ce Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 22 Jun 2020 11:35:23 +0200 Subject: [PATCH 1/2] lib,src: remove cpu profiler idle notifier I added it in commit 57231d5286 ("src: notify V8 profiler when we're idle") from October 2013 as a stop-gap measure to measure CPU time rather than wall clock time, otherwise processes that spend a lot of time sleeping in system calls give a false impression of being very busy. That fix is not without drawbacks because the idle flag is set before libuv makes I/O callbacks and cleared again after. I/O callbacks can result into calls into JS code and executing JS code is as non-idle as you can get. In commit 96ffcb9a21 ("src: reduce cpu profiler overhead") from January 2015, I made Node.js block off the SIGPROF signal that V8's CPU profiler uses before Node.js goes to sleep. The goal of that commit is to reduce the overhead from EINTR system call wakeups but it also has the pleasant side effect of fixing what the idle notifier tried to fix. This commit removes the idle notifier and turns the JS process object methods into no-ops. Fixes: https://github.com/nodejs/node/issues/19009 Refs: https://github.com/nodejs/node/pull/33138 --- .../bootstrap/switches/is_main_thread.js | 4 +- .../bootstrap/switches/is_not_main_thread.js | 2 - src/api/environment.cc | 2 +- src/env-inl.h | 4 -- src/env.cc | 44 +------------------ src/env.h | 9 +--- src/node.cc | 12 +---- src/node_internals.h | 1 - src/node_process_methods.cc | 14 ------ 9 files changed, 6 insertions(+), 86 deletions(-) diff --git a/lib/internal/bootstrap/switches/is_main_thread.js b/lib/internal/bootstrap/switches/is_main_thread.js index be145e84dc27df..6c28d1e0c7c681 100644 --- a/lib/internal/bootstrap/switches/is_main_thread.js +++ b/lib/internal/bootstrap/switches/is_main_thread.js @@ -6,8 +6,8 @@ const rawMethods = internalBinding('process_methods'); // TODO(joyeecheung): deprecate and remove these underscore methods process._debugProcess = rawMethods._debugProcess; process._debugEnd = rawMethods._debugEnd; -process._startProfilerIdleNotifier = rawMethods._startProfilerIdleNotifier; -process._stopProfilerIdleNotifier = rawMethods._stopProfilerIdleNotifier; +process._startProfilerIdleNotifier = () => {}; +process._stopProfilerIdleNotifier = () => {}; function defineStream(name, getter) { ObjectDefineProperty(process, name, { diff --git a/lib/internal/bootstrap/switches/is_not_main_thread.js b/lib/internal/bootstrap/switches/is_not_main_thread.js index 852352eead0d73..379ad0a587a22a 100644 --- a/lib/internal/bootstrap/switches/is_not_main_thread.js +++ b/lib/internal/bootstrap/switches/is_not_main_thread.js @@ -4,8 +4,6 @@ const { ObjectDefineProperty } = primordials; delete process._debugProcess; delete process._debugEnd; -delete process._startProfilerIdleNotifier; -delete process._stopProfilerIdleNotifier; function defineStream(name, getter) { ObjectDefineProperty(process, name, { diff --git a/src/api/environment.cc b/src/api/environment.cc index a574b916fe6f00..6d0411c2c3df4d 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -422,7 +422,7 @@ MaybeLocal LoadEnvironment( Environment* env, StartExecutionCallback cb, std::unique_ptr removeme) { - env->InitializeLibuv(per_process::v8_is_profiling); + env->InitializeLibuv(); env->InitializeDiagnostics(); return StartExecution(env, cb); diff --git a/src/env-inl.h b/src/env-inl.h index fe92fcb3dd91cd..5cbb2cd60bba99 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -378,10 +378,6 @@ inline Environment* Environment::GetThreadLocalEnv() { return static_cast(uv_key_get(&thread_local_env)); } -inline bool Environment::profiler_idle_notifier_started() const { - return profiler_idle_notifier_started_; -} - inline v8::Isolate* Environment::isolate() const { return isolate_; } diff --git a/src/env.cc b/src/env.cc index 4030df9ec90f57..991ba3cf98d4a2 100644 --- a/src/env.cc +++ b/src/env.cc @@ -485,7 +485,7 @@ Environment::~Environment() { CHECK_EQ(base_object_count_, 0); } -void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { +void Environment::InitializeLibuv() { HandleScope handle_scope(isolate()); Context::Scope context_scope(context()); @@ -499,17 +499,6 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { uv_check_start(immediate_check_handle(), CheckImmediate); - // Inform V8's CPU profiler when we're idle. The profiler is sampling-based - // but not all samples are created equal; mark the wall clock time spent in - // epoll_wait() and friends so profiling tools can filter it out. The samples - // still end up in v8.log but with state=IDLE rather than state=EXTERNAL. - // TODO(bnoordhuis) Depends on a libuv implementation detail that we should - // probably fortify in the API contract, namely that the last started prepare - // or check watcher runs first. It's not 100% foolproof; if an add-on starts - // a prepare or check watcher after us, any samples attributed to its callback - // will be recorded with state=IDLE. - uv_prepare_init(event_loop(), &idle_prepare_handle_); - uv_check_init(event_loop(), &idle_check_handle_); uv_async_init( event_loop(), &task_queues_async_, @@ -518,8 +507,6 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { &Environment::task_queues_async_, async); env->RunAndClearNativeImmediates(); }); - uv_unref(reinterpret_cast(&idle_prepare_handle_)); - uv_unref(reinterpret_cast(&idle_check_handle_)); uv_unref(reinterpret_cast(&task_queues_async_)); { @@ -536,10 +523,6 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { // the one environment per process setup, but will be called in // FreeEnvironment. RegisterHandleCleanups(); - - if (start_profiler_idle_notifier) { - StartProfilerIdleNotifier(); - } } void Environment::ExitEnv() { @@ -567,8 +550,6 @@ void Environment::RegisterHandleCleanups() { register_handle(reinterpret_cast(timer_handle())); register_handle(reinterpret_cast(immediate_check_handle())); register_handle(reinterpret_cast(immediate_idle_handle())); - register_handle(reinterpret_cast(&idle_prepare_handle_)); - register_handle(reinterpret_cast(&idle_check_handle_)); register_handle(reinterpret_cast(&task_queues_async_)); } @@ -600,29 +581,6 @@ void Environment::CleanupHandles() { } } -void Environment::StartProfilerIdleNotifier() { - if (profiler_idle_notifier_started_) - return; - - profiler_idle_notifier_started_ = true; - - uv_prepare_start(&idle_prepare_handle_, [](uv_prepare_t* handle) { - Environment* env = ContainerOf(&Environment::idle_prepare_handle_, handle); - env->isolate()->SetIdle(true); - }); - - uv_check_start(&idle_check_handle_, [](uv_check_t* handle) { - Environment* env = ContainerOf(&Environment::idle_check_handle_, handle); - env->isolate()->SetIdle(false); - }); -} - -void Environment::StopProfilerIdleNotifier() { - profiler_idle_notifier_started_ = false; - uv_prepare_stop(&idle_prepare_handle_); - uv_check_stop(&idle_check_handle_); -} - void Environment::PrintSyncTrace() const { if (!trace_sync_io_) return; diff --git a/src/env.h b/src/env.h index ba3306a43347b8..763ca746b6b720 100644 --- a/src/env.h +++ b/src/env.h @@ -921,7 +921,7 @@ class Environment : public MemoryRetainer { ThreadId thread_id); ~Environment() override; - void InitializeLibuv(bool start_profiler_idle_notifier); + void InitializeLibuv(); inline const std::vector& exec_argv(); inline const std::vector& argv(); const std::string& exec_path() const; @@ -951,10 +951,6 @@ class Environment : public MemoryRetainer { inline void AssignToContext(v8::Local context, const ContextInfo& info); - void StartProfilerIdleNotifier(); - void StopProfilerIdleNotifier(); - inline bool profiler_idle_notifier_started() const; - inline v8::Isolate* isolate() const; inline uv_loop_t* event_loop() const; inline void TryLoadAddon( @@ -1281,11 +1277,8 @@ class Environment : public MemoryRetainer { uv_timer_t timer_handle_; uv_check_t immediate_check_handle_; uv_idle_t immediate_idle_handle_; - uv_prepare_t idle_prepare_handle_; - uv_check_t idle_check_handle_; uv_async_t task_queues_async_; int64_t task_queues_async_refs_ = 0; - bool profiler_idle_notifier_started_ = false; AsyncHooks async_hooks_; ImmediateInfo immediate_info_; diff --git a/src/node.cc b/src/node.cc index b7435d67f839ea..405e906092af9f 100644 --- a/src/node.cc +++ b/src/node.cc @@ -148,8 +148,6 @@ bool v8_initialized = false; // node_internals.h // process-relative uptime base in nanoseconds, initialized in node::Start() uint64_t node_start_time; -// Tells whether --prof is passed. -bool v8_is_profiling = false; // node_v8_platform-inl.h struct V8Platform v8_platform; @@ -786,19 +784,11 @@ int ProcessGlobalArgs(std::vector* args, env_opts->abort_on_uncaught_exception = true; } - // TODO(bnoordhuis) Intercept --prof arguments and start the CPU profiler - // manually? That would give us a little more control over its runtime - // behavior but it could also interfere with the user's intentions in ways - // we fail to anticipate. Dillema. - if (std::find(v8_args.begin(), v8_args.end(), "--prof") != v8_args.end()) { - per_process::v8_is_profiling = true; - } - #ifdef __POSIX__ // Block SIGPROF signals when sleeping in epoll_wait/kevent/etc. Avoids the // performance penalty of frequent EINTR wakeups when the profiler is running. // Only do this for v8.log profiling, as it breaks v8::CpuProfiler users. - if (per_process::v8_is_profiling) { + if (std::find(v8_args.begin(), v8_args.end(), "--prof") != v8_args.end()) { uv_loop_configure(uv_default_loop(), UV_LOOP_BLOCK_SIGNAL, SIGPROF); } #endif diff --git a/src/node_internals.h b/src/node_internals.h index 65a4edcbea6b81..dffaa084db409b 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -55,7 +55,6 @@ class NativeModuleLoader; namespace per_process { extern Mutex env_var_mutex; extern uint64_t node_start_time; -extern bool v8_is_profiling; } // namespace per_process // Forward declaration diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index 88d5f843f3072f..aa186185988a0a 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -196,16 +196,6 @@ void RawDebug(const FunctionCallbackInfo& args) { fflush(stderr); } -static void StartProfilerIdleNotifier(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - env->StartProfilerIdleNotifier(); -} - -static void StopProfilerIdleNotifier(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - env->StopProfilerIdleNotifier(); -} - static void Umask(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK(env->has_run_bootstrapping_code()); @@ -529,10 +519,6 @@ static void InitializeProcessMethods(Local target, env->SetMethod(target, "chdir", Chdir); } - env->SetMethod( - target, "_startProfilerIdleNotifier", StartProfilerIdleNotifier); - env->SetMethod(target, "_stopProfilerIdleNotifier", StopProfilerIdleNotifier); - env->SetMethod(target, "umask", Umask); env->SetMethod(target, "_rawDebug", RawDebug); env->SetMethod(target, "memoryUsage", MemoryUsage); From 49951aefcbd9d958e4095843832e2dfbda7e7ad5 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 23 Jun 2020 19:43:21 +0200 Subject: [PATCH 2/2] squash! add comment --- lib/internal/bootstrap/switches/is_main_thread.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/internal/bootstrap/switches/is_main_thread.js b/lib/internal/bootstrap/switches/is_main_thread.js index 6c28d1e0c7c681..08623898edafac 100644 --- a/lib/internal/bootstrap/switches/is_main_thread.js +++ b/lib/internal/bootstrap/switches/is_main_thread.js @@ -6,6 +6,10 @@ const rawMethods = internalBinding('process_methods'); // TODO(joyeecheung): deprecate and remove these underscore methods process._debugProcess = rawMethods._debugProcess; process._debugEnd = rawMethods._debugEnd; + +// See the discussion in https://github.com/nodejs/node/issues/19009 and +// https://github.com/nodejs/node/pull/34010 for why these are no-ops. +// Five word summary: they were broken beyond repair. process._startProfilerIdleNotifier = () => {}; process._stopProfilerIdleNotifier = () => {};