From 6576b9b9d0f3de931e5e867a7b4b43319b187955 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 5 Mar 2019 01:28:45 +0100 Subject: [PATCH] src: make creating per-binding data structures easier Enable the state associated with the individual bindings, e.g. fs or http2, to be moved out of the Environment class, in order for these to be more modular and for Environment to be come less of a collection of random data fields. Do this by using a BaseObject as the data for callbacks, which can hold the per-binding state. By default, no per-binding state is available, although that can be configured when setting up the binding. PR-URL: https://github.com/nodejs/node/pull/32538 Reviewed-By: James M Snell --- src/base_object-inl.h | 7 +++--- src/base_object.h | 6 ++--- src/env-inl.h | 48 +++++++++++++++++++++++++++++++++----- src/env.cc | 36 ++++++++++++++++++++-------- src/env.h | 20 ++++++++++++++++ src/fs_event_wrap.cc | 2 +- src/node.cc | 2 +- src/node_crypto.cc | 4 ++-- src/node_process_object.cc | 4 ++-- src/stream_wrap.cc | 2 +- src/tls_wrap.cc | 2 +- src/udp_wrap.cc | 2 +- 12 files changed, 104 insertions(+), 31 deletions(-) diff --git a/src/base_object-inl.h b/src/base_object-inl.h index fc2611444c1af4..60ccf81cc559c5 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -100,15 +100,16 @@ Environment* BaseObject::env() const { return env_; } -BaseObject* BaseObject::FromJSObject(v8::Local obj) { - CHECK_GT(obj->InternalFieldCount(), 0); +BaseObject* BaseObject::FromJSObject(v8::Local value) { + v8::Local obj = value.As(); + DCHECK_GE(obj->InternalFieldCount(), BaseObject::kSlot); return static_cast( obj->GetAlignedPointerFromInternalField(BaseObject::kSlot)); } template -T* BaseObject::FromJSObject(v8::Local object) { +T* BaseObject::FromJSObject(v8::Local object) { return static_cast(FromJSObject(object)); } diff --git a/src/base_object.h b/src/base_object.h index 2c67445c31fbb6..03c69976ee4a68 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -61,9 +61,9 @@ class BaseObject : public MemoryRetainer { // was also passed to the `BaseObject()` constructor initially. // This may return `nullptr` if the C++ object has not been constructed yet, // e.g. when the JS object used `MakeLazilyInitializedJSTemplate`. - static inline BaseObject* FromJSObject(v8::Local object); + static inline BaseObject* FromJSObject(v8::Local object); template - static inline T* FromJSObject(v8::Local object); + static inline T* FromJSObject(v8::Local object); // Make the `v8::Global` a weak reference and, `delete` this object once // the JS object has been garbage collected and there are no (strong) @@ -152,7 +152,7 @@ class BaseObject : public MemoryRetainer { // Global alias for FromJSObject() to avoid churn. template -inline T* Unwrap(v8::Local obj) { +inline T* Unwrap(v8::Local obj) { return BaseObject::FromJSObject(obj); } diff --git a/src/env-inl.h b/src/env-inl.h index 69a082c95d9cca..dccfa7ce8c6549 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -327,16 +327,48 @@ inline Environment* Environment::GetCurrent( return GetFromCallbackData(info.Data()); } -inline Environment* Environment::GetFromCallbackData(v8::Local val) { +Environment* Environment::GetFromCallbackData(v8::Local val) { DCHECK(val->IsObject()); v8::Local obj = val.As(); - DCHECK_GE(obj->InternalFieldCount(), 1); - Environment* env = - static_cast(obj->GetAlignedPointerFromInternalField(0)); + DCHECK_GE(obj->InternalFieldCount(), + BaseObject::kInternalFieldCount); + Environment* env = Unwrap(obj)->env(); DCHECK(env->as_callback_data_template()->HasInstance(obj)); return env; } +template +Environment::BindingScope::BindingScope(Environment* env) : env(env) { + v8::Local callback_data; + if (!env->MakeBindingCallbackData().ToLocal(&callback_data)) + return; + data = Unwrap(callback_data); + + // No nesting allowed currently. + CHECK_EQ(env->current_callback_data(), env->as_callback_data()); + env->set_current_callback_data(callback_data); +} + +template +Environment::BindingScope::~BindingScope() { + env->set_current_callback_data(env->as_callback_data()); +} + +template +v8::MaybeLocal Environment::MakeBindingCallbackData() { + v8::Local ctor; + v8::Local obj; + if (!as_callback_data_template()->GetFunction(context()).ToLocal(&ctor) || + !ctor->NewInstance(context()).ToLocal(&obj)) { + return v8::MaybeLocal(); + } + T* data = new T(this, obj); + // This won't compile if T is not a BaseObject subclass. + CHECK_EQ(data, static_cast(data)); + data->MakeWeak(); + return obj; +} + inline Environment* Environment::GetThreadLocalEnv() { return static_cast(uv_key_get(&thread_local_env)); } @@ -1123,7 +1155,7 @@ inline v8::Local v8::Local signature, v8::ConstructorBehavior behavior, v8::SideEffectType side_effect_type) { - v8::Local external = as_callback_data(); + v8::Local external = current_callback_data(); return v8::FunctionTemplate::New(isolate(), callback, external, signature, 0, behavior, side_effect_type); } @@ -1261,7 +1293,7 @@ void Environment::modify_base_object_count(int64_t delta) { } int64_t Environment::base_object_count() const { - return base_object_count_; + return base_object_count_ - initial_base_object_count_; } void Environment::set_main_utf16(std::unique_ptr str) { @@ -1321,6 +1353,10 @@ void Environment::set_process_exit_handler( } } // namespace node +// These two files depend on each other. Including base_object-inl.h after this +// file is the easiest way to avoid issues with that circular dependency. +#include "base_object-inl.h" + #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #endif // SRC_ENV_INL_H_ diff --git a/src/env.cc b/src/env.cc index bee70ae58e22f6..19e93f6d538d41 100644 --- a/src/env.cc +++ b/src/env.cc @@ -258,18 +258,30 @@ void TrackingTraceStateObserver::UpdateTraceCategoryState() { USE(cb->Call(env_->context(), Undefined(isolate), arraysize(args), args)); } +class NoBindingData : public BaseObject { + public: + NoBindingData(Environment* env, Local obj) : BaseObject(env, obj) {} + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(NoBindingData) + SET_SELF_SIZE(NoBindingData) +}; + void Environment::CreateProperties() { HandleScope handle_scope(isolate_); Local ctx = context(); - Local templ = FunctionTemplate::New(isolate()); - templ->InstanceTemplate()->SetInternalFieldCount(1); - Local obj = templ->GetFunction(ctx) - .ToLocalChecked() - ->NewInstance(ctx) - .ToLocalChecked(); - obj->SetAlignedPointerInInternalField(0, this); - set_as_callback_data(obj); - set_as_callback_data_template(templ); + { + Context::Scope context_scope(ctx); + Local templ = FunctionTemplate::New(isolate()); + templ->InstanceTemplate()->SetInternalFieldCount( + BaseObject::kInternalFieldCount); + set_as_callback_data_template(templ); + + Local obj = MakeBindingCallbackData() + .ToLocalChecked(); + set_as_callback_data(obj); + set_current_callback_data(obj); + } // Store primordials setup by the per-context script in the environment. Local per_context_bindings = @@ -417,6 +429,10 @@ Environment::Environment(IsolateData* isolate_data, // TODO(joyeecheung): deserialize when the snapshot covers the environment // properties. CreateProperties(); + + // This adjusts the return value of base_object_count() so that tests that + // check the count do not have to account for internally created BaseObjects. + initial_base_object_count_ = base_object_count(); } Environment::~Environment() { @@ -467,7 +483,7 @@ Environment::~Environment() { } } - CHECK_EQ(base_object_count(), 0); + CHECK_EQ(base_object_count_, 0); } void Environment::InitializeLibuv(bool start_profiler_idle_notifier) { diff --git a/src/env.h b/src/env.h index b3bc553cc75f1e..a7a0981bb0e2b5 100644 --- a/src/env.h +++ b/src/env.h @@ -436,6 +436,7 @@ constexpr size_t kFsStatsBufferLength = V(async_hooks_promise_resolve_function, v8::Function) \ V(buffer_prototype_object, v8::Object) \ V(crypto_key_object_constructor, v8::Function) \ + V(current_callback_data, v8::Object) \ V(domain_callback, v8::Function) \ V(domexception_function, v8::Function) \ V(enhance_fatal_stack_after_inspector, v8::Function) \ @@ -871,6 +872,24 @@ class Environment : public MemoryRetainer { static inline Environment* GetFromCallbackData(v8::Local val); + // Methods created using SetMethod(), SetPrototypeMethod(), etc. inside + // this scope can access the created T* object using + // Unwrap(args.Data()) later. + template + struct BindingScope { + explicit inline BindingScope(Environment* env); + inline ~BindingScope(); + + T* data = nullptr; + Environment* env; + + inline operator bool() const { return data != nullptr; } + inline bool operator !() const { return data == nullptr; } + }; + + template + inline v8::MaybeLocal MakeBindingCallbackData(); + static uv_key_t thread_local_env; static inline Environment* GetThreadLocalEnv(); @@ -1461,6 +1480,7 @@ class Environment : public MemoryRetainer { bool started_cleanup_ = false; int64_t base_object_count_ = 0; + int64_t initial_base_object_count_ = 0; std::atomic_bool is_stopping_ { false }; std::function process_exit_handler_ { diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 858455bff2d42d..b4b110cc2c839c 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -108,7 +108,7 @@ void FSEventWrap::Initialize(Local target, Local get_initialized_templ = FunctionTemplate::New(env->isolate(), GetInitialized, - env->as_callback_data(), + env->current_callback_data(), Signature::New(env->isolate(), t)); t->PrototypeTemplate()->SetAccessorProperty( diff --git a/src/node.cc b/src/node.cc index 9e0e2464a02f4d..649ac43fbe7f80 100644 --- a/src/node.cc +++ b/src/node.cc @@ -331,7 +331,7 @@ MaybeLocal Environment::BootstrapNode() { Local env_string = FIXED_ONE_BYTE_STRING(isolate_, "env"); Local env_var_proxy; - if (!CreateEnvVarProxy(context(), isolate_, as_callback_data()) + if (!CreateEnvVarProxy(context(), isolate_, current_callback_data()) .ToLocal(&env_var_proxy) || process_object()->Set(context(), env_string, env_var_proxy).IsNothing()) { return MaybeLocal(); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d61890def60bac..f97c29e28e5a5f 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -505,7 +505,7 @@ void SecureContext::Initialize(Environment* env, Local target) { Local ctx_getter_templ = FunctionTemplate::New(env->isolate(), CtxGetter, - env->as_callback_data(), + env->current_callback_data(), Signature::New(env->isolate(), t)); @@ -5101,7 +5101,7 @@ void DiffieHellman::Initialize(Environment* env, Local target) { Local verify_error_getter_templ = FunctionTemplate::New(env->isolate(), DiffieHellman::VerifyErrorGetter, - env->as_callback_data(), + env->current_callback_data(), Signature::New(env->isolate(), t), /* length */ 0, ConstructorBehavior::kThrow, diff --git a/src/node_process_object.cc b/src/node_process_object.cc index ddbb58abe535b0..ee7d771c717c0a 100644 --- a/src/node_process_object.cc +++ b/src/node_process_object.cc @@ -147,7 +147,7 @@ void PatchProcessObject(const FunctionCallbackInfo& args) { FIXED_ONE_BYTE_STRING(isolate, "title"), ProcessTitleGetter, env->owns_process_state() ? ProcessTitleSetter : nullptr, - env->as_callback_data(), + env->current_callback_data(), DEFAULT, None, SideEffectType::kHasNoSideEffect) @@ -198,7 +198,7 @@ void PatchProcessObject(const FunctionCallbackInfo& args) { FIXED_ONE_BYTE_STRING(isolate, "debugPort"), DebugPortGetter, env->owns_process_state() ? DebugPortSetter : nullptr, - env->as_callback_data()) + env->current_callback_data()) .FromJust()); } diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index cc0d046ec11f2d..90b2bb93416878 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -142,7 +142,7 @@ Local LibuvStreamWrap::GetConstructorTemplate( Local get_write_queue_size = FunctionTemplate::New(env->isolate(), GetWriteQueueSize, - env->as_callback_data(), + env->current_callback_data(), Signature::New(env->isolate(), tmpl)); tmpl->PrototypeTemplate()->SetAccessorProperty( env->write_queue_size_string(), diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index b24c0df0c2bfe8..df6a989dfbe0b3 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -1276,7 +1276,7 @@ void TLSWrap::Initialize(Local target, Local get_write_queue_size = FunctionTemplate::New(env->isolate(), GetWriteQueueSize, - env->as_callback_data(), + env->current_callback_data(), Signature::New(env->isolate(), t)); t->PrototypeTemplate()->SetAccessorProperty( env->write_queue_size_string(), diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 770782ee9cf38e..1c42481f178a4b 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -145,7 +145,7 @@ void UDPWrap::Initialize(Local target, Local get_fd_templ = FunctionTemplate::New(env->isolate(), UDPWrap::GetFD, - env->as_callback_data(), + env->current_callback_data(), signature); t->PrototypeTemplate()->SetAccessorProperty(env->fd_string(),