From de409d8ac39a647c5bb2f6b3326d46c051e71934 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 20 Apr 2020 03:51:05 +0800 Subject: [PATCH 1/5] src: retrieve binding data from the context Instead of passing them through the data bound to function templates, store references to them in a list embedded inside the context, and store the integer index (which is context-independent) in the function template data. This makes the function templates more context-independent, and makes it possible to embed binding data in non-main contexts. --- src/README.md | 10 ++-- src/env-inl.h | 90 ++++++++++++++++++++++------------- src/env.cc | 19 +++++--- src/env.h | 43 +++++++++++++---- src/node_binding.cc | 6 ++- src/node_context_data.h | 5 ++ src/node_env_var.cc | 2 +- src/node_file-inl.h | 2 +- src/node_file.cc | 14 +++--- src/node_file.h | 8 ++-- src/node_http2.cc | 8 ++-- src/node_http2_state.h | 67 ++++++++++++-------------- src/node_http_parser.cc | 9 ++-- src/node_native_module_env.cc | 2 +- src/node_process.h | 2 +- src/node_stat_watcher.cc | 3 +- src/node_v8.cc | 12 ++--- 17 files changed, 181 insertions(+), 121 deletions(-) diff --git a/src/README.md b/src/README.md index e2b256fba20de8..7666d15ab18ae5 100644 --- a/src/README.md +++ b/src/README.md @@ -402,13 +402,13 @@ 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 object is always a [`BaseObject`][]. +That object is always a `BindingDataBase`. ```c++ // In the HTTP parser source code file: -class BindingData : public BaseObject { +class BindingData : public BindingDataBase { public: - BindingData(Environment* env, Local obj) : BaseObject(env, obj) {} + BindingData(Environment* env, Local obj) : BindingDataBase(env, obj) {} std::vector parser_buffer; bool parser_buffer_in_use = false; @@ -418,7 +418,7 @@ class BindingData : public BaseObject { // Available for binding functions, e.g. the HTTP Parser constructor: static void New(const FunctionCallbackInfo& args) { - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); new Parser(binding_data, args.This()); } @@ -429,7 +429,7 @@ void InitializeHttpParser(Local target, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(env); + Environment::BindingScope binding_scope(env, context, target); if (!binding_scope) return; BindingData* binding_data = binding_scope.data; diff --git a/src/env-inl.h b/src/env-inl.h index cab967f10db530..967902d8781f52 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -287,6 +287,10 @@ inline void Environment::AssignToContext(v8::Local context, // Used by Environment::GetCurrent to know that we are on a node context. context->SetAlignedPointerInEmbedderData( ContextEmbedderIndex::kContextTag, Environment::kNodeContextTagPtr); + // Used to retrieve bindings + context->SetAlignedPointerInEmbedderData( + ContextEmbedderIndex::KBindingListIndex, &(this->bindings_)); + #if HAVE_INSPECTOR inspector_agent()->ContextCreated(context, info); #endif // HAVE_INSPECTOR @@ -318,55 +322,74 @@ inline Environment* Environment::GetCurrent(v8::Local context) { inline Environment* Environment::GetCurrent( const v8::FunctionCallbackInfo& info) { - return GetFromCallbackData(info.Data()); + return BindingDataBase::Unwrap(info)->env(); } template inline Environment* Environment::GetCurrent( const v8::PropertyCallbackInfo& info) { - return GetFromCallbackData(info.Data()); + return BindingDataBase::Unwrap(info)->env(); } -Environment* Environment::GetFromCallbackData(v8::Local val) { - DCHECK(val->IsObject()); - v8::Local obj = val.As(); - DCHECK_GE(obj->InternalFieldCount(), - BaseObject::kInternalFieldCount); - Environment* env = Unwrap(obj)->env(); - DCHECK(env->as_callback_data_template()->HasInstance(obj)); - return env; +inline BindingDataBase::BindingDataBase(Environment* env, + v8::Local target) + : BaseObject(env, target) {} + +template +inline T* BindingDataBase::Unwrap(const v8::PropertyCallbackInfo& info) { + return Unwrap(info.GetIsolate()->GetCurrentContext(), info.Data()); } template -Environment::BindingScope::BindingScope(Environment* env) : env(env) { - v8::Local callback_data; - if (!env->MakeBindingCallbackData().ToLocal(&callback_data)) - return; - data = Unwrap(callback_data); +inline T* BindingDataBase::Unwrap( + const v8::FunctionCallbackInfo& info) { + return Unwrap(info.GetIsolate()->GetCurrentContext(), info.Data()); +} - // No nesting allowed currently. - CHECK_EQ(env->current_callback_data(), env->as_callback_data()); - env->set_current_callback_data(callback_data); +template +inline T* BindingDataBase::Unwrap(v8::Local context, + v8::Local val) { + CHECK(val->IsUint32()); + uint32_t index = val.As()->Value(); + std::vector* list = + static_cast*>( + context->GetAlignedPointerFromEmbedderData( + ContextEmbedderIndex::KBindingListIndex)); + CHECK_GT(list->size(), index); + T* result = static_cast(list->at(index)); + return result; } template -Environment::BindingScope::~BindingScope() { - env->set_current_callback_data(env->as_callback_data()); +inline v8::Local BindingDataBase::New( + Environment* env, + v8::Local context, + v8::Local target) { + T* data = new T(env, target); + // This won't compile if T is not a BindingDataBase subclass. + BindingDataBase* item = static_cast(data); + std::vector* list = + static_cast*>( + context->GetAlignedPointerFromEmbedderData( + ContextEmbedderIndex::KBindingListIndex)); + size_t index = list->size(); + list->push_back(item); + return v8::Integer::NewFromUnsigned(env->isolate(), index).As(); } 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; +Environment::BindingScope::BindingScope(Environment* env, + v8::Local context, + v8::Local target) + : env(env) { + v8::Local index = BindingDataBase::New(env, context, target); + data = BindingDataBase::Unwrap(context, index); + env->set_current_callback_data(index); +} + +template +Environment::BindingScope::~BindingScope() { + env->set_current_callback_data(env->default_callback_data()); } inline Environment* Environment::GetThreadLocalEnv() { @@ -1085,7 +1108,7 @@ inline v8::Local v8::Local signature, v8::ConstructorBehavior behavior, v8::SideEffectType side_effect_type) { - v8::Local external = current_callback_data(); + v8::Local external = current_callback_data(); return v8::FunctionTemplate::New(isolate(), callback, external, signature, 0, behavior, side_effect_type); } @@ -1276,6 +1299,7 @@ void Environment::set_process_exit_handler( } ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES(V) ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) + ENVIRONMENT_CALLBACK_DATA(V) #undef V inline v8::Local Environment::context() const { diff --git a/src/env.cc b/src/env.cc index f966bfba1ee7e6..8e0267332375fb 100644 --- a/src/env.cc +++ b/src/env.cc @@ -50,6 +50,7 @@ using v8::String; using v8::Symbol; using v8::TracingController; using v8::TryCatch; +using v8::Uint32; using v8::Undefined; using v8::Value; using worker::Worker; @@ -261,9 +262,10 @@ void TrackingTraceStateObserver::UpdateTraceCategoryState() { USE(cb->Call(env_->context(), Undefined(isolate), arraysize(args), args)); } -class NoBindingData : public BaseObject { +class NoBindingData : public BindingDataBase { public: - NoBindingData(Environment* env, Local obj) : BaseObject(env, obj) {} + NoBindingData(Environment* env, Local obj) + : BindingDataBase(env, obj) {} SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(NoBindingData) @@ -273,17 +275,19 @@ class NoBindingData : public BaseObject { void Environment::CreateProperties() { HandleScope handle_scope(isolate_); Local ctx = context(); + { 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); + set_binding_data_ctor_template(templ); + Local ctor = templ->GetFunction(ctx).ToLocalChecked(); + Local obj = ctor->NewInstance(ctx).ToLocalChecked(); + Local index = BindingDataBase::New(this, ctx, obj); + set_default_callback_data(index); + set_current_callback_data(index); } // Store primordials setup by the per-context script in the environment. @@ -1133,6 +1137,7 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { #define V(PropertyName, TypeName) \ tracker->TrackField(#PropertyName, PropertyName()); ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) + ENVIRONMENT_CALLBACK_DATA(V) #undef V // FIXME(joyeecheung): track other fields in Environment. diff --git a/src/env.h b/src/env.h index e2dc5e866f451c..c6d47afbd7318c 100644 --- a/src/env.h +++ b/src/env.h @@ -390,9 +390,9 @@ constexpr size_t kFsStatsBufferLength = V(zero_return_string, "ZERO_RETURN") #define ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES(V) \ - V(as_callback_data_template, v8::FunctionTemplate) \ V(async_wrap_ctor_template, v8::FunctionTemplate) \ V(async_wrap_object_ctor_template, v8::FunctionTemplate) \ + V(binding_data_ctor_template, v8::FunctionTemplate) \ V(compiled_fn_entry_template, v8::ObjectTemplate) \ V(dir_instance_template, v8::ObjectTemplate) \ V(fd_constructor_template, v8::ObjectTemplate) \ @@ -419,8 +419,11 @@ constexpr size_t kFsStatsBufferLength = V(write_wrap_template, v8::ObjectTemplate) \ V(worker_heap_snapshot_taker_template, v8::ObjectTemplate) +#define ENVIRONMENT_CALLBACK_DATA(V) \ + V(default_callback_data, v8::Uint32) \ + V(current_callback_data, v8::Uint32) + #define ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) \ - V(as_callback_data, v8::Object) \ V(async_hooks_after_function, v8::Function) \ V(async_hooks_before_function, v8::Function) \ V(async_hooks_binding, v8::Object) \ @@ -429,7 +432,6 @@ 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) \ @@ -824,6 +826,28 @@ class CleanupHookCallback { uint64_t insertion_order_counter_; }; +class BindingDataBase : public BaseObject { + public: + // Exposed for subclasses + inline BindingDataBase(Environment* env, v8::Local target); + // Unwrap a subclass T object from a v8::Value which needs to be an + // v8::Uint32 + template + static inline T* Unwrap(const v8::PropertyCallbackInfo& info); + template + static inline T* Unwrap(const v8::FunctionCallbackInfo& info); + + template + static inline T* Unwrap(v8::Local context, + v8::Local val); + // Create a BindingData of subclass T, put it into the context binding list, + // return the index as v8::Integer + template + static inline v8::Local New(Environment* env, + v8::Local context, + v8::Local target); +}; + class Environment : public MemoryRetainer { public: Environment(const Environment&) = delete; @@ -864,14 +888,14 @@ class Environment : public MemoryRetainer { static inline Environment* GetCurrent( const v8::PropertyCallbackInfo& info); - 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); + explicit inline BindingScope(Environment* env, + v8::Local context, + v8::Local target); inline ~BindingScope(); T* data = nullptr; @@ -881,9 +905,6 @@ class Environment : public MemoryRetainer { inline bool operator !() const { return data == nullptr; } }; - template - inline v8::MaybeLocal MakeBindingCallbackData(); - static uv_key_t thread_local_env; static inline Environment* GetThreadLocalEnv(); @@ -1128,6 +1149,7 @@ class Environment : public MemoryRetainer { #define V(PropertyName, TypeName) \ inline v8::Local PropertyName() const; \ inline void set_ ## PropertyName(v8::Local value); + ENVIRONMENT_CALLBACK_DATA(V) ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES(V) #undef V @@ -1428,6 +1450,8 @@ class Environment : public MemoryRetainer { void RequestInterruptFromV8(); static void CheckImmediate(uv_check_t* handle); + std::vector bindings_; + // Use an unordered_set, so that we have efficient insertion and removal. std::unordered_set PropertyName ## _; + ENVIRONMENT_CALLBACK_DATA(V) ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES(V) #undef V diff --git a/src/node_binding.cc b/src/node_binding.cc index 592d0ca2a397e2..fdd84c39a20b01 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -230,6 +230,7 @@ namespace node { using v8::Context; using v8::Exception; +using v8::Function; using v8::FunctionCallbackInfo; using v8::Local; using v8::NewStringType; @@ -556,8 +557,11 @@ inline struct node_module* FindModule(struct node_module* list, static Local InitModule(Environment* env, node_module* mod, Local module) { - Local exports = Object::New(env->isolate()); // Internal bindings don't have a "module" object, only exports. + Local ctor = env->binding_data_ctor_template() + ->GetFunction(env->context()) + .ToLocalChecked(); + Local exports = ctor->NewInstance(env->context()).ToLocalChecked(); CHECK_NULL(mod->nm_register_func); CHECK_NOT_NULL(mod->nm_context_register_func); Local unused = Undefined(env->isolate()); diff --git a/src/node_context_data.h b/src/node_context_data.h index 807ba56c7c69fb..cae88c32cbb35a 100644 --- a/src/node_context_data.h +++ b/src/node_context_data.h @@ -25,11 +25,16 @@ namespace node { #define NODE_CONTEXT_TAG 35 #endif +#ifndef NODE_BINDING_LIST +#define NODE_BINDING_LIST_INDEX 36 +#endif + enum ContextEmbedderIndex { kEnvironment = NODE_CONTEXT_EMBEDDER_DATA_INDEX, kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX, kAllowWasmCodeGeneration = NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX, kContextTag = NODE_CONTEXT_TAG, + KBindingListIndex = NODE_BINDING_LIST_INDEX }; } // namespace node diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 8eaf79538a26e7..6e388caeccbb52 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -379,7 +379,7 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { MaybeLocal CreateEnvVarProxy(Local context, Isolate* isolate, - Local data) { + Local data) { EscapableHandleScope scope(isolate); Local env_proxy_template = ObjectTemplate::New(isolate); env_proxy_template->SetHandler(NamedPropertyHandlerConfiguration( diff --git a/src/node_file-inl.h b/src/node_file-inl.h index d30d77301005a3..f53bebbd74446e 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -227,7 +227,7 @@ FSReqBase* GetReqWrap(const v8::FunctionCallbackInfo& args, return Unwrap(value.As()); } - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); Environment* env = binding_data->env(); if (value->StrictEquals(env->fs_use_promises_symbol())) { if (use_bigint) { diff --git a/src/node_file.cc b/src/node_file.cc index 203ec5c8ca57b1..b96e64c8e9dce2 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -158,7 +158,7 @@ FileHandle* FileHandle::New(BindingData* binding_data, } void FileHandle::New(const FunctionCallbackInfo& args) { - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); Environment* env = binding_data->env(); CHECK(args.IsConstructCall()); CHECK(args[0]->IsInt32()); @@ -543,7 +543,7 @@ void FSReqCallback::SetReturnValue(const FunctionCallbackInfo& args) { void NewFSReqCallback(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); new FSReqCallback(binding_data, args.This(), args[0]->IsTrue()); } @@ -948,7 +948,7 @@ static void InternalModuleStat(const FunctionCallbackInfo& args) { } static void Stat(const FunctionCallbackInfo& args) { - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); Environment* env = binding_data->env(); const int argc = args.Length(); @@ -979,7 +979,7 @@ static void Stat(const FunctionCallbackInfo& args) { } static void LStat(const FunctionCallbackInfo& args) { - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); Environment* env = binding_data->env(); const int argc = args.Length(); @@ -1011,7 +1011,7 @@ static void LStat(const FunctionCallbackInfo& args) { } static void FStat(const FunctionCallbackInfo& args) { - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); Environment* env = binding_data->env(); const int argc = args.Length(); @@ -1674,7 +1674,7 @@ static void Open(const FunctionCallbackInfo& args) { } static void OpenFileHandle(const FunctionCallbackInfo& args) { - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); Environment* env = binding_data->env(); Isolate* isolate = env->isolate(); @@ -2309,7 +2309,7 @@ void Initialize(Local target, void* priv) { Environment* env = Environment::GetCurrent(context); Isolate* isolate = env->isolate(); - Environment::BindingScope binding_scope(env); + Environment::BindingScope binding_scope(env, context, target); if (!binding_scope) return; BindingData* binding_data = binding_scope.data; diff --git a/src/node_file.h b/src/node_file.h index 1fda81361fef79..36fcc240a79834 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -13,12 +13,12 @@ namespace fs { class FileHandleReadWrap; -class BindingData : public BaseObject { +class BindingData : public BindingDataBase { public: explicit BindingData(Environment* env, v8::Local wrap) - : BaseObject(env, wrap), - stats_field_array(env->isolate(), kFsStatsBufferLength), - stats_field_bigint_array(env->isolate(), kFsStatsBufferLength) {} + : BindingDataBase(env, wrap), + stats_field_array(env->isolate(), kFsStatsBufferLength), + stats_field_bigint_array(env->isolate(), kFsStatsBufferLength) {} AliasedFloat64Array stats_field_array; AliasedBigUint64Array stats_field_bigint_array; diff --git a/src/node_http2.cc b/src/node_http2.cc index 6637166e954267..4368ac6128265c 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2351,7 +2351,7 @@ void HttpErrorString(const FunctionCallbackInfo& args) { // would be suitable, for instance, for creating the Base64 // output for an HTTP2-Settings header field. void PackSettings(const FunctionCallbackInfo& args) { - Http2State* state = Unwrap(args.Data()); + Http2State* state = BindingDataBase::Unwrap(args); args.GetReturnValue().Set(Http2Settings::Pack(state)); } @@ -2359,7 +2359,7 @@ void PackSettings(const FunctionCallbackInfo& args) { // default SETTINGS. RefreshDefaultSettings updates that TypedArray with the // default values. void RefreshDefaultSettings(const FunctionCallbackInfo& args) { - Http2State* state = Unwrap(args.Data()); + Http2State* state = BindingDataBase::Unwrap(args); Http2Settings::RefreshDefaults(state); } @@ -2423,7 +2423,7 @@ void Http2Session::RefreshState(const FunctionCallbackInfo& args) { // Constructor for new Http2Session instances. void Http2Session::New(const FunctionCallbackInfo& args) { - Http2State* state = Unwrap(args.Data()); + Http2State* state = BindingDataBase::Unwrap(args); Environment* env = state->env(); CHECK(args.IsConstructCall()); SessionType type = @@ -2950,7 +2950,7 @@ void Initialize(Local target, Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); - Environment::BindingScope binding_scope(env); + Environment::BindingScope binding_scope(env, context, target); if (!binding_scope) return; Http2State* state = binding_scope.data; diff --git a/src/node_http2_state.h b/src/node_http2_state.h index 80efb40cd27589..e1e825a802f2e4 100644 --- a/src/node_http2_state.h +++ b/src/node_http2_state.h @@ -80,44 +80,39 @@ namespace http2 { IDX_SESSION_STATS_COUNT }; -class Http2State : public BaseObject { +class Http2State : public BindingDataBase { public: Http2State(Environment* env, v8::Local obj) - : BaseObject(env, obj), - root_buffer( - env->isolate(), - sizeof(http2_state_internal)), - session_state_buffer( - env->isolate(), - offsetof(http2_state_internal, session_state_buffer), - IDX_SESSION_STATE_COUNT, - root_buffer), - stream_state_buffer( - env->isolate(), - offsetof(http2_state_internal, stream_state_buffer), - IDX_STREAM_STATE_COUNT, - root_buffer), - stream_stats_buffer( - env->isolate(), - offsetof(http2_state_internal, stream_stats_buffer), - IDX_STREAM_STATS_COUNT, - root_buffer), - session_stats_buffer( - env->isolate(), - offsetof(http2_state_internal, session_stats_buffer), - IDX_SESSION_STATS_COUNT, - root_buffer), - options_buffer( - env->isolate(), - offsetof(http2_state_internal, options_buffer), - IDX_OPTIONS_FLAGS + 1, - root_buffer), - settings_buffer( - env->isolate(), - offsetof(http2_state_internal, settings_buffer), - IDX_SETTINGS_COUNT + 1, - root_buffer) { - } + : BindingDataBase(env, obj), + root_buffer(env->isolate(), sizeof(http2_state_internal)), + session_state_buffer( + env->isolate(), + offsetof(http2_state_internal, session_state_buffer), + IDX_SESSION_STATE_COUNT, + root_buffer), + stream_state_buffer( + env->isolate(), + offsetof(http2_state_internal, stream_state_buffer), + IDX_STREAM_STATE_COUNT, + root_buffer), + stream_stats_buffer( + env->isolate(), + offsetof(http2_state_internal, stream_stats_buffer), + IDX_STREAM_STATS_COUNT, + root_buffer), + session_stats_buffer( + env->isolate(), + offsetof(http2_state_internal, session_stats_buffer), + IDX_SESSION_STATS_COUNT, + root_buffer), + options_buffer(env->isolate(), + offsetof(http2_state_internal, options_buffer), + IDX_OPTIONS_FLAGS + 1, + root_buffer), + settings_buffer(env->isolate(), + offsetof(http2_state_internal, settings_buffer), + IDX_SETTINGS_COUNT + 1, + root_buffer) {} AliasedUint8Array root_buffer; AliasedFloat64Array session_state_buffer; diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index f98e9f5c7b6dab..9b161c45aa79fa 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -82,9 +82,10 @@ inline bool IsOWS(char c) { return c == ' ' || c == '\t'; } -class BindingData : public BaseObject { +class BindingData : public BindingDataBase { public: - BindingData(Environment* env, Local obj) : BaseObject(env, obj) {} + BindingData(Environment* env, Local obj) + : BindingDataBase(env, obj) {} std::vector parser_buffer; bool parser_buffer_in_use = false; @@ -444,7 +445,7 @@ class Parser : public AsyncWrap, public StreamListener { } static void New(const FunctionCallbackInfo& args) { - BindingData* binding_data = Unwrap(args.Data()); + BindingData* binding_data = BindingDataBase::Unwrap(args); new Parser(binding_data, args.This()); } @@ -920,7 +921,7 @@ void InitializeHttpParser(Local target, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(env); + Environment::BindingScope binding_scope(env, context, target); if (!binding_scope) return; Local t = env->NewFunctionTemplate(Parser::New); diff --git a/src/node_native_module_env.cc b/src/node_native_module_env.cc index b48ae962dd639f..9ec5ddf9a84ab6 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(), + env->default_callback_data(), DEFAULT, None, SideEffectType::kHasNoSideEffect) diff --git a/src/node_process.h b/src/node_process.h index ad86b449f95290..4c262beef34363 100644 --- a/src/node_process.h +++ b/src/node_process.h @@ -9,7 +9,7 @@ namespace node { v8::MaybeLocal CreateEnvVarProxy(v8::Local context, v8::Isolate* isolate, - v8::Local data); + v8::Local data); // Most of the time, it's best to use `console.error` to write // to the process.stderr stream. However, in some cases, such as diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index af23affc412935..a35c247b6a16e8 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -95,7 +95,8 @@ void StatWatcher::Callback(uv_fs_poll_t* handle, void StatWatcher::New(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); - fs::BindingData* binding_data = Unwrap(args.Data()); + fs::BindingData* binding_data = + BindingDataBase::Unwrap(args); new StatWatcher(binding_data, args.This(), args[0]->IsTrue()); } diff --git a/src/node_v8.cc b/src/node_v8.cc index 7174d9ae7f830b..03bbdd83750b1e 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -86,10 +86,10 @@ static const size_t kHeapCodeStatisticsPropertiesCount = HEAP_CODE_STATISTICS_PROPERTIES(V); #undef V -class BindingData : public BaseObject { +class BindingData : public BindingDataBase { public: BindingData(Environment* env, Local obj) - : BaseObject(env, obj), + : BindingDataBase(env, obj), heap_statistics_buffer(env->isolate(), kHeapStatisticsPropertiesCount), heap_space_statistics_buffer(env->isolate(), kHeapSpaceStatisticsPropertiesCount), @@ -121,7 +121,7 @@ void CachedDataVersionTag(const FunctionCallbackInfo& args) { } void UpdateHeapStatisticsBuffer(const FunctionCallbackInfo& args) { - BindingData* data = Unwrap(args.Data()); + BindingData* data = BindingDataBase::Unwrap(args); HeapStatistics s; args.GetIsolate()->GetHeapStatistics(&s); AliasedFloat64Array& buffer = data->heap_statistics_buffer; @@ -132,7 +132,7 @@ void UpdateHeapStatisticsBuffer(const FunctionCallbackInfo& args) { void UpdateHeapSpaceStatisticsBuffer(const FunctionCallbackInfo& args) { - BindingData* data = Unwrap(args.Data()); + BindingData* data = BindingDataBase::Unwrap(args); HeapSpaceStatistics s; Isolate* const isolate = args.GetIsolate(); CHECK(args[0]->IsUint32()); @@ -147,7 +147,7 @@ void UpdateHeapSpaceStatisticsBuffer(const FunctionCallbackInfo& args) { } void UpdateHeapCodeStatisticsBuffer(const FunctionCallbackInfo& args) { - BindingData* data = Unwrap(args.Data()); + BindingData* data = BindingDataBase::Unwrap(args); HeapCodeStatistics s; args.GetIsolate()->GetHeapCodeAndMetadataStatistics(&s); AliasedFloat64Array& buffer = data->heap_code_statistics_buffer; @@ -170,7 +170,7 @@ void Initialize(Local target, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(env); + Environment::BindingScope binding_scope(env, context, target); if (!binding_scope) return; BindingData* binding_data = binding_scope.data; From a3e137103267aa50e971d45f370e1cdbed26b3b0 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 29 Apr 2020 04:59:33 +0200 Subject: [PATCH 2/5] squash! src: retrieve binding data from the context Co-authored-by: Anna Henningsen --- src/README.md | 10 ++-- src/env-inl.h | 87 ++++++++++++++++++----------------- src/env.cc | 14 +++--- src/env.h | 59 +++++++++++------------- src/node_context_data.h | 2 +- src/node_file-inl.h | 2 +- src/node_file.cc | 14 +++--- src/node_file.h | 4 +- src/node_http2.cc | 8 ++-- src/node_http2_state.h | 4 +- src/node_http_parser.cc | 8 ++-- src/node_native_module_env.cc | 2 +- src/node_stat_watcher.cc | 2 +- src/node_v8.cc | 12 ++--- 14 files changed, 113 insertions(+), 115 deletions(-) diff --git a/src/README.md b/src/README.md index 7666d15ab18ae5..d29c03b8c18a7c 100644 --- a/src/README.md +++ b/src/README.md @@ -402,13 +402,13 @@ 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 object is always a `BindingDataBase`. +That object is always a [`BaseObject`][]. ```c++ // In the HTTP parser source code file: -class BindingData : public BindingDataBase { +class BindingData : public BaseObject { public: - BindingData(Environment* env, Local obj) : BindingDataBase(env, obj) {} + BindingData(Environment* env, Local obj) : BaseObject(env, obj) {} std::vector parser_buffer; bool parser_buffer_in_use = false; @@ -418,7 +418,7 @@ class BindingData : public BindingDataBase { // Available for binding functions, e.g. the HTTP Parser constructor: static void New(const FunctionCallbackInfo& args) { - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); new Parser(binding_data, args.This()); } @@ -429,7 +429,7 @@ void InitializeHttpParser(Local target, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(env, context, target); + Environment::BindingScope binding_scope(context, target); if (!binding_scope) return; BindingData* binding_data = binding_scope.data; diff --git a/src/env-inl.h b/src/env-inl.h index 967902d8781f52..1c5acd940f3439 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -289,7 +289,7 @@ inline void Environment::AssignToContext(v8::Local context, ContextEmbedderIndex::kContextTag, Environment::kNodeContextTagPtr); // Used to retrieve bindings context->SetAlignedPointerInEmbedderData( - ContextEmbedderIndex::KBindingListIndex, &(this->bindings_)); + ContextEmbedderIndex::kBindingListIndex, &(this->bindings_)); #if HAVE_INSPECTOR inspector_agent()->ContextCreated(context, info); @@ -322,74 +322,69 @@ inline Environment* Environment::GetCurrent(v8::Local context) { inline Environment* Environment::GetCurrent( const v8::FunctionCallbackInfo& info) { - return BindingDataBase::Unwrap(info)->env(); + return GetCurrent(info.GetIsolate()->GetCurrentContext()); } template inline Environment* Environment::GetCurrent( const v8::PropertyCallbackInfo& info) { - return BindingDataBase::Unwrap(info)->env(); + return GetCurrent(info.GetIsolate()->GetCurrentContext()); } -inline BindingDataBase::BindingDataBase(Environment* env, - v8::Local target) - : BaseObject(env, target) {} - template -inline T* BindingDataBase::Unwrap(const v8::PropertyCallbackInfo& info) { - return Unwrap(info.GetIsolate()->GetCurrentContext(), info.Data()); +inline T* Environment::GetBindingData(const v8::PropertyCallbackInfo& info) { + return GetBindingData(info.GetIsolate()->GetCurrentContext(), info.Data()); } template -inline T* BindingDataBase::Unwrap( +inline T* Environment::GetBindingData( const v8::FunctionCallbackInfo& info) { - return Unwrap(info.GetIsolate()->GetCurrentContext(), info.Data()); + return GetBindingData(info.GetIsolate()->GetCurrentContext(), info.Data()); } template -inline T* BindingDataBase::Unwrap(v8::Local context, - v8::Local val) { - CHECK(val->IsUint32()); +inline T* Environment::GetBindingData(v8::Local context, + v8::Local val) { + DCHECK(val->IsUint32()); uint32_t index = val.As()->Value(); - std::vector* list = - static_cast*>( - context->GetAlignedPointerFromEmbedderData( - ContextEmbedderIndex::KBindingListIndex)); - CHECK_GT(list->size(), index); - T* result = static_cast(list->at(index)); + BindingDataList* list = static_cast( + context->GetAlignedPointerFromEmbedderData( + ContextEmbedderIndex::kBindingListIndex)); + DCHECK_NOT_NULL(list); + DCHECK_GT(list->size(), index); + T* result = static_cast(list->at(index).get()); + DCHECK_NOT_NULL(result); + DCHECK_EQ(result->env(), GetCurrent(context)); return result; } template -inline v8::Local BindingDataBase::New( - Environment* env, +inline std::pair Environment::NewBindingData( v8::Local context, v8::Local target) { - T* data = new T(env, target); - // This won't compile if T is not a BindingDataBase subclass. - BindingDataBase* item = static_cast(data); - std::vector* list = - static_cast*>( - context->GetAlignedPointerFromEmbedderData( - ContextEmbedderIndex::KBindingListIndex)); + 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( + context->GetAlignedPointerFromEmbedderData( + ContextEmbedderIndex::kBindingListIndex)); + DCHECK_NOT_NULL(list); size_t index = list->size(); - list->push_back(item); - return v8::Integer::NewFromUnsigned(env->isolate(), index).As(); + list->emplace_back(item); + return std::make_pair(item.get(), index); } template -Environment::BindingScope::BindingScope(Environment* env, - v8::Local context, +Environment::BindingScope::BindingScope(v8::Local context, v8::Local target) - : env(env) { - v8::Local index = BindingDataBase::New(env, context, target); - data = BindingDataBase::Unwrap(context, index); - env->set_current_callback_data(index); + : env(Environment::GetCurrent(context)) { + std::tie(data, env->current_callback_data_) = + env->NewBindingData(context, target); } template Environment::BindingScope::~BindingScope() { - env->set_current_callback_data(env->default_callback_data()); + env->current_callback_data_ = env->default_callback_data_; } inline Environment* Environment::GetThreadLocalEnv() { @@ -1299,12 +1294,20 @@ void Environment::set_process_exit_handler( } ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES(V) ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) - ENVIRONMENT_CALLBACK_DATA(V) #undef V - inline v8::Local Environment::context() const { - return PersistentToLocal::Strong(context_); - } +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 8e0267332375fb..b2d2d825c5649c 100644 --- a/src/env.cc +++ b/src/env.cc @@ -50,7 +50,6 @@ using v8::String; using v8::Symbol; using v8::TracingController; using v8::TryCatch; -using v8::Uint32; using v8::Undefined; using v8::Value; using worker::Worker; @@ -262,10 +261,10 @@ void TrackingTraceStateObserver::UpdateTraceCategoryState() { USE(cb->Call(env_->context(), Undefined(isolate), arraysize(args), args)); } -class NoBindingData : public BindingDataBase { +class NoBindingData : public BaseObject { public: NoBindingData(Environment* env, Local obj) - : BindingDataBase(env, obj) {} + : BaseObject(env, obj) {} SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(NoBindingData) @@ -285,9 +284,9 @@ void Environment::CreateProperties() { set_binding_data_ctor_template(templ); Local ctor = templ->GetFunction(ctx).ToLocalChecked(); Local obj = ctor->NewInstance(ctx).ToLocalChecked(); - Local index = BindingDataBase::New(this, ctx, obj); - set_default_callback_data(index); - set_current_callback_data(index); + 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. @@ -678,6 +677,8 @@ void Environment::RunCleanup() { started_cleanup_ = true; TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment), "RunCleanup", this); + bindings_.clear(); + initial_base_object_count_ = 0; CleanupHandles(); while (!cleanup_hooks_.empty()) { @@ -1137,7 +1138,6 @@ void Environment::MemoryInfo(MemoryTracker* tracker) const { #define V(PropertyName, TypeName) \ tracker->TrackField(#PropertyName, PropertyName()); ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) - ENVIRONMENT_CALLBACK_DATA(V) #undef V // FIXME(joyeecheung): track other fields in Environment. diff --git a/src/env.h b/src/env.h index c6d47afbd7318c..d050f966a12384 100644 --- a/src/env.h +++ b/src/env.h @@ -419,10 +419,6 @@ constexpr size_t kFsStatsBufferLength = V(write_wrap_template, v8::ObjectTemplate) \ V(worker_heap_snapshot_taker_template, v8::ObjectTemplate) -#define ENVIRONMENT_CALLBACK_DATA(V) \ - V(default_callback_data, v8::Uint32) \ - V(current_callback_data, v8::Uint32) - #define ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) \ V(async_hooks_after_function, v8::Function) \ V(async_hooks_before_function, v8::Function) \ @@ -826,28 +822,6 @@ class CleanupHookCallback { uint64_t insertion_order_counter_; }; -class BindingDataBase : public BaseObject { - public: - // Exposed for subclasses - inline BindingDataBase(Environment* env, v8::Local target); - // Unwrap a subclass T object from a v8::Value which needs to be an - // v8::Uint32 - template - static inline T* Unwrap(const v8::PropertyCallbackInfo& info); - template - static inline T* Unwrap(const v8::FunctionCallbackInfo& info); - - template - static inline T* Unwrap(v8::Local context, - v8::Local val); - // Create a BindingData of subclass T, put it into the context binding list, - // return the index as v8::Integer - template - static inline v8::Local New(Environment* env, - v8::Local context, - v8::Local target); -}; - class Environment : public MemoryRetainer { public: Environment(const Environment&) = delete; @@ -890,11 +864,10 @@ class Environment : public MemoryRetainer { // Methods created using SetMethod(), SetPrototypeMethod(), etc. inside // this scope can access the created T* object using - // Unwrap(args.Data()) later. + // GetBindingData(args.Data()) later. template struct BindingScope { - explicit inline BindingScope(Environment* env, - v8::Local context, + explicit inline BindingScope(v8::Local context, v8::Local target); inline ~BindingScope(); @@ -905,6 +878,25 @@ class Environment : public MemoryRetainer { inline bool operator !() const { return data == nullptr; } }; + 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); + + typedef std::vector> BindingDataList; + static uv_key_t thread_local_env; static inline Environment* GetThreadLocalEnv(); @@ -1149,12 +1141,13 @@ class Environment : public MemoryRetainer { #define V(PropertyName, TypeName) \ inline v8::Local PropertyName() const; \ inline void set_ ## PropertyName(v8::Local value); - ENVIRONMENT_CALLBACK_DATA(V) ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES(V) #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 { @@ -1450,7 +1443,7 @@ class Environment : public MemoryRetainer { void RequestInterruptFromV8(); static void CheckImmediate(uv_check_t* handle); - std::vector bindings_; + BindingDataList bindings_; // Use an unordered_set, so that we have efficient insertion and removal. std::unordered_set PropertyName ## _; - ENVIRONMENT_CALLBACK_DATA(V) ENVIRONMENT_STRONG_PERSISTENT_VALUES(V) ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES(V) #undef V v8::Global 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/node_context_data.h b/src/node_context_data.h index cae88c32cbb35a..912e65b42707fa 100644 --- a/src/node_context_data.h +++ b/src/node_context_data.h @@ -34,7 +34,7 @@ enum ContextEmbedderIndex { kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX, kAllowWasmCodeGeneration = NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX, kContextTag = NODE_CONTEXT_TAG, - KBindingListIndex = NODE_BINDING_LIST_INDEX + kBindingListIndex = NODE_BINDING_LIST_INDEX }; } // namespace node diff --git a/src/node_file-inl.h b/src/node_file-inl.h index f53bebbd74446e..2ad8c73f05e490 100644 --- a/src/node_file-inl.h +++ b/src/node_file-inl.h @@ -227,7 +227,7 @@ FSReqBase* GetReqWrap(const v8::FunctionCallbackInfo& args, return Unwrap(value.As()); } - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); Environment* env = binding_data->env(); if (value->StrictEquals(env->fs_use_promises_symbol())) { if (use_bigint) { diff --git a/src/node_file.cc b/src/node_file.cc index b96e64c8e9dce2..7f1c9d1d97f130 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -158,7 +158,7 @@ FileHandle* FileHandle::New(BindingData* binding_data, } void FileHandle::New(const FunctionCallbackInfo& args) { - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); Environment* env = binding_data->env(); CHECK(args.IsConstructCall()); CHECK(args[0]->IsInt32()); @@ -543,7 +543,7 @@ void FSReqCallback::SetReturnValue(const FunctionCallbackInfo& args) { void NewFSReqCallback(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); new FSReqCallback(binding_data, args.This(), args[0]->IsTrue()); } @@ -948,7 +948,7 @@ static void InternalModuleStat(const FunctionCallbackInfo& args) { } static void Stat(const FunctionCallbackInfo& args) { - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); Environment* env = binding_data->env(); const int argc = args.Length(); @@ -979,7 +979,7 @@ static void Stat(const FunctionCallbackInfo& args) { } static void LStat(const FunctionCallbackInfo& args) { - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); Environment* env = binding_data->env(); const int argc = args.Length(); @@ -1011,7 +1011,7 @@ static void LStat(const FunctionCallbackInfo& args) { } static void FStat(const FunctionCallbackInfo& args) { - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); Environment* env = binding_data->env(); const int argc = args.Length(); @@ -1674,7 +1674,7 @@ static void Open(const FunctionCallbackInfo& args) { } static void OpenFileHandle(const FunctionCallbackInfo& args) { - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); Environment* env = binding_data->env(); Isolate* isolate = env->isolate(); @@ -2309,7 +2309,7 @@ void Initialize(Local target, void* priv) { Environment* env = Environment::GetCurrent(context); Isolate* isolate = env->isolate(); - Environment::BindingScope binding_scope(env, context, target); + Environment::BindingScope binding_scope(context, target); if (!binding_scope) return; BindingData* binding_data = binding_scope.data; diff --git a/src/node_file.h b/src/node_file.h index 36fcc240a79834..90e71efd3ad84b 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -13,10 +13,10 @@ namespace fs { class FileHandleReadWrap; -class BindingData : public BindingDataBase { +class BindingData : public BaseObject { public: explicit BindingData(Environment* env, v8::Local wrap) - : BindingDataBase(env, wrap), + : BaseObject(env, wrap), stats_field_array(env->isolate(), kFsStatsBufferLength), stats_field_bigint_array(env->isolate(), kFsStatsBufferLength) {} diff --git a/src/node_http2.cc b/src/node_http2.cc index 4368ac6128265c..a6670155ce5ba3 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2351,7 +2351,7 @@ void HttpErrorString(const FunctionCallbackInfo& args) { // would be suitable, for instance, for creating the Base64 // output for an HTTP2-Settings header field. void PackSettings(const FunctionCallbackInfo& args) { - Http2State* state = BindingDataBase::Unwrap(args); + Http2State* state = Environment::GetBindingData(args); args.GetReturnValue().Set(Http2Settings::Pack(state)); } @@ -2359,7 +2359,7 @@ void PackSettings(const FunctionCallbackInfo& args) { // default SETTINGS. RefreshDefaultSettings updates that TypedArray with the // default values. void RefreshDefaultSettings(const FunctionCallbackInfo& args) { - Http2State* state = BindingDataBase::Unwrap(args); + Http2State* state = Environment::GetBindingData(args); Http2Settings::RefreshDefaults(state); } @@ -2423,7 +2423,7 @@ void Http2Session::RefreshState(const FunctionCallbackInfo& args) { // Constructor for new Http2Session instances. void Http2Session::New(const FunctionCallbackInfo& args) { - Http2State* state = BindingDataBase::Unwrap(args); + Http2State* state = Environment::GetBindingData(args); Environment* env = state->env(); CHECK(args.IsConstructCall()); SessionType type = @@ -2950,7 +2950,7 @@ void Initialize(Local target, Isolate* isolate = env->isolate(); HandleScope handle_scope(isolate); - Environment::BindingScope binding_scope(env, context, target); + Environment::BindingScope binding_scope(context, target); if (!binding_scope) return; Http2State* state = binding_scope.data; diff --git a/src/node_http2_state.h b/src/node_http2_state.h index e1e825a802f2e4..7ebcaa03ca52d8 100644 --- a/src/node_http2_state.h +++ b/src/node_http2_state.h @@ -80,10 +80,10 @@ namespace http2 { IDX_SESSION_STATS_COUNT }; -class Http2State : public BindingDataBase { +class Http2State : public BaseObject { public: Http2State(Environment* env, v8::Local obj) - : BindingDataBase(env, obj), + : BaseObject(env, obj), root_buffer(env->isolate(), sizeof(http2_state_internal)), session_state_buffer( env->isolate(), diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 9b161c45aa79fa..3abdd32e73dd60 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -82,10 +82,10 @@ inline bool IsOWS(char c) { return c == ' ' || c == '\t'; } -class BindingData : public BindingDataBase { +class BindingData : public BaseObject { public: BindingData(Environment* env, Local obj) - : BindingDataBase(env, obj) {} + : BaseObject(env, obj) {} std::vector parser_buffer; bool parser_buffer_in_use = false; @@ -445,7 +445,7 @@ class Parser : public AsyncWrap, public StreamListener { } static void New(const FunctionCallbackInfo& args) { - BindingData* binding_data = BindingDataBase::Unwrap(args); + BindingData* binding_data = Environment::GetBindingData(args); new Parser(binding_data, args.This()); } @@ -921,7 +921,7 @@ void InitializeHttpParser(Local target, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(env, context, target); + Environment::BindingScope binding_scope(context, target); if (!binding_scope) return; Local t = env->NewFunctionTemplate(Parser::New); diff --git a/src/node_native_module_env.cc b/src/node_native_module_env.cc index 9ec5ddf9a84ab6..b48ae962dd639f 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->default_callback_data(), + env->as_callback_data(), DEFAULT, None, SideEffectType::kHasNoSideEffect) diff --git a/src/node_stat_watcher.cc b/src/node_stat_watcher.cc index a35c247b6a16e8..70903525baa735 100644 --- a/src/node_stat_watcher.cc +++ b/src/node_stat_watcher.cc @@ -96,7 +96,7 @@ void StatWatcher::Callback(uv_fs_poll_t* handle, void StatWatcher::New(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); fs::BindingData* binding_data = - BindingDataBase::Unwrap(args); + Environment::GetBindingData(args); new StatWatcher(binding_data, args.This(), args[0]->IsTrue()); } diff --git a/src/node_v8.cc b/src/node_v8.cc index 03bbdd83750b1e..1cabab59c4761d 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -86,10 +86,10 @@ static const size_t kHeapCodeStatisticsPropertiesCount = HEAP_CODE_STATISTICS_PROPERTIES(V); #undef V -class BindingData : public BindingDataBase { +class BindingData : public BaseObject { public: BindingData(Environment* env, Local obj) - : BindingDataBase(env, obj), + : BaseObject(env, obj), heap_statistics_buffer(env->isolate(), kHeapStatisticsPropertiesCount), heap_space_statistics_buffer(env->isolate(), kHeapSpaceStatisticsPropertiesCount), @@ -121,7 +121,7 @@ void CachedDataVersionTag(const FunctionCallbackInfo& args) { } void UpdateHeapStatisticsBuffer(const FunctionCallbackInfo& args) { - BindingData* data = BindingDataBase::Unwrap(args); + BindingData* data = Environment::GetBindingData(args); HeapStatistics s; args.GetIsolate()->GetHeapStatistics(&s); AliasedFloat64Array& buffer = data->heap_statistics_buffer; @@ -132,7 +132,7 @@ void UpdateHeapStatisticsBuffer(const FunctionCallbackInfo& args) { void UpdateHeapSpaceStatisticsBuffer(const FunctionCallbackInfo& args) { - BindingData* data = BindingDataBase::Unwrap(args); + BindingData* data = Environment::GetBindingData(args); HeapSpaceStatistics s; Isolate* const isolate = args.GetIsolate(); CHECK(args[0]->IsUint32()); @@ -147,7 +147,7 @@ void UpdateHeapSpaceStatisticsBuffer(const FunctionCallbackInfo& args) { } void UpdateHeapCodeStatisticsBuffer(const FunctionCallbackInfo& args) { - BindingData* data = BindingDataBase::Unwrap(args); + BindingData* data = Environment::GetBindingData(args); HeapCodeStatistics s; args.GetIsolate()->GetHeapCodeAndMetadataStatistics(&s); AliasedFloat64Array& buffer = data->heap_code_statistics_buffer; @@ -170,7 +170,7 @@ void Initialize(Local target, Local context, void* priv) { Environment* env = Environment::GetCurrent(context); - Environment::BindingScope binding_scope(env, context, target); + Environment::BindingScope binding_scope(context, target); if (!binding_scope) return; BindingData* binding_data = binding_scope.data; From 2ff3173570242c1114d2db1c0641ef1cd50cb536 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 6 May 2020 01:12:56 +0200 Subject: [PATCH 3/5] fixup! squash! src: retrieve binding data from the context --- src/README.md | 21 +++++++++------ src/env-inl.h | 50 +++++++++-------------------------- src/env.cc | 15 ----------- src/env.h | 39 +++++++-------------------- src/fs_event_wrap.cc | 2 +- src/node.cc | 2 +- src/node_crypto.cc | 4 +-- src/node_file.cc | 9 ++++--- src/node_file.h | 2 ++ src/node_http2.cc | 8 +++--- src/node_http2_state.h | 2 ++ src/node_http_parser.cc | 10 +++++-- src/node_native_module_env.cc | 2 +- src/node_process_object.cc | 4 +-- src/node_v8.cc | 10 ++++--- src/stream_wrap.cc | 2 +- src/tls_wrap.cc | 2 +- src/udp_wrap.cc | 2 +- src/util-inl.h | 31 ++++++++++++++++++++++ src/util.h | 21 +++++++++++++++ 20 files changed, 127 insertions(+), 111 deletions(-) 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 From 0c4bd7d79b66ce148722a4bddff0c68cfb1c838b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 6 May 2020 02:32:32 +0200 Subject: [PATCH 4/5] fixup! fixup! squash! src: retrieve binding data from the context --- src/env-inl.h | 1 + src/util-inl.h | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 8103cb7682fdc3..b8a783a174281c 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -369,6 +369,7 @@ inline T* Environment::AddBindingData( DCHECK_NOT_NULL(list); auto result = list->emplace(T::binding_data_name, item); CHECK(result.second); + DCHECK_EQ(GetBindingData(context), item.get()); return item.get(); } diff --git a/src/util-inl.h b/src/util-inl.h index 898b2eb4c55684..6b4bdfe034d64f 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -552,8 +552,8 @@ constexpr bool FastStringKey::operator==(const FastStringKey& other) const { const char* p2 = other.name_; if (p1 == p2) return true; do { - if (*p1 != *p2) return false; - } while (*(p1++) != '\0'); + if (*(p1++) != *(p2++)) return false; + } while (*p1 != '\0'); return true; } From f878946190259dde854b3cd234791471882ee3f3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 6 May 2020 05:22:52 +0200 Subject: [PATCH 5/5] fixup! fixup! fixup! squash! src: retrieve binding data from the context --- src/README.md | 3 ++- src/env-inl.h | 14 +++++++------- src/node.cc | 3 +-- src/node_env_var.cc | 6 ++---- src/node_process.h | 3 +-- 5 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/README.md b/src/README.md index 7ccc47cb0689e4..f940da3fe2475d 100644 --- a/src/README.md +++ b/src/README.md @@ -406,7 +406,8 @@ 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. +and which could e.g. match the binding’s name (in the example above, that would +be `cares_wrap`). ```c++ // In the HTTP parser source code file: diff --git a/src/env-inl.h b/src/env-inl.h index b8a783a174281c..853ba78de6d548 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -344,12 +344,12 @@ inline T* Environment::GetBindingData( template inline T* Environment::GetBindingData(v8::Local context) { - BindingDataStore* list = static_cast( + BindingDataStore* map = static_cast( context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingListIndex)); - DCHECK_NOT_NULL(list); - auto it = list->find(T::binding_data_name); - DCHECK_NE(it, list->end()); + DCHECK_NOT_NULL(map); + auto it = map->find(T::binding_data_name); + if (UNLIKELY(it == map->end())) return nullptr; T* result = static_cast(it->second.get()); DCHECK_NOT_NULL(result); DCHECK_EQ(result->env(), GetCurrent(context)); @@ -363,11 +363,11 @@ inline T* Environment::AddBindingData( DCHECK_EQ(GetCurrent(context), this); // This won't compile if T is not a BaseObject subclass. BaseObjectPtr item = MakeDetachedBaseObject(this, target); - BindingDataStore* list = static_cast( + BindingDataStore* map = static_cast( context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kBindingListIndex)); - DCHECK_NOT_NULL(list); - auto result = list->emplace(T::binding_data_name, item); + DCHECK_NOT_NULL(map); + auto result = map->emplace(T::binding_data_name, item); CHECK(result.second); DCHECK_EQ(GetBindingData(context), item.get()); return item.get(); diff --git a/src/node.cc b/src/node.cc index c9c8be571ff138..240f1f2e3b00f3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -331,8 +331,7 @@ MaybeLocal Environment::BootstrapNode() { Local env_string = FIXED_ONE_BYTE_STRING(isolate_, "env"); Local env_var_proxy; - if (!CreateEnvVarProxy(context(), isolate_, Local()) - .ToLocal(&env_var_proxy) || + if (!CreateEnvVarProxy(context(), isolate_).ToLocal(&env_var_proxy) || process_object()->Set(context(), env_string, env_var_proxy).IsNothing()) { return MaybeLocal(); } diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 6e388caeccbb52..23eaad48586130 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -377,13 +377,11 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { env->env_vars()->Enumerate(env->isolate())); } -MaybeLocal CreateEnvVarProxy(Local context, - Isolate* isolate, - Local data) { +MaybeLocal CreateEnvVarProxy(Local context, Isolate* isolate) { EscapableHandleScope scope(isolate); Local env_proxy_template = ObjectTemplate::New(isolate); env_proxy_template->SetHandler(NamedPropertyHandlerConfiguration( - EnvGetter, EnvSetter, EnvQuery, EnvDeleter, EnvEnumerator, data, + EnvGetter, EnvSetter, EnvQuery, EnvDeleter, EnvEnumerator, Local(), PropertyHandlerFlags::kHasNoSideEffect)); return scope.EscapeMaybe(env_proxy_template->NewInstance(context)); } diff --git a/src/node_process.h b/src/node_process.h index 4c262beef34363..2e87385348d936 100644 --- a/src/node_process.h +++ b/src/node_process.h @@ -8,8 +8,7 @@ namespace node { v8::MaybeLocal CreateEnvVarProxy(v8::Local context, - v8::Isolate* isolate, - v8::Local data); + v8::Isolate* isolate); // Most of the time, it's best to use `console.error` to write // to the process.stderr stream. However, in some cases, such as