From c3cd453cbae84ff6d639a2b8d7776a15b3e3431b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 31 May 2016 16:42:52 +0200 Subject: [PATCH] src: make IsolateData creation explicit Make it easier to reason about the lifetime and the ownership of the IsolateData instance by making its creation explicit and by removing reference counting logic. The creator of the Environment is now responsible for passing in the IsolateData instance and for keeping it alive as long as the Environment is alive. PR-URL: https://github.com/nodejs/node/pull/7082 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- src/debug-agent.cc | 10 +++++-- src/env-inl.h | 54 +++++++++--------------------------- src/env.h | 23 +++++----------- src/node.cc | 69 +++++++++++++++++++--------------------------- src/node.h | 20 +++++--------- 5 files changed, 63 insertions(+), 113 deletions(-) diff --git a/src/debug-agent.cc b/src/debug-agent.cc index e4e0ea061bd30f..78eacf49ab2fc8 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -172,9 +172,15 @@ void Agent::WorkerRun() { Local context = Context::New(isolate); Context::Scope context_scope(context); + + // FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences + // a nullptr when a context hasn't been entered first. The symbol registry + // is a lazily created JS object but object instantiation does not work + // without a context. + IsolateData isolate_data(isolate, &child_loop_); + Environment* env = CreateEnvironment( - isolate, - &child_loop_, + &isolate_data, context, arraysize(argv), argv, diff --git a/src/env-inl.h b/src/env-inl.h index a8a7d0255211b0..cf62ca7eb481db 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -15,28 +15,6 @@ namespace node { -inline IsolateData* IsolateData::Get(v8::Isolate* isolate) { - return static_cast(isolate->GetData(kIsolateSlot)); -} - -inline IsolateData* IsolateData::GetOrCreate(v8::Isolate* isolate, - uv_loop_t* loop) { - IsolateData* isolate_data = Get(isolate); - if (isolate_data == nullptr) { - isolate_data = new IsolateData(isolate, loop); - isolate->SetData(kIsolateSlot, isolate_data); - } - isolate_data->ref_count_ += 1; - return isolate_data; -} - -inline void IsolateData::Put() { - if (--ref_count_ == 0) { - isolate()->SetData(kIsolateSlot, nullptr); - delete this; - } -} - // Create string properties as internalized one byte strings. // // Internalized because it makes property lookups a little faster and because @@ -46,9 +24,8 @@ inline void IsolateData::Put() { // // One byte because our strings are ASCII and we can safely skip V8's UTF-8 // decoding step. It's a one-time cost, but why pay it when you don't have to? -inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* loop) - : event_loop_(loop), - isolate_(isolate), +inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop) + : #define V(PropertyName, StringValue) \ PropertyName ## _( \ isolate, \ @@ -71,16 +48,12 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* loop) sizeof(StringValue) - 1).ToLocalChecked()), PER_ISOLATE_STRING_PROPERTIES(V) #undef V - ref_count_(0) {} + isolate_(isolate), event_loop_(event_loop) {} inline uv_loop_t* IsolateData::event_loop() const { return event_loop_; } -inline v8::Isolate* IsolateData::isolate() const { - return isolate_; -} - inline Environment::AsyncHooks::AsyncHooks() { for (int i = 0; i < kFieldsCount; i++) fields_[i] = 0; } @@ -176,9 +149,9 @@ inline void Environment::ArrayBufferAllocatorInfo::reset_fill_flag() { fields_[kNoZeroFill] = 0; } -inline Environment* Environment::New(v8::Local context, - uv_loop_t* loop) { - Environment* env = new Environment(context, loop); +inline Environment* Environment::New(IsolateData* isolate_data, + v8::Local context) { + Environment* env = new Environment(isolate_data, context); env->AssignToContext(context); return env; } @@ -212,11 +185,11 @@ inline Environment* Environment::GetCurrent( return static_cast(data.As()->Value()); } -inline Environment::Environment(v8::Local context, - uv_loop_t* loop) +inline Environment::Environment(IsolateData* isolate_data, + v8::Local context) : isolate_(context->GetIsolate()), - isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)), - timer_base_(uv_now(loop)), + isolate_data_(isolate_data), + timer_base_(uv_now(isolate_data->event_loop())), using_domains_(false), printed_error_(false), trace_sync_io_(false), @@ -253,7 +226,6 @@ inline Environment::~Environment() { #define V(PropertyName, TypeName) PropertyName ## _.Reset(); ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V - isolate_data()->Put(); delete[] heap_statistics_buffer_; delete[] heap_space_statistics_buffer_; @@ -541,9 +513,9 @@ inline v8::Local Environment::NewInternalFieldObject() { #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ inline \ - v8::Local IsolateData::PropertyName() const { \ + v8::Local IsolateData::PropertyName(v8::Isolate* isolate) const { \ /* Strings are immutable so casting away const-ness here is okay. */ \ - return const_cast(this)->PropertyName ## _.Get(isolate()); \ + return const_cast(this)->PropertyName ## _.Get(isolate); \ } PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) PER_ISOLATE_STRING_PROPERTIES(VS) @@ -555,7 +527,7 @@ inline v8::Local Environment::NewInternalFieldObject() { #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ inline v8::Local Environment::PropertyName() const { \ - return isolate_data()->PropertyName(); \ + return isolate_data()->PropertyName(isolate()); \ } PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) PER_ISOLATE_STRING_PROPERTIES(VS) diff --git a/src/env.h b/src/env.h index bbbd3f4b36e68d..a6aea815fcd75a 100644 --- a/src/env.h +++ b/src/env.h @@ -304,14 +304,13 @@ RB_HEAD(ares_task_list, ares_task_t); class IsolateData { public: - static inline IsolateData* GetOrCreate(v8::Isolate* isolate, uv_loop_t* loop); - inline void Put(); + inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop); inline uv_loop_t* event_loop() const; #define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ - inline v8::Local PropertyName() const; + inline v8::Local PropertyName(v8::Isolate* isolate) const; PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) PER_ISOLATE_STRING_PROPERTIES(VS) #undef V @@ -319,15 +318,6 @@ class IsolateData { #undef VP private: - static const int kIsolateSlot = NODE_ISOLATE_SLOT; - - inline static IsolateData* Get(v8::Isolate* isolate); - inline explicit IsolateData(v8::Isolate* isolate, uv_loop_t* loop); - inline v8::Isolate* isolate() const; - - uv_loop_t* const event_loop_; - v8::Isolate* const isolate_; - #define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ @@ -338,7 +328,8 @@ class IsolateData { #undef VS #undef VP - unsigned int ref_count_; + v8::Isolate* const isolate_; + uv_loop_t* const event_loop_; DISALLOW_COPY_AND_ASSIGN(IsolateData); }; @@ -474,8 +465,8 @@ class Environment { const v8::PropertyCallbackInfo& info); // See CreateEnvironment() in src/node.cc. - static inline Environment* New(v8::Local context, - uv_loop_t* loop); + static inline Environment* New(IsolateData* isolate_data, + v8::Local context); inline void CleanupHandles(); inline void Dispose(); @@ -609,7 +600,7 @@ class Environment { static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX; private: - inline Environment(v8::Local context, uv_loop_t* loop); + inline Environment(IsolateData* isolate_data, v8::Local context); inline ~Environment(); inline IsolateData* isolate_data() const; diff --git a/src/node.cc b/src/node.cc index 258ebb596a08ca..025c1a921a884b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4264,42 +4264,6 @@ int EmitExit(Environment* env) { } -// Just a convenience method -Environment* CreateEnvironment(Isolate* isolate, - Local context, - int argc, - const char* const* argv, - int exec_argc, - const char* const* exec_argv) { - Environment* env; - Context::Scope context_scope(context); - - env = CreateEnvironment(isolate, - uv_default_loop(), - context, - argc, - argv, - exec_argc, - exec_argv); - - LoadEnvironment(env); - - return env; -} - -static Environment* CreateEnvironment(Isolate* isolate, - Local context, - NodeInstanceData* instance_data) { - return CreateEnvironment(isolate, - instance_data->event_loop(), - context, - instance_data->argc(), - instance_data->argv(), - instance_data->exec_argc(), - instance_data->exec_argv()); -} - - static void HandleCloseCb(uv_handle_t* handle) { Environment* env = reinterpret_cast(handle->data); env->FinishHandleCleanup(handle); @@ -4314,17 +4278,27 @@ static void HandleCleanup(Environment* env, } -Environment* CreateEnvironment(Isolate* isolate, - uv_loop_t* loop, +IsolateData* CreateIsolateData(Isolate* isolate, uv_loop_t* loop) { + return new IsolateData(isolate, loop); +} + + +void FreeIsolateData(IsolateData* isolate_data) { + delete isolate_data; +} + + +Environment* CreateEnvironment(IsolateData* isolate_data, Local context, int argc, const char* const* argv, int exec_argc, const char* const* exec_argv) { + Isolate* isolate = context->GetIsolate(); HandleScope handle_scope(isolate); Context::Scope context_scope(context); - Environment* env = Environment::New(context, loop); + Environment* env = Environment::New(isolate_data, context); isolate->SetAutorunMicrotasks(false); @@ -4412,10 +4386,23 @@ static void StartNodeInstance(void* arg) { Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); Local context = Context::New(isolate); - Environment* env = CreateEnvironment(isolate, context, instance_data); - array_buffer_allocator->set_env(env); Context::Scope context_scope(context); + // FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences + // a nullptr when a context hasn't been entered first. The symbol registry + // is a lazily created JS object but object instantiation does not work + // without a context. + IsolateData isolate_data(isolate, instance_data->event_loop()); + + Environment* env = CreateEnvironment(&isolate_data, + context, + instance_data->argc(), + instance_data->argv(), + instance_data->exec_argc(), + instance_data->exec_argv()); + + array_buffer_allocator->set_env(env); + isolate->SetAbortOnUncaughtExceptionCallback( ShouldAbortOnUncaughtException); diff --git a/src/node.h b/src/node.h index 42c5ac59d7ecf2..c1c149cdc30eb9 100644 --- a/src/node.h +++ b/src/node.h @@ -190,28 +190,22 @@ NODE_EXTERN void Init(int* argc, int* exec_argc, const char*** exec_argv); +class IsolateData; class Environment; -NODE_EXTERN Environment* CreateEnvironment(v8::Isolate* isolate, - struct uv_loop_s* loop, - v8::Local context, - int argc, - const char* const* argv, - int exec_argc, - const char* const* exec_argv); -NODE_EXTERN void LoadEnvironment(Environment* env); -NODE_EXTERN void FreeEnvironment(Environment* env); +NODE_EXTERN IsolateData* CreateIsolateData(v8::Isolate* isolate, + struct uv_loop_s* loop); +NODE_EXTERN void FreeIsolateData(IsolateData* isolate_data); -// NOTE: Calling this is the same as calling -// CreateEnvironment() + LoadEnvironment() from above. -// `uv_default_loop()` will be passed as `loop`. -NODE_EXTERN Environment* CreateEnvironment(v8::Isolate* isolate, +NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data, v8::Local context, int argc, const char* const* argv, int exec_argc, const char* const* exec_argv); +NODE_EXTERN void LoadEnvironment(Environment* env); +NODE_EXTERN void FreeEnvironment(Environment* env); NODE_EXTERN void EmitBeforeExit(Environment* env); NODE_EXTERN int EmitExit(Environment* env);