Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: cache the result of GetOptions() in JS land #24091

Closed
wants to merge 2 commits into from
Closed

src: cache the result of GetOptions() in JS land #24091

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22446f1
src: cache the result of GetOptions() in JS land
joyeecheung Nov 4, 2018
977cb18
fixup! src: cache the result of GetOptions() in JS land
joyeecheung Nov 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/crypto.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,8 +211,7 @@

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid going back to process.config? 😕

Copy link
Member Author

@joyeecheung joyeecheung Nov 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also two other reference to config in here, I think a better option would probably be to just specialize something for NativeModule here - maybe just directly generate the whole map when the process starts and pass it to the loader bootstrapper here and let NativeModule short-circuit the internal/options implementation. That also opens up the opportunity to emit all the warning for experimental modules in one place here instead of emitting them everywhere across the code base.

EDIT: wait, we emit them regardless of the cli options..so never mind I guess

};
}

Expand Down
15 changes: 7 additions & 8 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,12 @@
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 @@ -764,10 +764,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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';
refack marked this conversation as resolved.
Show resolved Hide resolved

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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