modules: significantly improve repeated requires

1) It adds more benchmark options to properly verify the gains.

This makes sure the benchmark also tests requiring the same module
again instead of only loading each module only once.

2) Remove dead code:

The array check is obsolete as this function will only be called
internally with preprepared data which is always an array.

3) Simpler code

It was possible to use a more direct logic to prevent some branches.

4) Inline try catch

The function is not required anymore, since V8 is able to produce
performant code with it.

5) var -> let / const & less lines

6) Update require.extensions description

The comment was outdated.

7) Improve extension handling

This is a performance optimization to prevent loading the extensions
on each uncached require call. It uses proxies to intercept changes
and receives the necessary informations by doing that.

benchmark/module/module-loader.js

@@ -4,18 +4,20 @@ const path = require('path');
 const common = require('../common.js');
 
 const tmpdir = require('../../test/common/tmpdir');
-const benchmarkDirectory = path.join(tmpdir.path, 'nodejs-benchmark-module');
+let benchmarkDirectory = path.join(tmpdir.path, 'nodejs-benchmark-module');
 
 const bench = common.createBenchmark(main, {
-  n: [5e4],
-  fullPath: ['true', 'false'],
-  useCache: ['true', 'false']
+  name: ['', '/', '/index.js'],
+  dir: ['rel', 'abs'],
+  files: [5e2],
+  n: [1, 1e3],
+  cache: ['true', 'false']
 });
 
-function main({ n, fullPath, useCache }) {
+function main({ n, name, cache, files, dir }) {
   tmpdir.refresh();
-  try { fs.mkdirSync(benchmarkDirectory); } catch {}
-  for (var i = 0; i <= n; i++) {
+  fs.mkdirSync(benchmarkDirectory);
+  for (var i = 0; i <= files; i++) {
     fs.mkdirSync(`${benchmarkDirectory}${i}`);
     fs.writeFileSync(
       `${benchmarkDirectory}${i}/package.json`,
@@ -27,38 +29,25 @@ function main({ n, fullPath, useCache }) {
     );
   }
 
-  if (fullPath === 'true')
-    measureFull(n, useCache === 'true');
-  else
-    measureDir(n, useCache === 'true');
+  if (dir === 'rel')
+    benchmarkDirectory = path.relative(__dirname, benchmarkDirectory);
 
-  tmpdir.refresh();
-}
+  measureDir(n, cache === 'true', files, name);
 
-function measureFull(n, useCache) {
-  var i;
-  if (useCache) {
-    for (i = 0; i <= n; i++) {
-      require(`${benchmarkDirectory}${i}/index.js`);
-    }
-  }
-  bench.start();
-  for (i = 0; i <= n; i++) {
-    require(`${benchmarkDirectory}${i}/index.js`);
-  }
-  bench.end(n);
+  tmpdir.refresh();
 }
 
-function measureDir(n, useCache) {
+function measureDir(n, cache, files, name) {
   var i;
-  if (useCache) {
-    for (i = 0; i <= n; i++) {
-      require(`${benchmarkDirectory}${i}`);
+  if (cache) {
+    for (i = 0; i <= files; i++) {
+      require(`${benchmarkDirectory}${i}${name}`);
     }
   }
   bench.start();
-  for (i = 0; i <= n; i++) {
-    require(`${benchmarkDirectory}${i}`);
+  for (i = 0; i <= files; i++) {
+    for (var j = 0; j < n; j++)
+      require(`${benchmarkDirectory}${i}${name}`);
   }
-  bench.end(n);
+  bench.end(n * files);
 }

doc/api/deprecations.md

@@ -442,6 +442,9 @@ code.
 ### DEP0019: require('.') resolved outside directory
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/REPLACEME
+    description: Removed functionality.
   - version:
     - v4.8.6
     - v6.12.0
@@ -452,11 +455,10 @@ changes:
     description: Runtime deprecation.
 -->
 
-Type: Runtime
+Type: End-of-Life
 
-In certain cases, `require('.')` may resolve outside the package directory.
-This behavior is deprecated and will be removed in a future major Node.js
-release.
+In certain cases, `require('.')` could resolve outside the package directory.
+This behavior has been removed.
 
 <a id="DEP0020"></a>
 ### DEP0020: Server.connections

doc/api/modules.md

@@ -196,28 +196,26 @@ NODE_MODULES_PATHS(START)
 
 <!--type=misc-->
 
-Modules are cached after the first time they are loaded. This means
-(among other things) that every call to `require('foo')` will get
-exactly the same object returned, if it would resolve to the same file.
+Modules are cached after the first time they are loaded. This means (among other
+things) that every call to `require('foo')` will get exactly the same object
+returned, if it would resolve to the same file.
 
-Provided `require.cache` is not modified, multiple calls to
-`require('foo')` will not cause the module code to be executed multiple times.
-This is an important feature. With it, "partially done" objects can be returned,
-thus allowing transitive dependencies to be loaded even when they would cause
-cycles.
+Provided `require.cache` is not modified, multiple calls to `require('foo')`
+will not cause the module code to be executed multiple times. This is an
+important feature. With it, "partially done" objects can be returned, thus
+allowing transitive dependencies to be loaded even when they would cause cycles.
 
-To have a module execute code multiple times, export a function, and call
-that function.
+To have a module execute code multiple times, export a function, and call that
+function.
 
 ### Module Caching Caveats
 
 <!--type=misc-->
 
-Modules are cached based on their resolved filename. Since modules may
-resolve to a different filename based on the location of the calling
-module (loading from `node_modules` folders), it is not a *guarantee*
-that `require('foo')` will always return the exact same object, if it
-would resolve to different files.
+Modules are cached based on their resolved filename. Since modules may resolve
+to a different filename based on the location of the calling module (loading
+from `node_modules` folders), it is not a *guarantee* that `require('foo')` will
+always return the exact same object, if it would resolve to different files.
 
 Additionally, on case-insensitive file systems or operating systems, different
 resolved filenames can point to the same file, but the cache will still treat
@@ -412,7 +410,7 @@ are not found elsewhere.
 On Windows, `NODE_PATH` is delimited by semicolons (`;`) instead of colons.
 
 `NODE_PATH` was originally created to support loading modules from
-varying paths before the current [module resolution][] algorithm was frozen.
+varying paths before the current [module resolution][] algorithm was defined.
 
 `NODE_PATH` is still supported, but is less necessary now that the Node.js
 ecosystem has settled on a convention for locating dependent modules.
@@ -582,6 +580,10 @@ value from this object, the next `require` will reload the module. Note that
 this does not apply to [native addons][], for which reloading will result in an
 error.
 
+Adding ore replacing entries is also possible. This cache is checked before
+native modules and if such a name is added to the cache, no require call is
+going to receive the native module anymore. Use with care!
+
 #### require.extensions
 <!-- YAML
 added: v0.3.0
@@ -600,22 +602,15 @@ Process files with the extension `.sjs` as `.js`:
 require.extensions['.sjs'] = require.extensions['.js'];
 ```
 
-**Deprecated** In the past, this list has been used to load
-non-JavaScript modules into Node.js by compiling them on-demand.
-However, in practice, there are much better ways to do this, such as
-loading modules via some other Node.js program, or compiling them to
-JavaScript ahead of time.
-
-Since the module system is locked, this feature will probably never go
-away. However, it may have subtle bugs and complexities that are best
-left untouched.
-
-Note that the number of file system operations that the module system
-has to perform in order to resolve a `require(...)` statement to a
-filename scales linearly with the number of registered extensions.
+**Deprecated** In the past, this list has been used to load non-JavaScript
+modules into Node.js by compiling them on-demand. However, in practice, there
+are much better ways to do this, such as loading modules via some other Node.js
+program, or compiling them to JavaScript ahead of time.
 
-In other words, adding extensions slows down the module loader and
-should be discouraged.
+Using this could cause subtle bugs and adding extensions slows down the module
+loader and is therefore discouraged (the number of file system operations that
+the module system has to perform in order to resolve a `require(...)` statement
+to a filename scales linearly with the number of registered extensions).
 
 #### require.main
 <!-- YAML

lib/internal/bootstrap/loaders.js

@@ -179,21 +179,33 @@ NativeModule._cache = {};
 
 const config = internalBinding('config');
 
+// Do not expose this to user land even with --expose-internals.
+const loaderId = 'internal/bootstrap/loaders';
+
+// Create a native module map to tell if a module is internal or not.
+const moduleState = {};
+const moduleNames = Object.keys(source);
+for (const name of moduleNames) {
+  moduleState[name] = name === loaderId ||
+    !config.exposeInternals &&
+    (name.startsWith('internal/') ||
+      (name === 'worker_threads' && !config.experimentalWorker));
+}
+
 // Think of this as module.exports in this file even though it is not
 // written in CommonJS style.
 const loaderExports = { internalBinding, NativeModule };
-const loaderId = 'internal/bootstrap/loaders';
 
 NativeModule.require = function(id) {
-  if (id === loaderId) {
-    return loaderExports;
-  }
-
   const cached = NativeModule.getCached(id);
-  if (cached && (cached.loaded || cached.loading)) {
+  if (cached !== undefined && (cached.loaded || cached.loading)) {
     return cached.exports;
   }
 
+  if (id === loaderId) {
+    return loaderExports;
+  }
+
   if (!NativeModule.exists(id)) {
     // Model the error off the internal/errors.js model, but
     // do not use that module given that it could actually be
@@ -231,32 +243,16 @@ NativeModule.getCached = function(id) {
 };
 
 NativeModule.exists = function(id) {
-  return NativeModule._source.hasOwnProperty(id);
+  return NativeModule._source[id] !== undefined;
 };
 
-if (config.exposeInternals) {
-  NativeModule.nonInternalExists = function(id) {
-    // Do not expose this to user land even with --expose-internals.
-    if (id === loaderId) {
-      return false;
-    }
-    return NativeModule.exists(id);
-  };
-
-  NativeModule.isInternal = function(id) {
-    // Do not expose this to user land even with --expose-internals.
-    return id === loaderId;
-  };
-} else {
-  NativeModule.nonInternalExists = function(id) {
-    return NativeModule.exists(id) && !NativeModule.isInternal(id);
-  };
+NativeModule.nonInternalExists = function(id) {
+  return NativeModule.exists(id) && !NativeModule.isInternal(id);
+};
 
-  NativeModule.isInternal = function(id) {
-    return id.startsWith('internal/') ||
-        (id === 'worker_threads' && !config.experimentalWorker);
-  };
-}
+NativeModule.isInternal = function(id) {
+  return moduleState[id];
+};
 
 NativeModule.getSource = function(id) {
   return NativeModule._source[id];

lib/internal/modules/cjs/helpers.js

@@ -1,40 +1,53 @@
 'use strict';
 
 const { validateString } = require('internal/validators');
-
-const {
-  CHAR_LINE_FEED,
-  CHAR_CARRIAGE_RETURN,
-  CHAR_EXCLAMATION_MARK,
-  CHAR_HASH,
-} = require('internal/constants');
-
 const { getOptionValue } = require('internal/options');
+const {
+  ERR_INVALID_ARG_TYPE,
+  ERR_OUT_OF_RANGE
+} = require('internal/errors').codes;
+const { isAbsolute } = require('path');
 
 // Invoke with makeRequireFunction(module) where |module| is the Module object
 // to use as the context for the require() function.
 function makeRequireFunction(mod) {
   const Module = mod.constructor;
 
   function require(path) {
-    try {
-      exports.requireDepth += 1;
-      return mod.require(path);
-    } finally {
-      exports.requireDepth -= 1;
-    }
+    return mod.require(path);
   }
 
   function resolve(request, options) {
     validateString(request, 'request');
-    return Module._resolveFilename(request, mod, false, options);
+    if (options !== undefined && options.paths !== undefined) {
+      if (!Array.isArray(options.paths)) {
+        throw new ERR_INVALID_ARG_TYPE('options.paths', 'Array', options.paths);
+      }
+      if (options.paths.length === 0) {
+        throw new ERR_OUT_OF_RANGE('options.paths.length', '> 0',
+                                   options.paths.length);
+      }
+      if (!isAbsolute(request) &&
+          (request.charAt(0) !== '.' ||
+           (request.length > 1 &&
+            request.charAt(1) !== '.' &&
+            request.charAt(1) !== '/' &&
+            (process.platform !== 'win32' || request.charAt(1) !== '\\')))) {
+        const paths = new Set();
+        options.paths.forEach((path) =>
+          Module._nodeModulePaths(path).forEach((modPath) =>
+            paths.add(modPath)));
+        return Module._resolveFilename(request, mod, false, [...paths]);
+      }
+    }
+    return Module._resolveFilename(request, mod, false);
   }
 
   require.resolve = resolve;
 
   function paths(request) {
     validateString(request, 'request');
-    return Module._resolveLookupPaths(request, mod, true);
+    return Module._resolveLookupPaths(request, mod);
   }
 
   resolve.paths = paths;
@@ -66,31 +79,17 @@ function stripBOM(content) {
  */
 function stripShebang(content) {
   // Remove shebang
-  var contLen = content.length;
-  if (contLen >= 2) {
-    if (content.charCodeAt(0) === CHAR_HASH &&
-        content.charCodeAt(1) === CHAR_EXCLAMATION_MARK) {
-      if (contLen === 2) {
-        // Exact match
-        content = '';
-      } else {
-        // Find end of shebang line and slice it off
-        var i = 2;
-        for (; i < contLen; ++i) {
-          var code = content.charCodeAt(i);
-          if (code === CHAR_LINE_FEED || code === CHAR_CARRIAGE_RETURN)
-            break;
-        }
-        if (i === contLen)
-          content = '';
-        else {
-          // Note that this actually includes the newline character(s) in the
-          // new output. This duplicates the behavior of the regular expression
-          // that was previously used to replace the shebang line
-          content = content.slice(i);
-        }
-      }
-    }
+  if (content.charAt(0) === '#' && content.charAt(1) === '!') {
+    // Find end of shebang line and slice it off
+    let index = content.indexOf('\n', 2);
+    if (index === -1)
+      return '';
+    if (content.charAt(index - 1) === '\r')
+      index--;
+    // Note that this actually includes the newline character(s) in the
+    // new output. This duplicates the behavior of the regular expression
+    // that was previously used to replace the shebang line
+    content = content.slice(index);
   }
   return content;
 }