Skip to content

Commit

Permalink
src: reduce unnecessary serialization of CLI options in C++
Browse files Browse the repository at this point in the history
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: #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>
  • Loading branch information
joyeecheung authored and targos committed May 8, 2024
1 parent fb24c44 commit dc41c13
Show file tree
Hide file tree
Showing 6 changed files with 183 additions and 139 deletions.
7 changes: 5 additions & 2 deletions lib/internal/main/print_help.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down
52 changes: 16 additions & 36 deletions lib/internal/options.js
Original file line number Diff line number Diff line change
@@ -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() {
Expand All @@ -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() {
Expand All @@ -76,12 +61,7 @@ function getAllowUnauthorized() {
}

module.exports = {
get options() {
return getCLIOptionsFromBinding();
},
get aliases() {
return getAliasesFromBinding();
},
getCLIOptionsInfo: getCLIOptionsInfoFromBinding,
getOptionValue,
getAllowUnauthorized,
getEmbedderOptions,
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/process/per_thread.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit dc41c13

Please sign in to comment.