Skip to content

Commit

Permalink
src: cache the result of GetOptions() in JS land
Browse files Browse the repository at this point in the history
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: #24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
  • Loading branch information
joyeecheung authored and BridgeAR committed Nov 13, 2018
1 parent 1225a0a commit a9053c3
Show file tree
Hide file tree
Showing 15 changed files with 49 additions and 44 deletions.
4 changes: 2 additions & 2 deletions lib/crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ const {
ERR_CRYPTO_FIPS_UNAVAILABLE
} = require('internal/errors').codes;
const constants = internalBinding('constants').crypto;
const { getOptions } = internalBinding('options');
const pendingDeprecation = getOptions('--pending-deprecation');
const { getOptionValue } = require('internal/options');
const pendingDeprecation = getOptionValue('--pending-deprecation');
const {
fipsMode,
fipsForced
Expand Down
3 changes: 1 addition & 2 deletions lib/internal/bash_completion.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
'use strict';
const { getOptions } = internalBinding('options');
const { options, aliases } = require('internal/options');

function print(stream) {
const { options, aliases } = getOptions();
const all_opts = [...options.keys(), ...aliases.keys()];

stream.write(`_node_complete() {
Expand Down
3 changes: 1 addition & 2 deletions lib/internal/bootstrap/loaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,7 @@

NativeModule.isInternal = function(id) {
return id.startsWith('internal/') ||
(id === 'worker_threads' &&
!internalBinding('options').getOptions('--experimental-worker'));
(id === 'worker_threads' && !config.experimentalWorker);
};
}

Expand Down
16 changes: 8 additions & 8 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,13 @@
NativeModule.require('internal/inspector_async_hook').setup();
}

const { getOptions } = internalBinding('options');
const helpOption = getOptions('--help');
const completionBashOption = getOptions('--completion-bash');
const experimentalModulesOption = getOptions('--experimental-modules');
const experimentalVMModulesOption = getOptions('--experimental-vm-modules');
const experimentalWorkerOption = getOptions('--experimental-worker');
const { getOptionValue } = NativeModule.require('internal/options');
const helpOption = getOptionValue('--help');
const completionBashOption = getOptionValue('--completion-bash');
const experimentalModulesOption = getOptionValue('--experimental-modules');
const experimentalVMModulesOption =
getOptionValue('--experimental-vm-modules');
const experimentalWorkerOption = getOptionValue('--experimental-worker');
if (helpOption) {
NativeModule.require('internal/print_help').print(process.stdout);
return;
Expand Down Expand Up @@ -721,10 +722,9 @@

const get = () => {
const {
getOptions,
envSettings: { kAllowedInEnvironment }
} = internalBinding('options');
const { options, aliases } = getOptions();
const { options, aliases } = NativeModule.require('internal/options');

const allowedNodeEnvironmentFlags = [];
for (const [name, info] of options) {
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/cjs/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const {
CHAR_HASH,
} = require('internal/constants');

const { getOptions } = internalBinding('options');
const { getOptionValue } = require('internal/options');

// Invoke with makeRequireFunction(module) where |module| is the Module object
// to use as the context for the require() function.
Expand Down Expand Up @@ -107,7 +107,7 @@ const builtinLibs = [
'v8', 'vm', 'zlib'
];

if (getOptions('--experimental-worker')) {
if (getOptionValue('--experimental-worker')) {
builtinLibs.push('worker_threads');
builtinLibs.sort();
}
Expand Down
8 changes: 4 additions & 4 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ const {
stripBOM,
stripShebang
} = require('internal/modules/cjs/helpers');
const options = internalBinding('options');
const preserveSymlinks = options.getOptions('--preserve-symlinks');
const preserveSymlinksMain = options.getOptions('--preserve-symlinks-main');
const experimentalModules = options.getOptions('--experimental-modules');
const { getOptionValue } = require('internal/options');
const preserveSymlinks = getOptionValue('--preserve-symlinks');
const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
const experimentalModules = getOptionValue('--experimental-modules');

const {
ERR_INVALID_ARG_TYPE,
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/modules/esm/default_resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ const internalFS = require('internal/fs/utils');
const { NativeModule } = require('internal/bootstrap/loaders');
const { extname } = require('path');
const { realpathSync } = require('fs');
const { getOptions } = internalBinding('options');
const preserveSymlinks = getOptions('--preserve-symlinks');
const preserveSymlinksMain = getOptions('--preserve-symlinks-main');
const { getOptionValue } = require('internal/options');
const preserveSymlinks = getOptionValue('--preserve-symlinks');
const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main');
const {
ERR_MISSING_MODULE,
ERR_MODULE_RESOLUTION_LEGACY,
Expand Down
18 changes: 18 additions & 0 deletions lib/internal/options.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';

const { getOptions } = internalBinding('options');
const { options, aliases } = getOptions();

function getOptionValue(option) {
const result = options.get(option);
if (!result) {
return undefined;
}
return result.value;
}

module.exports = {
options,
aliases,
getOptionValue
};
5 changes: 3 additions & 2 deletions lib/internal/print_help.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const { getOptions, types } = internalBinding('options');

const { types } = internalBinding('options');

const typeLookup = [];
for (const key of Object.keys(types))
Expand Down Expand Up @@ -132,7 +133,7 @@ function format({ options, aliases = new Map(), firstColumn, secondColumn }) {
}

function print(stream) {
const { options, aliases } = getOptions();
const { options, aliases } = require('internal/options');

// Use 75 % of the available width, and at least 70 characters.
const width = Math.max(70, (stream.columns || 0) * 0.75);
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/process/esm_loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ exports.ESMLoader = undefined;
exports.setup = function() {
let ESMLoader = new Loader();
const loaderPromise = (async () => {
const userLoader = internalBinding('options').getOptions('--loader');
const userLoader = require('internal/options').getOptionValue('--loader');
if (userLoader) {
const hooks = await ESMLoader.import(
userLoader, pathToFileURL(`${process.cwd()}/`).href);
Expand Down
2 changes: 1 addition & 1 deletion lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ const {
ERR_SCRIPT_EXECUTION_INTERRUPTED
} = require('internal/errors').codes;
const { sendInspectorCommand } = require('internal/util/inspector');
const experimentalREPLAwait = internalBinding('options').getOptions(
const experimentalREPLAwait = require('internal/options').getOptionValue(
'--experimental-repl-await'
);
const { isRecoverableError } = require('internal/repl/recoverable');
Expand Down
2 changes: 1 addition & 1 deletion lib/vm.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ module.exports = {
compileFunction,
};

if (internalBinding('options').getOptions('--experimental-vm-modules')) {
if (require('internal/options').getOptionValue('--experimental-vm-modules')) {
const { SourceTextModule } = require('internal/vm/source_text_module');
module.exports.SourceTextModule = SourceTextModule;
}
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@
'lib/internal/modules/esm/translators.js',
'lib/internal/safe_globals.js',
'lib/internal/net.js',
'lib/internal/options.js',
'lib/internal/print_help.js',
'lib/internal/priority_queue.js',
'lib/internal/process/esm_loader.js',
Expand Down
17 changes: 2 additions & 15 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -365,9 +365,8 @@ HostPort SplitHostPort(const std::string& arg,
ParseAndValidatePort(arg.substr(colon + 1), errors) };
}

// Usage: Either:
// - getOptions() to get all options + metadata or
// - getOptions(string) to get the value of a particular option
// Return a map containing all the options and their metadata as well
// as the aliases
void GetOptions(const FunctionCallbackInfo<Value>& args) {
Mutex::ScopedLock lock(per_process_opts_mutex);
Environment* env = Environment::GetCurrent(args);
Expand All @@ -388,13 +387,8 @@ void GetOptions(const FunctionCallbackInfo<Value>& args) {

const auto& parser = PerProcessOptionsParser::instance;

std::string filter;
if (args[0]->IsString()) filter = *node::Utf8Value(isolate, args[0]);

Local<Map> options = Map::New(isolate);
for (const auto& item : parser.options_) {
if (!filter.empty() && item.first != filter) continue;

Local<Value> value;
const auto& option_info = item.second;
auto field = option_info.field;
Expand Down Expand Up @@ -443,11 +437,6 @@ void GetOptions(const FunctionCallbackInfo<Value>& args) {
}
CHECK(!value.IsEmpty());

if (!filter.empty()) {
args.GetReturnValue().Set(value);
return;
}

Local<Value> name = ToV8Value(context, item.first).ToLocalChecked();
Local<Object> info = Object::New(isolate);
Local<Value> help_text;
Expand All @@ -469,8 +458,6 @@ void GetOptions(const FunctionCallbackInfo<Value>& args) {
}
}

if (!filter.empty()) return;

Local<Value> aliases;
if (!ToV8Value(context, parser.aliases_).ToLocal(&aliases)) return;

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-bootstrap-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ const list = process.moduleLoadList.slice();

const assert = require('assert');

assert(list.length <= 77, list);
assert(list.length <= 78, list);

0 comments on commit a9053c3

Please sign in to comment.