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

joyeecheung · Trott · commit f895b5a58ec9 · 2018-11-07T20:40:38.000-08:00
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>
diff --git a/lib/crypto.js b/lib/crypto.js
@@ -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
diff --git a/lib/internal/bash_completion.js b/lib/internal/bash_completion.js
@@ -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() {
diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js
@@ -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);
     };
   }
 
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -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;
@@ -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) {
diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js
@@ -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.
@@ -107,7 +107,7 @@ const builtinLibs = [
   'v8', 'vm', 'zlib'
 ];
 
-if (getOptions('--experimental-worker')) {
+if (getOptionValue('--experimental-worker')) {
   builtinLibs.push('worker_threads');
   builtinLibs.sort();
 }
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -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,
diff --git a/lib/internal/modules/esm/default_resolve.js b/lib/internal/modules/esm/default_resolve.js
@@ -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,
diff --git a/lib/internal/options.js b/lib/internal/options.js
@@ -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
+};
diff --git a/lib/internal/print_help.js b/lib/internal/print_help.js
@@ -1,5 +1,6 @@
 'use strict';
-const { getOptions, types } = internalBinding('options');
+
+const { types } = internalBinding('options');
 
 const typeLookup = [];
 for (const key of Object.keys(types))
@@ -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);
diff --git a/lib/internal/process/esm_loader.js b/lib/internal/process/esm_loader.js
@@ -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);
diff --git a/lib/repl.js b/lib/repl.js
@@ -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');
diff --git a/lib/vm.js b/lib/vm.js
@@ -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;
 }
diff --git a/node.gyp b/node.gyp
@@ -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',
diff --git a/src/node_options.cc b/src/node_options.cc
@@ -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);
@@ -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;
@@ -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;
@@ -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;
 
diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js
@@ -11,4 +11,4 @@ const list = process.moduleLoadList.slice();
 
 const assert = require('assert');
 
-assert(list.length <= 77, list);
+assert(list.length <= 78, list);

Original file line number	Diff line number	Diff line change
`@@ -251,8 +251,7 @@`
`251`	`251`
`252`	`252`	`NativeModule.isInternal = function(id) {`
`253`	`253`	`return id.startsWith('internal/') \|\|`
`254`		`- (id === 'worker_threads' &&`
`255`		`- !internalBinding('options').getOptions('--experimental-worker'));`
	`254`	`+ (id === 'worker_threads' && !config.experimentalWorker);`
`256`	`255`	`};`
`257`	`256`	`}`
`258`	`257`