From aac79dfd78a20ac66d99a55f24ddf91f937a2388 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 1 Jun 2016 11:18:02 +0200 Subject: [PATCH] src: use stack-allocated Environment instances Makes it easier to reason about the lifetime of the Environment object. PR-URL: https://github.com/nodejs/node/pull/7090 Refs: https://github.com/nodejs/node/pull/7082 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- src/debug-agent.cc | 27 +++++++++------------- src/env-inl.h | 34 +++++++++------------------- src/env.h | 14 ++++-------- src/node.cc | 56 +++++++++++++++++++++------------------------- 4 files changed, 51 insertions(+), 80 deletions(-) diff --git a/src/debug-agent.cc b/src/debug-agent.cc index e5dc346dc6a354..3eb9f37a60c0b8 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -174,31 +174,24 @@ void Agent::WorkerRun() { Local context = Context::New(isolate); Context::Scope context_scope(context); - Environment* env = CreateEnvironment( - &isolate_data, - context, - arraysize(argv), - argv, - arraysize(argv), - argv); + Environment env(&isolate_data, context); - child_env_ = env; + const bool start_profiler_idle_notifier = false; + env.Start(arraysize(argv), argv, + arraysize(argv), argv, + start_profiler_idle_notifier); + + child_env_ = &env; // Expose API - InitAdaptor(env); - LoadEnvironment(env); + InitAdaptor(&env); + LoadEnvironment(&env); - CHECK_EQ(&child_loop_, env->event_loop()); + CHECK_EQ(&child_loop_, env.event_loop()); uv_run(&child_loop_, UV_RUN_DEFAULT); // Clean-up peristent api_.Reset(); - - // Clean-up all running handles - env->CleanupHandles(); - - env->Dispose(); - env = nullptr; } isolate->Dispose(); } diff --git a/src/env-inl.h b/src/env-inl.h index 97642566763029..d35f689c2f77b9 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -134,13 +134,6 @@ inline void Environment::TickInfo::set_index(uint32_t value) { fields_[kIndex] = value; } -inline Environment* Environment::New(IsolateData* isolate_data, - v8::Local context) { - Environment* env = new Environment(isolate_data, context); - env->AssignToContext(context); - return env; -} - inline void Environment::AssignToContext(v8::Local context) { context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this); } @@ -184,6 +177,7 @@ inline Environment::Environment(IsolateData* isolate_data, #if HAVE_INSPECTOR inspector_agent_(this), #endif + handle_cleanup_waiting_(0), http_parser_buffer_(nullptr), context_(context->GetIsolate(), context) { // We'll be creating new objects so make sure we've entered the context. @@ -200,24 +194,12 @@ inline Environment::Environment(IsolateData* isolate_data, set_generic_internal_field_template(obj); RB_INIT(&cares_task_list_); - handle_cleanup_waiting_ = 0; + AssignToContext(context); } inline Environment::~Environment() { v8::HandleScope handle_scope(isolate()); - context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, - nullptr); -#define V(PropertyName, TypeName) PropertyName ## _.Reset(); - ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) -#undef V - - delete[] heap_statistics_buffer_; - delete[] heap_space_statistics_buffer_; - delete[] http_parser_buffer_; -} - -inline void Environment::CleanupHandles() { while (HandleCleanup* hc = handle_cleanup_queue_.PopFront()) { handle_cleanup_waiting_++; hc->cb_(this, hc->handle_, hc->arg_); @@ -226,10 +208,16 @@ inline void Environment::CleanupHandles() { while (handle_cleanup_waiting_ != 0) uv_run(event_loop(), UV_RUN_ONCE); -} -inline void Environment::Dispose() { - delete this; + context()->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, + nullptr); +#define V(PropertyName, TypeName) PropertyName ## _.Reset(); + ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) +#undef V + + delete[] heap_statistics_buffer_; + delete[] heap_space_statistics_buffer_; + delete[] http_parser_buffer_; } inline v8::Isolate* Environment::isolate() const { diff --git a/src/env.h b/src/env.h index afbc505ab8caf4..0188a6d3bed8b9 100644 --- a/src/env.h +++ b/src/env.h @@ -446,17 +446,14 @@ class Environment { static inline Environment* GetCurrent( const v8::PropertyCallbackInfo& info); - // See CreateEnvironment() in src/node.cc. - static inline Environment* New(IsolateData* isolate_data, - v8::Local context); + inline Environment(IsolateData* isolate_data, v8::Local context); + inline ~Environment(); + void Start(int argc, const char* const* argv, int exec_argc, const char* const* exec_argv, bool start_profiler_idle_notifier); - inline void CleanupHandles(); - inline void Dispose(); - void AssignToContext(v8::Local context); void StartProfilerIdleNotifier(); @@ -472,7 +469,7 @@ class Environment { inline uv_check_t* immediate_check_handle(); inline uv_idle_t* immediate_idle_handle(); - // Register clean-up cb to be called on env->Dispose() + // Register clean-up cb to be called on environment destruction. inline void RegisterHandleCleanup(uv_handle_t* handle, HandleCleanupCb cb, void *arg); @@ -584,9 +581,6 @@ class Environment { static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX; private: - inline Environment(IsolateData* isolate_data, v8::Local context); - inline ~Environment(); - v8::Isolate* const isolate_; IsolateData* const isolate_data_; uv_check_t immediate_check_handle_; diff --git a/src/node.cc b/src/node.cc index 2cbc5bf3fc25f6..5fe52cafca7f04 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3363,12 +3363,6 @@ void LoadEnvironment(Environment* env) { } -void FreeEnvironment(Environment* env) { - CHECK_NE(env, nullptr); - env->Dispose(); -} - - static void PrintHelp(); static bool ParseDebugOpt(const char* arg) { @@ -4256,12 +4250,17 @@ Environment* CreateEnvironment(IsolateData* isolate_data, Isolate* isolate = context->GetIsolate(); HandleScope handle_scope(isolate); Context::Scope context_scope(context); - Environment* env = Environment::New(isolate_data, context); + auto env = new Environment(isolate_data, context); env->Start(argc, argv, exec_argc, exec_argv, v8_is_profiling); return env; } +void FreeEnvironment(Environment* env) { + delete env; +} + + // Entry point for new node instances, also called directly for the main // node instance. static void StartNodeInstance(void* arg) { @@ -4293,60 +4292,60 @@ static void StartNodeInstance(void* arg) { array_buffer_allocator.zero_fill_field()); Local context = Context::New(isolate); Context::Scope context_scope(context); - Environment* env = CreateEnvironment(&isolate_data, - context, - instance_data->argc(), - instance_data->argv(), - instance_data->exec_argc(), - instance_data->exec_argv()); + Environment env(&isolate_data, context); + env.Start(instance_data->argc(), + instance_data->argv(), + instance_data->exec_argc(), + instance_data->exec_argv(), + v8_is_profiling); isolate->SetAbortOnUncaughtExceptionCallback( ShouldAbortOnUncaughtException); // Start debug agent when argv has --debug if (instance_data->use_debug_agent()) - StartDebug(env, debug_wait_connect); + StartDebug(&env, debug_wait_connect); { - Environment::AsyncCallbackScope callback_scope(env); - LoadEnvironment(env); + Environment::AsyncCallbackScope callback_scope(&env); + LoadEnvironment(&env); } - env->set_trace_sync_io(trace_sync_io); + env.set_trace_sync_io(trace_sync_io); // Enable debugger if (instance_data->use_debug_agent()) - EnableDebug(env); + EnableDebug(&env); { SealHandleScope seal(isolate); bool more; do { v8::platform::PumpMessageLoop(default_platform, isolate); - more = uv_run(env->event_loop(), UV_RUN_ONCE); + more = uv_run(env.event_loop(), UV_RUN_ONCE); if (more == false) { v8::platform::PumpMessageLoop(default_platform, isolate); - EmitBeforeExit(env); + EmitBeforeExit(&env); // Emit `beforeExit` if the loop became alive either after emitting // event, or after running some callbacks. - more = uv_loop_alive(env->event_loop()); - if (uv_run(env->event_loop(), UV_RUN_NOWAIT) != 0) + more = uv_loop_alive(env.event_loop()); + if (uv_run(env.event_loop(), UV_RUN_NOWAIT) != 0) more = true; } } while (more == true); } - env->set_trace_sync_io(false); + env.set_trace_sync_io(false); - int exit_code = EmitExit(env); + int exit_code = EmitExit(&env); if (instance_data->is_main()) instance_data->set_exit_code(exit_code); - RunAtExit(env); + RunAtExit(&env); #if HAVE_INSPECTOR - if (env->inspector_agent()->connected()) { + if (env.inspector_agent()->connected()) { // Restore signal dispositions, the app is done and is no longer // capable of handling signals. #ifdef __POSIX__ @@ -4359,16 +4358,13 @@ static void StartNodeInstance(void* arg) { CHECK_EQ(0, sigaction(nr, &act, nullptr)); } #endif - env->inspector_agent()->WaitForDisconnect(); + env.inspector_agent()->WaitForDisconnect(); } #endif #if defined(LEAK_SANITIZER) __lsan_do_leak_check(); #endif - - env->Dispose(); - env = nullptr; } uv_mutex_lock(&node_isolate_mutex);