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.

Sign up for GitHub

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

lib,src: remove cpu profiler idle notifier #34010

Closed
wants to merge 2 commits into from
Closed

lib,src: remove cpu profiler idle notifier #34010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61468b8
lib,src: remove cpu profiler idle notifier
bnoordhuis Jun 22, 2020
49951ae
squash! add comment
bnoordhuis Jun 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/internal/bootstrap/switches/is_main_thread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@ 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;

// 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 = () => {};
bnoordhuis marked this conversation as resolved.
Show resolved Hide resolved
process._stopProfilerIdleNotifier = () => {};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax @BridgeAR I turned them into no-ops like #33138 does. I feel less strongly than Anna about emitting warnings but I also don't really see the point because the functionality has been broken-ish for a long time. Doing nothing is more correct than doing the wrong thing, IMO. :-)


function defineStream(name, getter) {
ObjectDefineProperty(process, name, {
Expand Down
2 changes: 0 additions & 2 deletions lib/internal/bootstrap/switches/is_not_main_thread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down
2 changes: 1 addition & 1 deletion src/api/environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ MaybeLocal<Value> LoadEnvironment(
Environment* env,
StartExecutionCallback cb,
std::unique_ptr<InspectorParentHandle> removeme) {
env->InitializeLibuv(per_process::v8_is_profiling);
env->InitializeLibuv();
env->InitializeDiagnostics();

return StartExecution(env, cb);
Expand Down
4 changes: 0 additions & 4 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,10 +378,6 @@ inline Environment* Environment::GetThreadLocalEnv() {
return static_cast<Environment*>(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_;
}
Expand Down
44 changes: 1 addition & 43 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand All @@ -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_,
Expand All @@ -518,8 +507,6 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
&Environment::task_queues_async_, async);
env->RunAndClearNativeImmediates();
});
uv_unref(reinterpret_cast<uv_handle_t*>(&idle_prepare_handle_));
uv_unref(reinterpret_cast<uv_handle_t*>(&idle_check_handle_));
uv_unref(reinterpret_cast<uv_handle_t*>(&task_queues_async_));

{
Expand All @@ -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() {
Expand Down Expand Up @@ -567,8 +550,6 @@ void Environment::RegisterHandleCleanups() {
register_handle(reinterpret_cast<uv_handle_t*>(timer_handle()));
register_handle(reinterpret_cast<uv_handle_t*>(immediate_check_handle()));
register_handle(reinterpret_cast<uv_handle_t*>(immediate_idle_handle()));
register_handle(reinterpret_cast<uv_handle_t*>(&idle_prepare_handle_));
register_handle(reinterpret_cast<uv_handle_t*>(&idle_check_handle_));
register_handle(reinterpret_cast<uv_handle_t*>(&task_queues_async_));
}

Expand Down Expand Up @@ -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;

Expand Down
9 changes: 1 addition & 8 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string>& exec_argv();
inline const std::vector<std::string>& argv();
const std::string& exec_path() const;
Expand Down Expand Up @@ -951,10 +951,6 @@ class Environment : public MemoryRetainer {
inline void AssignToContext(v8::Local<v8::Context> 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(
Expand Down Expand Up @@ -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_;
Expand Down
12 changes: 1 addition & 11 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -786,19 +784,11 @@ int ProcessGlobalArgs(std::vector<std::string>* 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
Expand Down
1 change: 0 additions & 1 deletion src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 0 additions & 14 deletions src/node_process_methods.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,16 +196,6 @@ void RawDebug(const FunctionCallbackInfo<Value>& args) {
fflush(stderr);
}

static void StartProfilerIdleNotifier(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
env->StartProfilerIdleNotifier();
}

static void StopProfilerIdleNotifier(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
env->StopProfilerIdleNotifier();
}

static void Umask(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK(env->has_run_bootstrapping_code());
Expand Down Expand Up @@ -529,10 +519,6 @@ static void InitializeProcessMethods(Local<Object> 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);
Expand Down