diff --git a/src/README.md b/src/README.md index d29c03b8c18a7c..7ccc47cb0689e4 100644 --- a/src/README.md +++ b/src/README.md @@ -400,16 +400,22 @@ NODE_MODULE_CONTEXT_AWARE_INTERNAL(cares_wrap, Initialize) Some internal bindings, such as the HTTP parser, maintain internal state that only affects that particular binding. In that case, one common way to store -that state is through the use of `Environment::BindingScope`, which gives all -new functions created within it access to an object for storing such state. +that state is through the use of `Environment::AddBindingData`, which gives +binding functions access to an object for storing such state. That object is always a [`BaseObject`][]. +Its class needs to have a static `binding_data_name` field that based on a +constant string, in order to disambiguate it from other classes of this type, +and which could e.g. match the binding’s name. + ```c++ // In the HTTP parser source code file: class BindingData : public BaseObject { public: BindingData(Environment* env, Local obj) : BaseObject(env, obj) {} + static constexpr FastStringKey binding_data_name { "http_parser" }; + std::vector parser_buffer; bool parser_buffer_in_use = false; @@ -422,18 +428,17 @@ static void New(const FunctionCallbackInfo& args) { new Parser(binding_data, args.This()); } -// ... because the initialization function told the Environment to use this -// BindingData class for all functions created by it: +// ... because the initialization function told the Environment to store the +// BindingData object: void InitializeHttpParser(Local target, Local unused, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(context, target); - if (!binding_scope) return; - BindingData* binding_data = binding_scope.data; + BindingData* const binding_data = + env->AddBindingData(context, target); + if (binding_data == nullptr) return; - // Created within the Environment::BindingScope Local t = env->NewFunctionTemplate(Parser::New); ... } diff --git a/src/env-inl.h b/src/env-inl.h index 1c5acd940f3439..8103cb7682fdc3 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -333,58 +333,43 @@ inline Environment* Environment::GetCurrent( template inline T* Environment::GetBindingData(const v8::PropertyCallbackInfo& info) { - return GetBindingData(info.GetIsolate()->GetCurrentContext(), info.Data()); + return GetBindingData(info.GetIsolate()->GetCurrentContext()); } template inline T* Environment::GetBindingData( const v8::FunctionCallbackInfo& info) { - return GetBindingData(info.GetIsolate()->GetCurrentContext(), info.Data()); + return GetBindingData(info.GetIsolate()->GetCurrentContext()); } template -inline T* Environment::GetBindingData(v8::Local context, - v8::Local val) { - DCHECK(val->IsUint32()); - uint32_t index = val.As()->Value(); - BindingDataList* list = static_cast( +inline T* Environment::GetBindingData(v8::Local context) { + BindingDataStore* list = static_cast( context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingListIndex)); DCHECK_NOT_NULL(list); - DCHECK_GT(list->size(), index); - T* result = static_cast(list->at(index).get()); + auto it = list->find(T::binding_data_name); + DCHECK_NE(it, list->end()); + T* result = static_cast(it->second.get()); DCHECK_NOT_NULL(result); DCHECK_EQ(result->env(), GetCurrent(context)); return result; } template -inline std::pair Environment::NewBindingData( +inline T* Environment::AddBindingData( v8::Local context, v8::Local target) { DCHECK_EQ(GetCurrent(context), this); // This won't compile if T is not a BaseObject subclass. BaseObjectPtr item = MakeDetachedBaseObject(this, target); - BindingDataList* list = static_cast( + BindingDataStore* list = static_cast( context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingListIndex)); DCHECK_NOT_NULL(list); - size_t index = list->size(); - list->emplace_back(item); - return std::make_pair(item.get(), index); -} - -template -Environment::BindingScope::BindingScope(v8::Local context, - v8::Local target) - : env(Environment::GetCurrent(context)) { - std::tie(data, env->current_callback_data_) = - env->NewBindingData(context, target); -} - -template -Environment::BindingScope::~BindingScope() { - env->current_callback_data_ = env->default_callback_data_; + auto result = list->emplace(T::binding_data_name, item); + CHECK(result.second); + return item.get(); } inline Environment* Environment::GetThreadLocalEnv() { @@ -1103,8 +1088,7 @@ inline v8::Local v8::Local signature, v8::ConstructorBehavior behavior, v8::SideEffectType side_effect_type) { - v8::Local external = current_callback_data(); - return v8::FunctionTemplate::New(isolate(), callback, external, + return v8::FunctionTemplate::New(isolate(), callback, v8::Local(), signature, 0, behavior, side_effect_type); } @@ -1300,14 +1284,6 @@ v8::Local Environment::context() const { return PersistentToLocal::Strong(context_); } -v8::Local Environment::as_callback_data() const { - return v8::Integer::NewFromUnsigned(isolate(), default_callback_data_); -} - -v8::Local Environment::current_callback_data() const { - return v8::Integer::NewFromUnsigned(isolate(), current_callback_data_); -} - } // namespace node // These two files depend on each other. Including base_object-inl.h after this diff --git a/src/env.cc b/src/env.cc index b2d2d825c5649c..3efa5c3b9c98ab 100644 --- a/src/env.cc +++ b/src/env.cc @@ -261,16 +261,6 @@ 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(); @@ -282,11 +272,6 @@ void Environment::CreateProperties() { BaseObject::kInternalFieldCount); set_binding_data_ctor_template(templ); - Local ctor = templ->GetFunction(ctx).ToLocalChecked(); - Local obj = ctor->NewInstance(ctx).ToLocalChecked(); - uint32_t index = NewBindingData(ctx, obj).second; - default_callback_data_ = index; - current_callback_data_ = index; } // Store primordials setup by the per-context script in the environment. diff --git a/src/env.h b/src/env.h index d050f966a12384..146754d03ab728 100644 --- a/src/env.h +++ b/src/env.h @@ -864,38 +864,22 @@ class Environment : public MemoryRetainer { // Methods created using SetMethod(), SetPrototypeMethod(), etc. inside // this scope can access the created T* object using - // GetBindingData(args.Data()) later. + // GetBindingData(args) later. template - struct BindingScope { - explicit inline BindingScope(v8::Local context, - v8::Local target); - inline ~BindingScope(); - - T* data = nullptr; - Environment* env; - - inline operator bool() const { return data != nullptr; } - inline bool operator !() const { return data == nullptr; } - }; - + T* AddBindingData(v8::Local context, + v8::Local target); template static inline T* GetBindingData(const v8::PropertyCallbackInfo& info); template static inline T* GetBindingData( const v8::FunctionCallbackInfo& info); - - // Unwrap a subclass T object from a v8::Value which needs to be an - // v8::Uint32 - template - static inline T* GetBindingData(v8::Local context, - v8::Local val); - // Create a BindingData of subclass T, put it into the context binding list, - // return the index as an integer template - inline std::pair NewBindingData(v8::Local context, - v8::Local target); + static inline T* GetBindingData(v8::Local context); - typedef std::vector> BindingDataList; + typedef std::unordered_map< + FastStringKey, + BaseObjectPtr, + FastStringKey::Hash> BindingDataStore; static uv_key_t thread_local_env; static inline Environment* GetThreadLocalEnv(); @@ -1146,8 +1130,6 @@ class Environment : public MemoryRetainer { #undef V inline v8::Local context() const; - inline v8::Local as_callback_data() const; - inline v8::Local current_callback_data() const; #if HAVE_INSPECTOR inline inspector::Agent* inspector_agent() const { @@ -1443,7 +1425,7 @@ class Environment : public MemoryRetainer { void RequestInterruptFromV8(); static void CheckImmediate(uv_check_t* handle); - BindingDataList bindings_; + BindingDataStore bindings_; // Use an unordered_set, so that we have efficient insertion and removal. std::unordered_set context_; - uint32_t default_callback_data_; - uint32_t current_callback_data_; - // Keeps the main script source alive is one was passed to LoadEnvironment(). // We should probably find a way to just use plain `v8::String`s created from // the source passed to LoadEnvironment() directly instead. diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index b4b110cc2c839c..9b6c553a630e5d 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->current_callback_data(), + Local(), Signature::New(env->isolate(), t)); t->PrototypeTemplate()->SetAccessorProperty( diff --git a/src/node.cc b/src/node.cc index 649ac43fbe7f80..c9c8be571ff138 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_, current_callback_data()) + if (!CreateEnvVarProxy(context(), isolate_, Local()) .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 afdb2e3c270348..d53a6d2f2325fe 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->current_callback_data(), + Local(), Signature::New(env->isolate(), t)); @@ -5103,7 +5103,7 @@ void DiffieHellman::Initialize(Environment* env, Local target) { Local verify_error_getter_templ = FunctionTemplate::New(env->isolate(), DiffieHellman::VerifyErrorGetter, - env->current_callback_data(), + Local(), Signature::New(env->isolate(), t), /* length */ 0, ConstructorBehavior::kThrow, diff --git a/src/node_file.cc b/src/node_file.cc index 7f1c9d1d97f130..be18b18fddd413 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2303,15 +2303,18 @@ void BindingData::MemoryInfo(MemoryTracker* tracker) const { file_handle_read_wrap_freelist); } +// TODO(addaleax): Remove once we're on C++17. +constexpr FastStringKey BindingData::binding_data_name; + void Initialize(Local target, Local unused, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); Isolate* isolate = env->isolate(); - Environment::BindingScope binding_scope(context, target); - if (!binding_scope) return; - BindingData* binding_data = binding_scope.data; + BindingData* const binding_data = + env->AddBindingData(context, target); + if (binding_data == nullptr) return; env->SetMethod(target, "access", Access); env->SetMethod(target, "close", Close); diff --git a/src/node_file.h b/src/node_file.h index 90e71efd3ad84b..7690f272848425 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -26,6 +26,8 @@ class BindingData : public BaseObject { std::vector> file_handle_read_wrap_freelist; + static constexpr FastStringKey binding_data_name { "fs" }; + void MemoryInfo(MemoryTracker* tracker) const override; SET_SELF_SIZE(BindingData) SET_MEMORY_INFO_NAME(BindingData) diff --git a/src/node_http2.cc b/src/node_http2.cc index a6670155ce5ba3..25f353bb1477e0 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2941,6 +2941,9 @@ void Http2State::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("root_buffer", root_buffer); } +// TODO(addaleax): Remove once we're on C++17. +constexpr FastStringKey Http2State::binding_data_name; + // Set up the process.binding('http2') binding. void Initialize(Local target, Local unused, @@ -2950,9 +2953,8 @@ void Initialize(Local target, Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); - Environment::BindingScope binding_scope(context, target); - if (!binding_scope) return; - Http2State* state = binding_scope.data; + Http2State* const state = env->AddBindingData(context, target); + if (state == nullptr) return; #define SET_STATE_TYPEDARRAY(name, field) \ target->Set(context, \ diff --git a/src/node_http2_state.h b/src/node_http2_state.h index 7ebcaa03ca52d8..2e9d3e5d6bceb5 100644 --- a/src/node_http2_state.h +++ b/src/node_http2_state.h @@ -126,6 +126,8 @@ class Http2State : public BaseObject { SET_SELF_SIZE(Http2State) SET_MEMORY_INFO_NAME(Http2State) + static constexpr FastStringKey binding_data_name { "http2" }; + private: struct http2_state_internal { // doubles first so that they are always sizeof(double)-aligned diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 3abdd32e73dd60..c7a3df8d067af4 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -87,6 +87,8 @@ class BindingData : public BaseObject { BindingData(Environment* env, Local obj) : BaseObject(env, obj) {} + static constexpr FastStringKey binding_data_name { "http_parser" }; + std::vector parser_buffer; bool parser_buffer_in_use = false; @@ -97,6 +99,9 @@ class BindingData : public BaseObject { SET_MEMORY_INFO_NAME(BindingData) }; +// TODO(addaleax): Remove once we're on C++17. +constexpr FastStringKey BindingData::binding_data_name; + // helper class for the Parser struct StringPtr { StringPtr() { @@ -921,8 +926,9 @@ void InitializeHttpParser(Local target, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(context, target); - if (!binding_scope) return; + BindingData* const binding_data = + env->AddBindingData(context, target); + if (binding_data == nullptr) return; Local t = env->NewFunctionTemplate(Parser::New); t->InstanceTemplate()->SetInternalFieldCount(Parser::kInternalFieldCount); diff --git a/src/node_native_module_env.cc b/src/node_native_module_env.cc index b48ae962dd639f..be647b01c640b0 100644 --- a/src/node_native_module_env.cc +++ b/src/node_native_module_env.cc @@ -190,7 +190,7 @@ void NativeModuleEnv::Initialize(Local target, FIXED_ONE_BYTE_STRING(env->isolate(), "moduleCategories"), GetModuleCategories, nullptr, - env->as_callback_data(), + Local(), DEFAULT, None, SideEffectType::kHasNoSideEffect) diff --git a/src/node_process_object.cc b/src/node_process_object.cc index ee7d771c717c0a..ca17da9583efc8 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->current_callback_data(), + Local(), 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->current_callback_data()) + Local()) .FromJust()); } diff --git a/src/node_v8.cc b/src/node_v8.cc index 1cabab59c4761d..047ca594095d16 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -96,6 +96,8 @@ class BindingData : public BaseObject { heap_code_statistics_buffer(env->isolate(), kHeapCodeStatisticsPropertiesCount) {} + static constexpr FastStringKey binding_data_name { "v8" }; + AliasedFloat64Array heap_statistics_buffer; AliasedFloat64Array heap_space_statistics_buffer; AliasedFloat64Array heap_code_statistics_buffer; @@ -111,6 +113,8 @@ class BindingData : public BaseObject { SET_MEMORY_INFO_NAME(BindingData) }; +// TODO(addaleax): Remove once we're on C++17. +constexpr FastStringKey BindingData::binding_data_name; void CachedDataVersionTag(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -170,9 +174,9 @@ void Initialize(Local target, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(context, target); - if (!binding_scope) return; - BindingData* binding_data = binding_scope.data; + BindingData* const binding_data = + env->AddBindingData(context, target); + if (binding_data == nullptr) return; env->SetMethodNoSideEffect(target, "cachedDataVersionTag", CachedDataVersionTag); diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 90b2bb93416878..bd396110fccade 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->current_callback_data(), + Local(), 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 df6a989dfbe0b3..0c2c2bbc014cc3 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->current_callback_data(), + Local(), 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 1c42481f178a4b..9013bc9fe335c3 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->current_callback_data(), + Local(), signature); t->PrototypeTemplate()->SetAccessorProperty(env->fd_string(), diff --git a/src/util-inl.h b/src/util-inl.h index d44ee09fefbb3a..898b2eb4c55684 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -533,6 +533,37 @@ inline bool IsSafeJsInt(v8::Local v) { return false; } +constexpr size_t FastStringKey::HashImpl(const char* str) { + // Low-quality hash (djb2), but just fine for current use cases. + size_t h = 5381; + do { + h = h * 33 + *str; // NOLINT(readability/pointer_notation) + } while (*(str++) != '\0'); + return h; +} + +constexpr size_t FastStringKey::Hash::operator()( + const FastStringKey& key) const { + return key.cached_hash_; +} + +constexpr bool FastStringKey::operator==(const FastStringKey& other) const { + const char* p1 = name_; + const char* p2 = other.name_; + if (p1 == p2) return true; + do { + if (*p1 != *p2) return false; + } while (*(p1++) != '\0'); + return true; +} + +constexpr FastStringKey::FastStringKey(const char* name) + : name_(name), cached_hash_(HashImpl(name)) {} + +constexpr const char* FastStringKey::c_str() const { + return name_; +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/util.h b/src/util.h index 5eaa20b760168b..979091e6d6e002 100644 --- a/src/util.h +++ b/src/util.h @@ -764,6 +764,27 @@ class PersistentToLocal { } }; +// Can be used as a key for std::unordered_map when lookup performance is more +// important than size and the keys are statically used to avoid redundant hash +// computations. +class FastStringKey { + public: + constexpr explicit FastStringKey(const char* name); + + struct Hash { + constexpr size_t operator()(const FastStringKey& key) const; + }; + constexpr bool operator==(const FastStringKey& other) const; + + constexpr const char* c_str() const; + + private: + static constexpr size_t HashImpl(const char* str); + + const char* name_; + size_t cached_hash_; +}; + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS