From dd295c4f84a1a315a6db74026864172049a3a37b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 10 Apr 2024 18:26:24 +0200 Subject: [PATCH] src: reduce unnecessary serialization of CLI options in C++ In this patch we split the serialization routine into two different routines: `getCLIOptionsValues()` for only serializing the key-value pairs and `getCLIOptionsInfo()` for getting additional information such as help text etc. The former is used a lot more frequently than the latter, which is only used for generating `--help` and building `process.allowedNodeEnvironmentFlags`. `getCLIOptionsValues()` also adds `--no-` entries for boolean options so there is no need to special case in the JS land. This patch also refactors the option serialization routines to use v8::Object constructor that takes key-value pairs in one go to avoid calling Map::Set or Object::Set repeatedly which can go up to a patched prototype. --- lib/internal/main/print_help.js | 7 +- lib/internal/options.js | 52 ++---- lib/internal/process/per_thread.js | 3 +- src/node_options.cc | 239 ++++++++++++++++---------- src/node_options.h | 5 +- test/parallel/test-options-binding.js | 16 +- 6 files changed, 183 insertions(+), 139 deletions(-) diff --git a/lib/internal/main/print_help.js b/lib/internal/main/print_help.js index 0ae4d3496609ae..de1ab43ce756b0 100644 --- a/lib/internal/main/print_help.js +++ b/lib/internal/main/print_help.js @@ -24,6 +24,8 @@ const { markBootstrapComplete, } = require('internal/process/pre_execution'); +const { getCLIOptionsInfo, getOptionValue } = require('internal/options'); + const typeLookup = []; for (const key of ObjectKeys(types)) typeLookup[types[key]] = key; @@ -134,9 +136,10 @@ function format( ); for (const { - 0: name, 1: { helpText, type, value, defaultIsTrue }, + 0: name, 1: { helpText, type, defaultIsTrue }, } of sortedOptions) { if (!helpText) continue; + const value = getOptionValue(name); let displayName = name; @@ -198,7 +201,7 @@ function format( } function print(stream) { - const { options, aliases } = require('internal/options'); + const { options, aliases } = getCLIOptionsInfo(); // Use 75 % of the available width, and at least 70 characters. const width = MathMax(70, (stream.columns || 0) * 0.75); diff --git a/lib/internal/options.js b/lib/internal/options.js index 9a9dd2000bd95b..68acdb2a7b378e 100644 --- a/lib/internal/options.js +++ b/lib/internal/options.js @@ -1,36 +1,33 @@ 'use strict'; const { - StringPrototypeSlice, -} = primordials; - -const { - getCLIOptions, + getCLIOptionsValues, + getCLIOptionsInfo, getEmbedderOptions: getEmbedderOptionsFromBinding, } = internalBinding('options'); let warnOnAllowUnauthorized = true; -let optionsMap; -let aliasesMap; +let optionsDict; +let cliInfo; let embedderOptions; -// getCLIOptions() would serialize the option values from C++ land. +// getCLIOptionsValues() would serialize the option values from C++ land. // It would error if the values are queried before bootstrap is // complete so that we don't accidentally include runtime-dependent // states into a runtime-independent snapshot. function getCLIOptionsFromBinding() { - if (!optionsMap) { - ({ options: optionsMap } = getCLIOptions()); + if (!optionsDict) { + optionsDict = getCLIOptionsValues(); } - return optionsMap; + return optionsDict; } -function getAliasesFromBinding() { - if (!aliasesMap) { - ({ aliases: aliasesMap } = getCLIOptions()); +function getCLIOptionsInfoFromBinding() { + if (!cliInfo) { + cliInfo = getCLIOptionsInfo(); } - return aliasesMap; + return cliInfo; } function getEmbedderOptions() { @@ -41,24 +38,12 @@ function getEmbedderOptions() { } function refreshOptions() { - optionsMap = undefined; - aliasesMap = undefined; + optionsDict = undefined; } function getOptionValue(optionName) { - const options = getCLIOptionsFromBinding(); - if ( - optionName.length > 5 && - optionName[0] === '-' && - optionName[1] === '-' && - optionName[2] === 'n' && - optionName[3] === 'o' && - optionName[4] === '-' - ) { - const option = options.get('--' + StringPrototypeSlice(optionName, 5)); - return option && !option.value; - } - return options.get(optionName)?.value; + const optionsDict = getCLIOptionsFromBinding(); + return optionsDict[optionName]; } function getAllowUnauthorized() { @@ -76,12 +61,7 @@ function getAllowUnauthorized() { } module.exports = { - get options() { - return getCLIOptionsFromBinding(); - }, - get aliases() { - return getAliasesFromBinding(); - }, + getCLIOptionsInfo: getCLIOptionsInfoFromBinding, getOptionValue, getAllowUnauthorized, getEmbedderOptions, diff --git a/lib/internal/process/per_thread.js b/lib/internal/process/per_thread.js index 9b820ffe71aebe..408fb1b944d2a8 100644 --- a/lib/internal/process/per_thread.js +++ b/lib/internal/process/per_thread.js @@ -284,7 +284,8 @@ function buildAllowedFlags() { envSettings: { kAllowedInEnvvar }, types: { kBoolean }, } = internalBinding('options'); - const { options, aliases } = require('internal/options'); + const { getCLIOptionsInfo } = require('internal/options'); + const { options, aliases } = getCLIOptionsInfo(); const allowedNodeEnvironmentFlags = []; for (const { 0: name, 1: info } of options) { diff --git a/src/node_options.cc b/src/node_options.cc index 146812c89c74c2..aeec5eb507bc63 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -11,6 +11,7 @@ #endif #include +#include #include #include #include @@ -23,11 +24,12 @@ using v8::Integer; using v8::Isolate; using v8::Local; using v8::Map; +using v8::Name; +using v8::Null; using v8::Number; using v8::Object; using v8::Undefined; using v8::Value; - namespace node { namespace per_process { @@ -1161,11 +1163,32 @@ std::string GetBashCompletion() { return out.str(); } +struct IterateCLIOptionsScope { + IterateCLIOptionsScope(Environment* env) { + // Temporarily act as if the current Environment's/IsolateData's options + // were the default options, i.e. like they are the ones we'd access for + // global options parsing, so that all options are available from the main + // parser. + original_per_isolate = per_process::cli_options->per_isolate; + per_process::cli_options->per_isolate = env->isolate_data()->options(); + original_per_env = per_process::cli_options->per_isolate->per_env; + per_process::cli_options->per_isolate->per_env = env->options(); + } + ~IterateCLIOptionsScope() { + per_process::cli_options->per_isolate->per_env = original_per_env; + per_process::cli_options->per_isolate = original_per_isolate; + } + std::shared_ptr original_per_env; + std::shared_ptr original_per_isolate; +}; + // Return a map containing all the options and their metadata as well // as the aliases -void GetCLIOptions(const FunctionCallbackInfo& args) { - Mutex::ScopedLock lock(per_process::cli_options_mutex); - Environment* env = Environment::GetCurrent(args); +void GetCLIOptionsValues(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + Environment* env = Environment::GetCurrent(context); + if (!env->has_run_bootstrapping_code()) { // No code because this is an assertion. return env->ThrowError( @@ -1173,49 +1196,49 @@ void GetCLIOptions(const FunctionCallbackInfo& args) { } env->set_has_serialized_options(true); - Isolate* isolate = env->isolate(); - Local context = env->context(); - - // Temporarily act as if the current Environment's/IsolateData's options were - // the default options, i.e. like they are the ones we'd access for global - // options parsing, so that all options are available from the main parser. - auto original_per_isolate = per_process::cli_options->per_isolate; - per_process::cli_options->per_isolate = env->isolate_data()->options(); - auto original_per_env = per_process::cli_options->per_isolate->per_env; - per_process::cli_options->per_isolate->per_env = env->options(); - auto on_scope_leave = OnScopeLeave([&]() { - per_process::cli_options->per_isolate->per_env = original_per_env; - per_process::cli_options->per_isolate = original_per_isolate; - }); + Mutex::ScopedLock lock(per_process::cli_options_mutex); + IterateCLIOptionsScope s(env); - Local options = Map::New(isolate); - if (options - ->SetPrototype(context, env->primordials_safe_map_prototype_object()) - .IsNothing()) { - return; - } + std::vector> option_names; + std::vector> option_values; + option_names.reserve(_ppop_instance.options_.size() * 2); + option_values.reserve(_ppop_instance.options_.size() * 2); + + Local undefined_value = Undefined(isolate); + Local null_value = Null(isolate); for (const auto& item : _ppop_instance.options_) { Local value; const auto& option_info = item.second; auto field = option_info.field; PerProcessOptions* opts = per_process::cli_options.get(); + switch (option_info.type) { case kNoOp: case kV8Option: // Special case for --abort-on-uncaught-exception which is also // respected by Node.js internals if (item.first == "--abort-on-uncaught-exception") { - value = Boolean::New( - isolate, original_per_env->abort_on_uncaught_exception); + value = Boolean::New(isolate, + s.original_per_env->abort_on_uncaught_exception); } else { - value = Undefined(isolate); + value = undefined_value; } break; - case kBoolean: - value = Boolean::New(isolate, - *_ppop_instance.Lookup(field, opts)); + case kBoolean: { + bool original_value = *_ppop_instance.Lookup(field, opts); + value = Boolean::New(isolate, original_value); + + // Add --no-* entries. + std::string negated_name = + "--no" + item.first.substr(1, item.first.size()); + Local negated_value = Boolean::New(isolate, !original_value); + Local negated_name_v8 = + ToV8Value(context, negated_name).ToLocalChecked().As(); + option_names.push_back(negated_name_v8); + option_values.push_back(negated_value); break; + } case kInteger: value = Number::New( isolate, @@ -1242,46 +1265,86 @@ void GetCLIOptions(const FunctionCallbackInfo& args) { break; case kHostPort: { const HostPort& host_port = - *_ppop_instance.Lookup(field, opts); - Local obj = Object::New(isolate); + *_ppop_instance.Lookup(field, opts); Local host; - if (!ToV8Value(context, host_port.host()).ToLocal(&host) || - obj->Set(context, env->host_string(), host).IsNothing() || - obj->Set(context, - env->port_string(), - Integer::New(isolate, host_port.port())) - .IsNothing()) { + if (!ToV8Value(context, host_port.host()).ToLocal(&host)) { return; } - value = obj; + constexpr size_t kHostPortLength = 2; + std::array, kHostPortLength> names = {env->host_string(), + env->port_string()}; + std::array, kHostPortLength> values = { + host, Integer::New(isolate, host_port.port())}; + value = Object::New( + isolate, null_value, names.data(), values.data(), kHostPortLength); break; } default: UNREACHABLE(); } CHECK(!value.IsEmpty()); + Local name = + ToV8Value(context, item.first).ToLocalChecked().As(); + option_names.push_back(name); + option_values.push_back(value); + } + + Local options = Object::New(isolate, + null_value, + option_names.data(), + option_values.data(), + option_values.size()); + args.GetReturnValue().Set(options); +} + +void GetCLIOptionsInfo(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + Local context = isolate->GetCurrentContext(); + Environment* env = Environment::GetCurrent(context); + + if (!env->has_run_bootstrapping_code()) { + // No code because this is an assertion. + return env->ThrowError( + "Should not query options before bootstrapping is done"); + } + + Mutex::ScopedLock lock(per_process::cli_options_mutex); + IterateCLIOptionsScope s(env); + + Local options = Map::New(isolate); + if (options + ->SetPrototype(context, env->primordials_safe_map_prototype_object()) + .IsNothing()) { + return; + } - Local name = ToV8Value(context, item.first).ToLocalChecked(); - Local info = Object::New(isolate); + Local null_value = Null(isolate); + for (const auto& item : _ppop_instance.options_) { + const auto& option_info = item.second; + auto field = option_info.field; + + Local name = + ToV8Value(context, item.first).ToLocalChecked().As(); Local help_text; - if (!ToV8Value(context, option_info.help_text).ToLocal(&help_text) || - !info->Set(context, env->help_text_string(), help_text) - .FromMaybe(false) || - !info->Set(context, - env->env_var_settings_string(), - Integer::New(isolate, - static_cast(option_info.env_setting))) - .FromMaybe(false) || - !info->Set(context, - env->type_string(), - Integer::New(isolate, static_cast(option_info.type))) - .FromMaybe(false) || - !info->Set(context, - env->default_is_true_string(), - Boolean::New(isolate, option_info.default_is_true)) - .FromMaybe(false) || - info->Set(context, env->value_string(), value).IsNothing() || - options->Set(context, name, info).IsEmpty()) { + if (!ToV8Value(context, option_info.help_text).ToLocal(&help_text)) { + return; + } + constexpr size_t kInfoSize = 4; + std::array, kInfoSize> names = { + env->help_text_string(), + env->env_var_settings_string(), + env->type_string(), + env->default_is_true_string(), + }; + std::array, kInfoSize> values = { + help_text, + Integer::New(isolate, static_cast(option_info.env_setting)), + Integer::New(isolate, static_cast(option_info.type)), + Boolean::New(isolate, option_info.default_is_true), + }; + Local info = Object::New( + isolate, null_value, names.data(), values.data(), kInfoSize); + if (options->Set(context, name, info).IsEmpty()) { return; } } @@ -1295,12 +1358,12 @@ void GetCLIOptions(const FunctionCallbackInfo& args) { return; } - Local ret = Object::New(isolate); - if (ret->Set(context, env->options_string(), options).IsNothing() || - ret->Set(context, env->aliases_string(), aliases).IsNothing()) { - return; - } - + constexpr size_t kRetLength = 2; + std::array, kRetLength> names = {env->options_string(), + env->aliases_string()}; + std::array, kRetLength> values = {options, aliases}; + Local ret = + Object::New(isolate, null_value, names.data(), values.data(), kRetLength); args.GetReturnValue().Set(ret); } @@ -1312,30 +1375,22 @@ void GetEmbedderOptions(const FunctionCallbackInfo& args) { "Should not query options before bootstrapping is done"); } Isolate* isolate = args.GetIsolate(); - Local context = env->context(); - Local ret = Object::New(isolate); - - if (ret->Set(context, - FIXED_ONE_BYTE_STRING(env->isolate(), "shouldNotRegisterESMLoader"), - Boolean::New(isolate, env->should_not_register_esm_loader())) - .IsNothing()) return; - - if (ret->Set(context, - FIXED_ONE_BYTE_STRING(env->isolate(), "noGlobalSearchPaths"), - Boolean::New(isolate, env->no_global_search_paths())) - .IsNothing()) return; - - if (ret->Set(context, - FIXED_ONE_BYTE_STRING(env->isolate(), "noBrowserGlobals"), - Boolean::New(isolate, env->no_browser_globals())) - .IsNothing()) - return; - if (ret->Set(context, - FIXED_ONE_BYTE_STRING(env->isolate(), "hasEmbedderPreload"), - Boolean::New(isolate, env->embedder_preload() != nullptr)) - .IsNothing()) - return; + constexpr size_t kOptionsSize = 4; + std::array, kOptionsSize> names = { + FIXED_ONE_BYTE_STRING(env->isolate(), "shouldNotRegisterESMLoader"), + FIXED_ONE_BYTE_STRING(env->isolate(), "noGlobalSearchPaths"), + FIXED_ONE_BYTE_STRING(env->isolate(), "noBrowserGlobals"), + FIXED_ONE_BYTE_STRING(env->isolate(), "hasEmbedderPreload")}; + + std::array, kOptionsSize> values = { + Boolean::New(isolate, env->should_not_register_esm_loader()), + Boolean::New(isolate, env->no_global_search_paths()), + Boolean::New(isolate, env->no_browser_globals()), + Boolean::New(isolate, env->embedder_preload() != nullptr)}; + + Local ret = Object::New( + isolate, Null(isolate), names.data(), values.data(), kOptionsSize); args.GetReturnValue().Set(ret); } @@ -1346,7 +1401,10 @@ void Initialize(Local target, void* priv) { Environment* env = Environment::GetCurrent(context); Isolate* isolate = env->isolate(); - SetMethodNoSideEffect(context, target, "getCLIOptions", GetCLIOptions); + SetMethodNoSideEffect( + context, target, "getCLIOptionsValues", GetCLIOptionsValues); + SetMethodNoSideEffect( + context, target, "getCLIOptionsInfo", GetCLIOptionsInfo); SetMethodNoSideEffect( context, target, "getEmbedderOptions", GetEmbedderOptions); @@ -1372,7 +1430,8 @@ void Initialize(Local target, } void RegisterExternalReferences(ExternalReferenceRegistry* registry) { - registry->Register(GetCLIOptions); + registry->Register(GetCLIOptionsValues); + registry->Register(GetCLIOptionsInfo); registry->Register(GetEmbedderOptions); } } // namespace options_parser diff --git a/src/node_options.h b/src/node_options.h index cf744f13cd8f59..e105fe6330fee1 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -519,7 +519,10 @@ class OptionsParser { template friend class OptionsParser; - friend void GetCLIOptions(const v8::FunctionCallbackInfo& args); + friend void GetCLIOptionsValues( + const v8::FunctionCallbackInfo& args); + friend void GetCLIOptionsInfo( + const v8::FunctionCallbackInfo& args); friend std::string GetBashCompletion(); }; diff --git a/test/parallel/test-options-binding.js b/test/parallel/test-options-binding.js index b113a1ff66bcc9..5c910360d00fa4 100644 --- a/test/parallel/test-options-binding.js +++ b/test/parallel/test-options-binding.js @@ -2,17 +2,15 @@ 'use strict'; const common = require('../common'); -const { primordials: { SafeMap } } = require('internal/test/binding'); - -const { options, aliases, getOptionValue } = require('internal/options'); const assert = require('assert'); - -assert(options instanceof SafeMap, - "require('internal/options').options is a SafeMap"); - -assert(aliases instanceof SafeMap, - "require('internal/options').aliases is a SafeMap"); +const { getOptionValue } = require('internal/options'); Map.prototype.get = common.mustNotCall('`getOptionValue` must not call user-mutable method'); assert.strictEqual(getOptionValue('--expose-internals'), true); + +Object.prototype['--nonexistent-option'] = 'foo'; +assert.strictEqual(getOptionValue('--nonexistent-option'), undefined); + +// Make the test common global leak test happy. +delete Object.prototype['--nonexistent-option'];