From 66265f131d257b79fae2a91b60f513f48f866267 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 7 Jan 2016 21:56:10 +0100 Subject: [PATCH 1/4] fs: change statSync to accessSync in realpathSync fs.statSync() creates and returns a heavy-weight fs.Stat object whereas fs.accessSync() simply returns nothing. The return value is ignored, the call is for its side effect of throwing an ELOOP error in case of cyclic symbolic links. --- lib/fs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fs.js b/lib/fs.js index fbb2f47b0f438e..bebd11cb821d9d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1550,7 +1550,7 @@ fs.realpathSync = function realpathSync(p, cache) { } } if (linkTarget === null) { - fs.statSync(base); + fs.accessSync(base, fs.F_OK); // Throws ELOOP on cyclic links. linkTarget = fs.readlinkSync(base); } resolvedLink = pathModule.resolve(previous, linkTarget); From d67de9b966910066f924303e1235755058dae67e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 7 Jan 2016 22:36:30 +0100 Subject: [PATCH 2/4] module: cache stat() results more aggressively Reduce the number of stat() system calls that require() makes by caching the results more aggressively. To avoid unbounded growth without implementing a LRU cache, scope the cache to the lifetime of the first call to require(). Recursive calls (i.e. require() calls in the included code) transparently profit from the cache. The benchmarked application is the loopback-sample-app[0] and it sees the number of stat calls at start-up go down by 40%, from 4736 to 2810. [0] https://github.com/strongloop/loopback-sample-app --- lib/internal/module.js | 11 +++++++-- lib/module.js | 26 ++++++++++++++++++---- test/fixtures/module-require-depth/one.js | 9 ++++++++ test/fixtures/module-require-depth/two.js | 9 ++++++++ test/parallel/test-module-require-depth.js | 13 +++++++++++ 5 files changed, 62 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/module-require-depth/one.js create mode 100644 test/fixtures/module-require-depth/two.js create mode 100644 test/parallel/test-module-require-depth.js diff --git a/lib/internal/module.js b/lib/internal/module.js index ef55aa64bd5642..29f88999dbf72f 100644 --- a/lib/internal/module.js +++ b/lib/internal/module.js @@ -1,6 +1,8 @@ 'use strict'; -module.exports = { makeRequireFunction, stripBOM }; +exports = module.exports = { makeRequireFunction, stripBOM }; + +exports.requireDepth = 0; // Invoke with makeRequireFunction.call(module) where |module| is the // Module object to use as the context for the require() function. @@ -9,7 +11,12 @@ function makeRequireFunction() { const self = this; function require(path) { - return self.require(path); + try { + exports.requireDepth += 1; + return self.require(path); + } finally { + exports.requireDepth -= 1; + } } require.resolve = function(request) { diff --git a/lib/module.js b/lib/module.js index 82b1971e8bf009..0ee40005528648 100644 --- a/lib/module.js +++ b/lib/module.js @@ -33,6 +33,20 @@ function tryWrapper(wrapper, opts) { } +function stat(filename) { + filename = path._makeLong(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); + return result; +} +stat.cache = null; + + function Module(id, parent) { this.id = id; this.exports = {}; @@ -114,7 +128,7 @@ Module._realpathCache = {}; // check if the file exists and is not a directory function tryFile(requestPath) { - const rc = internalModuleStat(path._makeLong(requestPath)); + const rc = stat(requestPath); return rc === 0 && toRealPath(requestPath); } @@ -152,12 +166,12 @@ Module._findPath = function(request, paths) { // For each path for (var i = 0, PL = paths.length; i < PL; i++) { // Don't search further if path doesn't exist - if (paths[i] && internalModuleStat(path._makeLong(paths[i])) < 1) continue; + if (paths[i] && stat(paths[i]) < 1) continue; var basePath = path.resolve(paths[i], request); var filename; if (!trailingSlash) { - const rc = internalModuleStat(path._makeLong(basePath)); + const rc = stat(basePath); if (rc === 0) { // File. filename = toRealPath(basePath); } else if (rc === 1) { // Directory. @@ -405,7 +419,11 @@ Module.prototype._compile = function(content, filename) { const dirname = path.dirname(filename); const require = internalModule.makeRequireFunction.call(this); const args = [this.exports, require, this, filename, dirname]; - return compiledWrapper.apply(this.exports, args); + const depth = internalModule.requireDepth; + if (depth === 0) stat.cache = new Map(); + const result = compiledWrapper.apply(this.exports, args); + if (depth === 0) stat.cache = null; + return result; }; diff --git a/test/fixtures/module-require-depth/one.js b/test/fixtures/module-require-depth/one.js new file mode 100644 index 00000000000000..5927908b7540ab --- /dev/null +++ b/test/fixtures/module-require-depth/one.js @@ -0,0 +1,9 @@ +// Flags: --expose_internals +'use strict'; +const assert = require('assert'); +const internalModule = require('internal/module'); + +exports.requireDepth = internalModule.requireDepth; +assert.strictEqual(internalModule.requireDepth, 1); +assert.deepStrictEqual(require('./two'), { requireDepth: 2 }); +assert.strictEqual(internalModule.requireDepth, 1); diff --git a/test/fixtures/module-require-depth/two.js b/test/fixtures/module-require-depth/two.js new file mode 100644 index 00000000000000..aea49947d1152d --- /dev/null +++ b/test/fixtures/module-require-depth/two.js @@ -0,0 +1,9 @@ +// Flags: --expose_internals +'use strict'; +const assert = require('assert'); +const internalModule = require('internal/module'); + +exports.requireDepth = internalModule.requireDepth; +assert.strictEqual(internalModule.requireDepth, 2); +assert.deepStrictEqual(require('./one'), { requireDepth: 1 }); +assert.strictEqual(internalModule.requireDepth, 2); diff --git a/test/parallel/test-module-require-depth.js b/test/parallel/test-module-require-depth.js new file mode 100644 index 00000000000000..4d2ddac151be68 --- /dev/null +++ b/test/parallel/test-module-require-depth.js @@ -0,0 +1,13 @@ +// Flags: --expose_internals +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const internalModule = require('internal/module'); + +// Module one loads two too so the expected depth for two is, well, two. +assert.strictEqual(internalModule.requireDepth, 0); +const one = require(common.fixturesDir + '/module-require-depth/one'); +const two = require(common.fixturesDir + '/module-require-depth/two'); +assert.deepStrictEqual(one, { requireDepth: 1 }); +assert.deepStrictEqual(two, { requireDepth: 2 }); +assert.strictEqual(internalModule.requireDepth, 0); From 359281b32902cff684000c2c90ecfb6190ae045d Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 7 Jan 2016 22:50:09 +0100 Subject: [PATCH 3/4] module: avoid ArgumentsAdaptorTrampoline frame Avoid an unneeded ArgumentsAdaptorTrampoline stack frame by passing the the right number of arguments to Module._load() in Module.require(). Shortens the following stack trace with one frame: LazyCompile:~Module.load module.js:345 LazyCompile:Module._load module.js:282 Builtin:ArgumentsAdaptorTrampoline LazyCompile:*Module.require module.js:361 LazyCompile:*require internal/module.js:11 --- lib/module.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/module.js b/lib/module.js index 0ee40005528648..064e8a1d9989dc 100644 --- a/lib/module.js +++ b/lib/module.js @@ -375,7 +375,7 @@ Module.prototype.load = function(filename) { Module.prototype.require = function(path) { assert(path, 'missing path'); assert(typeof path === 'string', 'path must be a string'); - return Module._load(path, this); + return Module._load(path, this, /* isMain */ false); }; From 2e672b688c492c6b587cf079aae3b22f6b9ad97b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 8 Jan 2016 10:40:32 +0100 Subject: [PATCH 4/4] module: optimize js and json file i/o Use internalModuleReadFile() to read the file from disk to avoid the fs.fstatSync() call that fs.readFileSync() makes. It does so to know the file size in advance so it doesn't have to allocate O(n) buffers when reading the file from disk. internalModuleReadFile() is plenty efficient though, even more so because we want a string and not a buffer. This way we also don't allocate a buffer that immediately gets thrown away again. This commit reduces the number of fstat() system calls in a benchmark application[0] from 549 to 29, all made by the application itself. [0] https://github.com/strongloop/loopback-sample-app --- lib/module.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/module.js b/lib/module.js index 064e8a1d9989dc..a639d78aa398e2 100644 --- a/lib/module.js +++ b/lib/module.js @@ -429,14 +429,14 @@ Module.prototype._compile = function(content, filename) { // Native extension for .js Module._extensions['.js'] = function(module, filename) { - var content = fs.readFileSync(filename, 'utf8'); + var content = internalModuleReadFile(filename); module._compile(internalModule.stripBOM(content), filename); }; // Native extension for .json Module._extensions['.json'] = function(module, filename) { - var content = fs.readFileSync(filename, 'utf8'); + var content = internalModuleReadFile(filename); try { module.exports = JSON.parse(internalModule.stripBOM(content)); } catch (err) {