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 e2b256fba20..7666d15ab18 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 cab967f10db..967902d8781 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 f966bfba1ee..8e026733237 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 e2dc5e866f4..c6d47afbd73 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 592d0ca2a39..fdd84c39a20 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 807ba56c7c6..cae88c32cbb 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 8eaf79538a2..6e388caeccb 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 d30d7730100..f53bebbd744 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 203ec5c8ca5..b96e64c8e9d 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 1fda81361fe..36fcc240a79 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 6637166e954..4368ac61282 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 80efb40cd27..e1e825a802f 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 f98e9f5c7b6..9b161c45aa7 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 b48ae962dd6..9ec5ddf9a84 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 ad86b449f95..4c262beef34 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 af23affc412..a35c247b6a1 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 7174d9ae7f8..03bbdd83750 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 7666d15ab18..d29c03b8c18 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 967902d8781..1c5acd940f3 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 8e026733237..b2d2d825c56 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 c6d47afbd73..d050f966a12 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 cae88c32cbb..912e65b4270 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 f53bebbd744..2ad8c73f05e 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 b96e64c8e9d..7f1c9d1d97f 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 36fcc240a79..90e71efd3ad 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 4368ac61282..a6670155ce5 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 e1e825a802f..7ebcaa03ca5 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 9b161c45aa7..3abdd32e73d 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 9ec5ddf9a84..b48ae962dd6 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 a35c247b6a1..70903525baa 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 03bbdd83750..1cabab59c47 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 d29c03b8c18..7ccc47cb068 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 1c5acd940f3..8103cb7682f 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 b2d2d825c56..3efa5c3b9c9 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 d050f966a12..146754d03ab 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 b4b110cc2c8..9b6c553a630 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 649ac43fbe7..c9c8be571ff 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 afdb2e3c270..d53a6d2f232 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 7f1c9d1d97f..be18b18fddd 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 90e71efd3ad..7690f272848 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 a6670155ce5..25f353bb147 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 7ebcaa03ca5..2e9d3e5d6bc 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 3abdd32e73d..c7a3df8d067 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 b48ae962dd6..be647b01c64 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 ee7d771c717..ca17da9583e 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 1cabab59c47..047ca594095 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 90b2bb93416..bd396110fcc 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 df6a989dfbe..0c2c2bbc014 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 1c42481f178..9013bc9fe33 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 d44ee09fefb..898b2eb4c55 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 5eaa20b7601..979091e6d6e 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 8103cb7682f..b8a783a1742 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 898b2eb4c55..6b4bdfe034d 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 7ccc47cb068..f940da3fe24 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 b8a783a1742..853ba78de6d 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 c9c8be571ff..240f1f2e3b0 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 6e388caeccb..23eaad48586 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 4c262beef34..2e87385348d 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