From 56db1ab792b95edb91aa60c5d6ea95a16e7eef02 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Wed, 24 Jan 2018 23:25:20 -0500 Subject: [PATCH 1/4] process: js fast path for cached bindings --- lib/internal/bootstrap_node.js | 44 +++++++++++++++++++++++++++------- src/node.cc | 37 +++++++++------------------- 2 files changed, 47 insertions(+), 34 deletions(-) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index b11f5901a1769c..09f9e6b3112b9b 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -21,9 +21,6 @@ setupProcessObject(); - internalBinding = process._internalBinding; - delete process._internalBinding; - // do this good and early, since it handles errors. setupProcessFatal(); @@ -246,13 +243,44 @@ perf.markMilestone(NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE); } + function pushValueToArray() { + for (var i = 0; i < arguments.length; i++) + this.push(arguments[i]); + } + + function setupProcessBinding(bindingObj) { + const _binding = process.binding; + + process.binding = function binding(module) { + if (!module || typeof module !== 'string') + return; + if (typeof bindingObj[module] === 'object') + return bindingObj[module]; + return _binding(module); + }; + } + + function setupInternalBinding(bindingObj) { + const _internalBinding = process._internalBinding; + delete process._internalBinding; + + internalBinding = function internalBinding(module) { + if (!module || typeof module !== 'string') + return; + if (typeof bindingObj[module] === 'object') + return bindingObj[module]; + return _internalBinding(module); + }; + } + function setupProcessObject() { - process._setupProcessObject(pushValueToArray); + const [ + bindingObj, + internalBindingObj + ] = process._setupProcessObject(pushValueToArray); - function pushValueToArray() { - for (var i = 0; i < arguments.length; i++) - this.push(arguments[i]); - } + setupProcessBinding(bindingObj); + setupInternalBinding(internalBindingObj); } function setupGlobalVariables() { diff --git a/src/node.cc b/src/node.cc index 656d132ec9fb32..e8935d71318bf3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -860,6 +860,13 @@ void SetupProcessObject(const FunctionCallbackInfo& args) { env->process_object()->Delete( env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "_setupProcessObject")).FromJust(); + + + Local ret = Array::New(env->isolate(), 2); + ret->Set(env->context(), 0, env->binding_cache_object()).FromJust(); + ret->Set(env->context(), 1, env->internal_binding_cache_object()).FromJust(); + + args.GetReturnValue().Set(ret); } @@ -2526,22 +2533,6 @@ Maybe ProcessEmitDeprecationWarning(Environment* env, } -static bool PullFromCache(Environment* env, - const FunctionCallbackInfo& args, - Local module, - Local cache) { - Local context = env->context(); - Local exports_v; - Local exports; - if (cache->Get(context, module).ToLocal(&exports_v) && - exports_v->IsObject() && - exports_v->ToObject(context).ToLocal(&exports)) { - args.GetReturnValue().Set(exports); - return true; - } - return false; -} - static Local InitModule(Environment* env, node_module* mod, Local module) { @@ -2569,14 +2560,11 @@ static void ThrowIfNoSuchModule(Environment* env, const char* module_v) { static void Binding(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Local module; - if (!args[0]->ToString(env->context()).ToLocal(&module)) return; + CHECK(args[0]->IsString()); + Local module = args[0].As(); Local cache = env->binding_cache_object(); - if (PullFromCache(env, args, module, cache)) - return; - // Append a string to process.moduleLoadList char buf[1024]; node::Utf8Value module_v(env->isolate(), module); @@ -2609,14 +2597,11 @@ static void Binding(const FunctionCallbackInfo& args) { static void InternalBinding(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Local module; - if (!args[0]->ToString(env->context()).ToLocal(&module)) return; + CHECK(args[0]->IsString()); + Local module = args[0].As(); Local cache = env->internal_binding_cache_object(); - if (PullFromCache(env, args, module, cache)) - return; - // Append a string to process.moduleLoadList char buf[1024]; node::Utf8Value module_v(env->isolate(), module); From 753a08149293ef6764fb317419d40efc7a799e2d Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 25 Jan 2018 08:48:43 -0500 Subject: [PATCH 2/4] fixup: move the full caching functionality to JS --- lib/internal/bootstrap_node.js | 47 +++++++++++++++++++--------------- src/env-inl.h | 10 -------- src/env.h | 2 -- src/node.cc | 21 ++------------- 4 files changed, 28 insertions(+), 52 deletions(-) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 09f9e6b3112b9b..5067e812462b54 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -243,44 +243,49 @@ perf.markMilestone(NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE); } - function pushValueToArray() { - for (var i = 0; i < arguments.length; i++) - this.push(arguments[i]); - } - - function setupProcessBinding(bindingObj) { - const _binding = process.binding; + { + const bindingObj = Object.create(null); + const getBinding = process.binding; process.binding = function binding(module) { - if (!module || typeof module !== 'string') - return; + module = String(module); + if (typeof bindingObj[module] === 'object') + return bindingObj[module]; + bindingObj[module] = getBinding(module); + return bindingObj[module]; + }; + + const getLinkedBinding = process._linkedBinding; + process._linkedBinding = function _linkedBinding(module) { + module = String(module); if (typeof bindingObj[module] === 'object') return bindingObj[module]; - return _binding(module); + bindingObj[module] = getLinkedBinding(module); + return bindingObj[module]; }; } - function setupInternalBinding(bindingObj) { - const _internalBinding = process._internalBinding; + { + const bindingObj = Object.create(null); + + const getInternalBinding = process._internalBinding; delete process._internalBinding; internalBinding = function internalBinding(module) { - if (!module || typeof module !== 'string') - return; if (typeof bindingObj[module] === 'object') return bindingObj[module]; - return _internalBinding(module); + bindingObj[module] = getInternalBinding(module); + return bindingObj[module]; }; } function setupProcessObject() { - const [ - bindingObj, - internalBindingObj - ] = process._setupProcessObject(pushValueToArray); + process._setupProcessObject(pushValueToArray); - setupProcessBinding(bindingObj); - setupInternalBinding(internalBindingObj); + function pushValueToArray() { + for (var i = 0; i < arguments.length; i++) + this.push(arguments[i]); + } } function setupGlobalVariables() { diff --git a/src/env-inl.h b/src/env-inl.h index 4a6e73cc38807e..3235626a8c0f32 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -329,16 +329,6 @@ inline Environment::Environment(IsolateData* isolate_data, v8::Context::Scope context_scope(context); set_as_external(v8::External::New(isolate(), this)); - v8::Local null = v8::Null(isolate()); - v8::Local binding_cache_object = v8::Object::New(isolate()); - CHECK(binding_cache_object->SetPrototype(context, null).FromJust()); - set_binding_cache_object(binding_cache_object); - - v8::Local internal_binding_cache_object = - v8::Object::New(isolate()); - CHECK(internal_binding_cache_object->SetPrototype(context, null).FromJust()); - set_internal_binding_cache_object(internal_binding_cache_object); - set_module_load_list_array(v8::Array::New(isolate())); AssignToContext(context, ContextInfo("")); diff --git a/src/env.h b/src/env.h index a1f505c4fe1da1..ffaaf64453d6a9 100644 --- a/src/env.h +++ b/src/env.h @@ -277,8 +277,6 @@ class ModuleWrap; V(async_hooks_after_function, v8::Function) \ V(async_hooks_promise_resolve_function, v8::Function) \ V(async_hooks_binding, v8::Object) \ - V(binding_cache_object, v8::Object) \ - V(internal_binding_cache_object, v8::Object) \ V(buffer_prototype_object, v8::Object) \ V(context, v8::Context) \ V(host_import_module_dynamically_callback, v8::Function) \ diff --git a/src/node.cc b/src/node.cc index e8935d71318bf3..7736217a768d27 100644 --- a/src/node.cc +++ b/src/node.cc @@ -860,13 +860,6 @@ void SetupProcessObject(const FunctionCallbackInfo& args) { env->process_object()->Delete( env->context(), FIXED_ONE_BYTE_STRING(env->isolate(), "_setupProcessObject")).FromJust(); - - - Local ret = Array::New(env->isolate(), 2); - ret->Set(env->context(), 0, env->binding_cache_object()).FromJust(); - ret->Set(env->context(), 1, env->internal_binding_cache_object()).FromJust(); - - args.GetReturnValue().Set(ret); } @@ -2563,7 +2556,6 @@ static void Binding(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); Local module = args[0].As(); - Local cache = env->binding_cache_object(); // Append a string to process.moduleLoadList char buf[1024]; @@ -2589,7 +2581,6 @@ static void Binding(const FunctionCallbackInfo& args) { } else { return ThrowIfNoSuchModule(env, *module_v); } - cache->Set(module, exports); args.GetReturnValue().Set(exports); } @@ -2600,7 +2591,6 @@ static void InternalBinding(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); Local module = args[0].As(); - Local cache = env->internal_binding_cache_object(); // Append a string to process.moduleLoadList char buf[1024]; @@ -2614,7 +2604,6 @@ static void InternalBinding(const FunctionCallbackInfo& args) { node_module* mod = get_internal_module(*module_v); if (mod == nullptr) return ThrowIfNoSuchModule(env, *module_v); Local exports = InitModule(env, mod, module); - cache->Set(module, exports); args.GetReturnValue().Set(exports); } @@ -2622,14 +2611,9 @@ static void InternalBinding(const FunctionCallbackInfo& args) { static void LinkedBinding(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args.GetIsolate()); - Local module_name; - if (!args[0]->ToString(env->context()).ToLocal(&module_name)) return; - - Local cache = env->binding_cache_object(); - Local exports_v = cache->Get(module_name); + CHECK(args[0]->IsString()); - if (exports_v->IsObject()) - return args.GetReturnValue().Set(exports_v.As()); + Local module_name = args[0].As(); node::Utf8Value module_name_v(env->isolate(), module_name); node_module* mod = get_linked_module(*module_name_v); @@ -2660,7 +2644,6 @@ static void LinkedBinding(const FunctionCallbackInfo& args) { } auto effective_exports = module->Get(exports_prop); - cache->Set(module_name, effective_exports); args.GetReturnValue().Set(effective_exports); } From b5fb9c1f9eb730f0dea2426a58436556fd7c7009 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 25 Jan 2018 10:03:38 -0500 Subject: [PATCH 3/4] fixup: bnoordhuis comment --- lib/internal/bootstrap_node.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 5067e812462b54..001f63f5edb67f 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -249,19 +249,19 @@ const getBinding = process.binding; process.binding = function binding(module) { module = String(module); - if (typeof bindingObj[module] === 'object') - return bindingObj[module]; - bindingObj[module] = getBinding(module); - return bindingObj[module]; + let mod = bindingObj[module]; + if (typeof mod !== 'object') + mod = bindingObj[module] = getBinding(module); + return mod; }; const getLinkedBinding = process._linkedBinding; process._linkedBinding = function _linkedBinding(module) { module = String(module); - if (typeof bindingObj[module] === 'object') - return bindingObj[module]; - bindingObj[module] = getLinkedBinding(module); - return bindingObj[module]; + let mod = bindingObj[module]; + if (typeof mod !== 'object') + mod = bindingObj[module] = getLinkedBinding(module); + return mod; }; } @@ -272,10 +272,10 @@ delete process._internalBinding; internalBinding = function internalBinding(module) { - if (typeof bindingObj[module] === 'object') - return bindingObj[module]; - bindingObj[module] = getInternalBinding(module); - return bindingObj[module]; + let mod = bindingObj[module]; + if (typeof mod !== 'object') + mod = bindingObj[module] = getInternalBinding(module); + return mod; }; } From ea60d2c198738a0762a34fbb51021e0e1715bc5f Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Thu, 25 Jan 2018 10:37:16 -0500 Subject: [PATCH 4/4] process: move moduleLoadList definition & usage to JS --- lib/internal/bootstrap_node.js | 18 +++++++++++++++--- src/env-inl.h | 2 -- src/env.h | 1 - src/node.cc | 21 --------------------- 4 files changed, 15 insertions(+), 27 deletions(-) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 001f63f5edb67f..f3f3264f353c24 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -243,6 +243,14 @@ perf.markMilestone(NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE); } + const moduleLoadList = []; + Object.defineProperty(process, 'moduleLoadList', { + value: moduleLoadList, + configurable: true, + enumerable: true, + writable: false + }); + { const bindingObj = Object.create(null); @@ -250,8 +258,10 @@ process.binding = function binding(module) { module = String(module); let mod = bindingObj[module]; - if (typeof mod !== 'object') + if (typeof mod !== 'object') { mod = bindingObj[module] = getBinding(module); + moduleLoadList.push(`Binding ${module}`); + } return mod; }; @@ -273,8 +283,10 @@ internalBinding = function internalBinding(module) { let mod = bindingObj[module]; - if (typeof mod !== 'object') + if (typeof mod !== 'object') { mod = bindingObj[module] = getInternalBinding(module); + moduleLoadList.push(`Internal Binding ${module}`); + } return mod; }; } @@ -583,7 +595,7 @@ throw err; } - process.moduleLoadList.push(`NativeModule ${id}`); + moduleLoadList.push(`NativeModule ${id}`); const nativeModule = new NativeModule(id); diff --git a/src/env-inl.h b/src/env-inl.h index 3235626a8c0f32..aea52ff38840f3 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -329,8 +329,6 @@ inline Environment::Environment(IsolateData* isolate_data, v8::Context::Scope context_scope(context); set_as_external(v8::External::New(isolate(), this)); - set_module_load_list_array(v8::Array::New(isolate())); - AssignToContext(context, ContextInfo("")); destroy_async_id_list_.reserve(512); diff --git a/src/env.h b/src/env.h index ffaaf64453d6a9..2dbedce1d06c51 100644 --- a/src/env.h +++ b/src/env.h @@ -285,7 +285,6 @@ class ModuleWrap; V(http2settings_constructor_template, v8::ObjectTemplate) \ V(immediate_callback_function, v8::Function) \ V(inspector_console_api_object, v8::Object) \ - V(module_load_list_array, v8::Array) \ V(pbkdf2_constructor_template, v8::ObjectTemplate) \ V(pipe_constructor_template, v8::FunctionTemplate) \ V(performance_entry_callback, v8::Function) \ diff --git a/src/node.cc b/src/node.cc index 7736217a768d27..7c66d67c4e2b9b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2556,15 +2556,7 @@ static void Binding(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); Local module = args[0].As(); - - // Append a string to process.moduleLoadList - char buf[1024]; node::Utf8Value module_v(env->isolate(), module); - snprintf(buf, sizeof(buf), "Binding %s", *module_v); - - Local modules = env->module_load_list_array(); - uint32_t l = modules->Length(); - modules->Set(l, OneByteString(env->isolate(), buf)); node_module* mod = get_builtin_module(*module_v); Local exports; @@ -2591,15 +2583,7 @@ static void InternalBinding(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); Local module = args[0].As(); - - // Append a string to process.moduleLoadList - char buf[1024]; node::Utf8Value module_v(env->isolate(), module); - snprintf(buf, sizeof(buf), "Internal Binding %s", *module_v); - - Local modules = env->module_load_list_array(); - uint32_t l = modules->Length(); - modules->Set(l, OneByteString(env->isolate(), buf)); node_module* mod = get_internal_module(*module_v); if (mod == nullptr) return ThrowIfNoSuchModule(env, *module_v); @@ -2971,11 +2955,6 @@ void SetupProcessObject(Environment* env, "version", FIXED_ONE_BYTE_STRING(env->isolate(), NODE_VERSION)); - // process.moduleLoadList - READONLY_PROPERTY(process, - "moduleLoadList", - env->module_load_list_array()); - // process.versions Local versions = Object::New(env->isolate()); READONLY_PROPERTY(process, "versions", versions);