Nodejs/node
1
0
Fork 0
mirror of https://github.com/nodejs/node.git synced 2024-11-21 10:59:27 +00:00
Code Issues Actions 25 Packages Projects Releases Wiki Activity

src: reduce unnecessary serialization of CLI options in C++

Browse Source 
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.

PR-URL: https://github.com/nodejs/node/pull/52451
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit is contained in:
Joyee Cheung 2024-05-03 00:57:03 +02:00 committed by GitHub
parent c5cfdd4849
commit a480e52b6e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 180 additions and 136 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
lib/internal/main/print_help.js
View File

@ -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);

52
lib/internal/options.js
View File

@ -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,

3
lib/internal/process/per_thread.js
View File

@ -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) {

233
src/node_options.cc
View File

@ -11,6 +11,7 @@
#endif
#include <algorithm>
#include <array>
#include <charconv>
#include <limits>
#include <sstream>
@ -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 {
@ -1154,11 +1156,32 @@ std::string GetBashCompletion() {
return out.str();
}
struct IterateCLIOptionsScope {
explicit 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<node::EnvironmentOptions> original_per_env;
std::shared_ptr<node::PerIsolateOptions> original_per_isolate;
};
// Return a map containing all the options and their metadata as well
// as the aliases
void GetCLIOptions(const FunctionCallbackInfo<Value>& args) {
Mutex::ScopedLock lock(per_process::cli_options_mutex);
Environment* env = Environment::GetCurrent(args);
void GetCLIOptionsValues(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
Local<Context> context = isolate->GetCurrentContext();
Environment* env = Environment::GetCurrent(context);
if (!env->has_run_bootstrapping_code()) {
// No code because this is an assertion.
return env->ThrowError(
@ -1166,49 +1189,49 @@ void GetCLIOptions(const FunctionCallbackInfo<Value>& args) {
}
env->set_has_serialized_options(true);
Isolate* isolate = env->isolate();
Local<Context> context = env->context();
Mutex::ScopedLock lock(per_process::cli_options_mutex);
IterateCLIOptionsScope s(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.
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;
});
std::vector<Local<Name>> option_names;
std::vector<Local<Value>> option_values;
option_names.reserve(_ppop_instance.options_.size() * 2);
option_values.reserve(_ppop_instance.options_.size() * 2);
Local<Map> options = Map::New(isolate);
if (options
->SetPrototype(context, env->primordials_safe_map_prototype_object())
.IsNothing()) {
return;
}
Local<Value> undefined_value = Undefined(isolate);
Local<Value> null_value = Null(isolate);
for (const auto& item : _ppop_instance.options_) {
Local<Value> 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<bool>(field, opts));
case kBoolean: {
bool original_value = *_ppop_instance.Lookup<bool>(field, opts);
value = Boolean::New(isolate, original_value);
// Add --no-* entries.
std::string negated_name =
"--no" + item.first.substr(1, item.first.size());
Local<Value> negated_value = Boolean::New(isolate, !original_value);
Local<Name> negated_name_v8 =
ToV8Value(context, negated_name).ToLocalChecked().As<Name>();
option_names.push_back(negated_name_v8);
option_values.push_back(negated_value);
break;
}
case kInteger:
value = Number::New(
isolate,
@ -1235,46 +1258,86 @@ void GetCLIOptions(const FunctionCallbackInfo<Value>& args) {
break;
case kHostPort: {
const HostPort& host_port =
*_ppop_instance.Lookup<HostPort>(field, opts);
Local<Object> obj = Object::New(isolate);
*_ppop_instance.Lookup<HostPort>(field, opts);
Local<Value> 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<Local<Name>, kHostPortLength> names = {env->host_string(),
env->port_string()};
std::array<Local<Value>, 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> name =
ToV8Value(context, item.first).ToLocalChecked().As<Name>();
option_names.push_back(name);
option_values.push_back(value);
}
Local<Value> name = ToV8Value(context, item.first).ToLocalChecked();
Local<Object> info = Object::New(isolate);
Local<Object> options = Object::New(isolate,
null_value,
option_names.data(),
option_values.data(),
option_values.size());
args.GetReturnValue().Set(options);
}
void GetCLIOptionsInfo(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
Local<Context> 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<Map> options = Map::New(isolate);
if (options
->SetPrototype(context, env->primordials_safe_map_prototype_object())
.IsNothing()) {
return;
}
Local<Value> null_value = Null(isolate);
for (const auto& item : _ppop_instance.options_) {
const auto& option_info = item.second;
auto field = option_info.field;
Local<Name> name =
ToV8Value(context, item.first).ToLocalChecked().As<Name>();
Local<Value> 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<int>(option_info.env_setting)))
.FromMaybe(false) ||
!info->Set(context,
env->type_string(),
Integer::New(isolate, static_cast<int>(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<Local<Name>, kInfoSize> names = {
env->help_text_string(),
env->env_var_settings_string(),
env->type_string(),
env->default_is_true_string(),
};
std::array<Local<Value>, kInfoSize> values = {
help_text,
Integer::New(isolate, static_cast<int>(option_info.env_setting)),
Integer::New(isolate, static_cast<int>(option_info.type)),
Boolean::New(isolate, option_info.default_is_true),
};
Local<Object> info = Object::New(
isolate, null_value, names.data(), values.data(), kInfoSize);
if (options->Set(context, name, info).IsEmpty()) {
return;
}
}
@ -1288,12 +1351,12 @@ void GetCLIOptions(const FunctionCallbackInfo<Value>& args) {
return;
}
Local<Object> 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<Local<Name>, kRetLength> names = {env->options_string(),
env->aliases_string()};
std::array<Local<Value>, kRetLength> values = {options, aliases};
Local<Value> ret =
Object::New(isolate, null_value, names.data(), values.data(), kRetLength);
args.GetReturnValue().Set(ret);
}
@ -1305,30 +1368,22 @@ void GetEmbedderOptions(const FunctionCallbackInfo<Value>& args) {
"Should not query options before bootstrapping is done");
}
Isolate* isolate = args.GetIsolate();
Local<Context> context = env->context();
Local<Object> 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;
constexpr size_t kOptionsSize = 4;
std::array<Local<Name>, 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")};
if (ret->Set(context,
FIXED_ONE_BYTE_STRING(env->isolate(), "noGlobalSearchPaths"),
Boolean::New(isolate, env->no_global_search_paths()))
.IsNothing()) return;
std::array<Local<Value>, 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)};
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;
Local<Object> ret = Object::New(
isolate, Null(isolate), names.data(), values.data(), kOptionsSize);
args.GetReturnValue().Set(ret);
}
@ -1339,7 +1394,10 @@ void Initialize(Local<Object> 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);
@ -1365,7 +1423,8 @@ void Initialize(Local<Object> target,
}
void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(GetCLIOptions);
registry->Register(GetCLIOptionsValues);
registry->Register(GetCLIOptionsInfo);
registry->Register(GetEmbedderOptions);
}
} // namespace options_parser

5
src/node_options.h
View File

@ -517,7 +517,10 @@ class OptionsParser {
template <typename OtherOptions>
friend class OptionsParser;
friend void GetCLIOptions(const v8::FunctionCallbackInfo<v8::Value>& args);
friend void GetCLIOptionsValues(
const v8::FunctionCallbackInfo<v8::Value>& args);
friend void GetCLIOptionsInfo(
const v8::FunctionCallbackInfo<v8::Value>& args);
friend std::string GetBashCompletion();
};

16
test/parallel/test-options-binding.js
View File

@ -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'];