From c43011fbd1e110bfaab6ddc4316cc8bc5b027f5b Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Mon, 31 Dec 2018 02:01:00 +0100 Subject: [PATCH 1/5] module: simpler shebang function This simplifies the shebang function significantly. Before, it was optimized for two characters input. Any module actually parsed should however have more characters than just the shebang. The performance stays the same as before. --- lib/internal/modules/cjs/helpers.js | 43 ++++++++--------------------- 1 file changed, 11 insertions(+), 32 deletions(-) diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 2e5290b4520a27..234fcd5da20127 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -5,13 +5,6 @@ const path = require('path'); const { pathToFileURL } = require('internal/url'); const { URL } = require('url'); -const { - CHAR_LINE_FEED, - CHAR_CARRIAGE_RETURN, - CHAR_EXCLAMATION_MARK, - CHAR_HASH, -} = require('internal/constants'); - // Invoke with makeRequireFunction(module) where |module| is the Module object // to use as the context for the require() function. function makeRequireFunction(mod) { @@ -67,31 +60,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; } From 0b3c65f366ad95971ba550e194443bfc4b7ef269 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 22 Feb 2019 18:52:38 +0100 Subject: [PATCH 2/5] module: fix stat cache The current caching logic broke by [0] because it used destructuring on the module arguments. Since the exported property is a primitive counting it up or down would not have any effect anymore in the module that required that property. The original implementation would cache all stat calls caused during bootstrap. Afterwards it would clear the cache and lazy require calls during runtime would create a new cascading cache for the then loaded modules and clear the cache again. This behavior is now restored. This is difficult to test without exposing a lot of information and therfore the existing tests have been removed (as they could not detect the issue). With the broken implementation it caused each module compilation to reset the cache and therefore minimizing the effect drastically. [0] https://github.com/nodejs/node/pull/19177 --- lib/internal/modules/cjs/helpers.js | 8 +------ lib/internal/modules/cjs/loader.js | 27 +++++++++++----------- test/fixtures/module-require-depth/one.js | 11 --------- test/fixtures/module-require-depth/two.js | 11 --------- test/parallel/test-module-require-depth.js | 16 ------------- 5 files changed, 15 insertions(+), 58 deletions(-) delete mode 100644 test/fixtures/module-require-depth/one.js delete mode 100644 test/fixtures/module-require-depth/two.js delete mode 100644 test/parallel/test-module-require-depth.js diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 234fcd5da20127..5f45f5e3bb62f2 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -11,12 +11,7 @@ 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) { @@ -139,7 +134,6 @@ module.exports = exports = { builtinLibs, makeRequireFunction, normalizeReferrerURL, - requireDepth: 0, stripBOM, stripShebang }; diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index e22294f337698c..0f2c7d7d330b65 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -37,7 +37,6 @@ const { safeGetenv } = internalBinding('credentials'); const { makeRequireFunction, normalizeReferrerURL, - requireDepth, stripBOM, stripShebang } = require('internal/modules/cjs/helpers'); @@ -85,18 +84,17 @@ const { const isWindows = process.platform === 'win32'; +let requireDepth = 0; +let statCache = new Map(); function stat(filename) { filename = path.toNamespacedPath(filename); - const cache = stat.cache; - if (cache !== null) { - const result = cache.get(filename); - if (result !== undefined) return result; - } - const result = internalModuleStat(filename); - if (cache !== null) cache.set(filename, result); + if (statCache === null) statCache = new Map(); + let result = statCache.get(filename); + if (result !== undefined) return result; + result = internalModuleStat(filename); + statCache.set(filename, result); return result; } -stat.cache = null; function updateChildren(parent, child, scan) { var children = parent && parent.children; @@ -710,7 +708,12 @@ Module.prototype.require = function(id) { throw new ERR_INVALID_ARG_VALUE('id', id, 'must be a non-empty string'); } - return Module._load(id, this, /* isMain */ false); + requireDepth++; + try { + return Module._load(id, this, /* isMain */ false); + } finally { + requireDepth--; + } }; @@ -793,8 +796,6 @@ Module.prototype._compile = function(content, filename) { } var dirname = path.dirname(filename); var require = makeRequireFunction(this); - var depth = requireDepth; - if (depth === 0) stat.cache = new Map(); var result; var exports = this.exports; var thisValue = exports; @@ -806,7 +807,7 @@ Module.prototype._compile = function(content, filename) { result = compiledWrapper.call(thisValue, exports, require, module, filename, dirname); } - if (depth === 0) stat.cache = null; + if (requireDepth === 0) statCache = null; return result; }; diff --git a/test/fixtures/module-require-depth/one.js b/test/fixtures/module-require-depth/one.js deleted file mode 100644 index 02b451465bb76c..00000000000000 --- a/test/fixtures/module-require-depth/one.js +++ /dev/null @@ -1,11 +0,0 @@ -// Flags: --expose_internals -'use strict'; -const assert = require('assert'); -const { - requireDepth -} = require('internal/modules/cjs/helpers'); - -exports.requireDepth = requireDepth; -assert.strictEqual(requireDepth, 1); -assert.deepStrictEqual(require('./two'), { requireDepth: 2 }); -assert.strictEqual(requireDepth, 1); diff --git a/test/fixtures/module-require-depth/two.js b/test/fixtures/module-require-depth/two.js deleted file mode 100644 index 5c94c4c89aa9ef..00000000000000 --- a/test/fixtures/module-require-depth/two.js +++ /dev/null @@ -1,11 +0,0 @@ -// Flags: --expose_internals -'use strict'; -const assert = require('assert'); -const { - requireDepth -} = require('internal/modules/cjs/helpers'); - -exports.requireDepth = requireDepth; -assert.strictEqual(requireDepth, 2); -assert.deepStrictEqual(require('./one'), { requireDepth: 1 }); -assert.strictEqual(requireDepth, 2); diff --git a/test/parallel/test-module-require-depth.js b/test/parallel/test-module-require-depth.js deleted file mode 100644 index 0a3fc2826c71f4..00000000000000 --- a/test/parallel/test-module-require-depth.js +++ /dev/null @@ -1,16 +0,0 @@ -// Flags: --expose_internals -'use strict'; -require('../common'); -const fixtures = require('../common/fixtures'); -const assert = require('assert'); -const { - requireDepth -} = require('internal/modules/cjs/helpers'); - -// Module one loads two too so the expected depth for two is, well, two. -assert.strictEqual(requireDepth, 0); -const one = require(fixtures.path('module-require-depth', 'one')); -const two = require(fixtures.path('module-require-depth', 'two')); -assert.deepStrictEqual(one, { requireDepth: 1 }); -assert.deepStrictEqual(two, { requireDepth: 2 }); -assert.strictEqual(requireDepth, 0); From fdbf4724db823a1bda275fab30cb0a5e5978edfa Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 22 Feb 2019 22:24:24 +0100 Subject: [PATCH 3/5] fixup: add period at end of sentence Co-Authored-By: BridgeAR --- lib/internal/modules/cjs/helpers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index 5f45f5e3bb62f2..df727ff3c88a45 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -64,7 +64,7 @@ function stripShebang(content) { 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 + // that was previously used to replace the shebang line. content = content.slice(index); } return content; From 512210f113b24adf77d88fee248bb505d4bbb534 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 28 Feb 2019 18:36:11 +0100 Subject: [PATCH 4/5] fixup: address comment --- lib/internal/modules/cjs/loader.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 0f2c7d7d330b65..a7782bfae89483 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -88,7 +88,6 @@ let requireDepth = 0; let statCache = new Map(); function stat(filename) { filename = path.toNamespacedPath(filename); - if (statCache === null) statCache = new Map(); let result = statCache.get(filename); if (result !== undefined) return result; result = internalModuleStat(filename); @@ -800,6 +799,7 @@ Module.prototype._compile = function(content, filename) { var exports = this.exports; var thisValue = exports; var module = this; + if (statCache === null) statCache = new Map(); if (inspectorWrapper) { result = inspectorWrapper(compiledWrapper, thisValue, exports, require, module, filename, dirname); From e18e8be3f68671a6ad27e183b01e86e0432a59cc Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 28 Feb 2019 20:08:12 +0100 Subject: [PATCH 5/5] Revert "fixup: address comment" This reverts commit 512210f113b24adf77d88fee248bb505d4bbb534. --- lib/internal/modules/cjs/loader.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index a7782bfae89483..0f2c7d7d330b65 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -88,6 +88,7 @@ let requireDepth = 0; let statCache = new Map(); function stat(filename) { filename = path.toNamespacedPath(filename); + if (statCache === null) statCache = new Map(); let result = statCache.get(filename); if (result !== undefined) return result; result = internalModuleStat(filename); @@ -799,7 +800,6 @@ Module.prototype._compile = function(content, filename) { var exports = this.exports; var thisValue = exports; var module = this; - if (statCache === null) statCache = new Map(); if (inspectorWrapper) { result = inspectorWrapper(compiledWrapper, thisValue, exports, require, module, filename, dirname);