From ae18bbef48d87d9c641df85369f62cfd5ed8c250 Mon Sep 17 00:00:00 2001 From: Brian White Date: Thu, 7 Apr 2016 10:04:47 -0400 Subject: [PATCH] lib: improve module loading performance This commit improves module loading performance by at least ~25-35% in the module-loader benchmarks. Some optimization strategies include: * Try-finally/try-catch isolation * Replacing regular expressions with manual parsing * Avoiding unnecessary string and array creation * Avoiding constant recompilation of anonymous functions and function definitions within functions PR-URL: https://github.com/nodejs/node/pull/5172 Reviewed-By: James M Snell --- lib/fs.js | 93 ++++--- lib/internal/bootstrap_node.js | 52 ++-- lib/internal/module.js | 6 +- lib/module.js | 233 +++++++++++++----- test/message/core_line_numbers.out | 2 +- test/message/error_exit.out | 1 + .../undefined_reference_in_new_context.out | 2 +- test/message/vm_display_runtime_error.out | 2 +- test/message/vm_display_syntax_error.out | 2 + .../message/vm_dont_display_runtime_error.out | 2 +- test/message/vm_dont_display_syntax_error.out | 1 + 11 files changed, 274 insertions(+), 122 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index bfb7d874f7d71f..acb2676bb4c196 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -450,6 +450,42 @@ function tryToString(buf, encoding, callback) { callback(e, buf); } +function tryStatSync(fd, isUserFd) { + var threw = true; + var st; + try { + st = fs.fstatSync(fd); + threw = false; + } finally { + if (threw && !isUserFd) fs.closeSync(fd); + } + return st; +} + +function tryCreateBuffer(size, fd, isUserFd) { + var threw = true; + var buffer; + try { + buffer = Buffer.allocUnsafe(size); + threw = false; + } finally { + if (threw && !isUserFd) fs.closeSync(fd); + } + return buffer; +} + +function tryReadSync(fd, isUserFd, buffer, pos, len) { + var threw = true; + var bytesRead; + try { + bytesRead = fs.readSync(fd, buffer, pos, len); + threw = false; + } finally { + if (threw && !isUserFd) fs.closeSync(fd); + } + return bytesRead; +} + fs.readFileSync = function(path, options) { if (!options) { options = { encoding: null, flag: 'r' }; @@ -466,17 +502,8 @@ fs.readFileSync = function(path, options) { var isUserFd = isFd(path); // file descriptor ownership var fd = isUserFd ? path : fs.openSync(path, flag, 0o666); - var st; - var size; - var threw = true; - try { - st = fs.fstatSync(fd); - size = st.isFile() ? st.size : 0; - threw = false; - } finally { - if (threw && !isUserFd) fs.closeSync(fd); - } - + var st = tryStatSync(fd, isUserFd); + var size = st.isFile() ? st.size : 0; var pos = 0; var buffer; // single buffer with file data var buffers; // list for when size is unknown @@ -484,39 +511,27 @@ fs.readFileSync = function(path, options) { if (size === 0) { buffers = []; } else { - threw = true; - try { - buffer = Buffer.allocUnsafe(size); - threw = false; - } finally { - if (threw && !isUserFd) fs.closeSync(fd); - } + buffer = tryCreateBuffer(size, fd, isUserFd); } - var done = false; var bytesRead; - while (!done) { - threw = true; - try { - if (size !== 0) { - bytesRead = fs.readSync(fd, buffer, pos, size - pos); - } else { - // the kernel lies about many files. - // Go ahead and try to read some bytes. - buffer = Buffer.allocUnsafe(8192); - bytesRead = fs.readSync(fd, buffer, 0, 8192); - if (bytesRead) { - buffers.push(buffer.slice(0, bytesRead)); - } + if (size !== 0) { + do { + bytesRead = tryReadSync(fd, isUserFd, buffer, pos, size - pos); + pos += bytesRead; + } while (bytesRead !== 0 && pos < size); + } else { + do { + // the kernel lies about many files. + // Go ahead and try to read some bytes. + buffer = Buffer.allocUnsafe(8192); + bytesRead = tryReadSync(fd, isUserFd, buffer, 0, 8192); + if (bytesRead !== 0) { + buffers.push(buffer.slice(0, bytesRead)); } - threw = false; - } finally { - if (threw && !isUserFd) fs.closeSync(fd); - } - - pos += bytesRead; - done = (bytesRead === 0) || (size !== 0 && pos >= size); + pos += bytesRead; + } while (bytesRead !== 0); } if (!isUserFd) diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js index 58d78b5490bc9a..2960234fa08e36 100644 --- a/lib/internal/bootstrap_node.js +++ b/lib/internal/bootstrap_node.js @@ -284,37 +284,45 @@ }; } - function evalScript(name) { - var Module = NativeModule.require('module'); - var path = NativeModule.require('path'); - + function tryGetCwd(path) { + var threw = true; + var cwd; try { - var cwd = process.cwd(); - } catch (e) { - // getcwd(3) can fail if the current working directory has been deleted. - // Fall back to the directory name of the (absolute) executable path. - // It's not really correct but what are the alternatives? - cwd = path.dirname(process.execPath); + cwd = process.cwd(); + threw = false; + } finally { + if (threw) { + // getcwd(3) can fail if the current working directory has been deleted. + // Fall back to the directory name of the (absolute) executable path. + // It's not really correct but what are the alternatives? + return path.dirname(process.execPath); + } } + return cwd; + } + + function evalScript(name) { + const Module = NativeModule.require('module'); + const path = NativeModule.require('path'); + const cwd = tryGetCwd(path); - var module = new Module(name); + const module = new Module(name); module.filename = path.join(cwd, name); module.paths = Module._nodeModulePaths(cwd); - var script = process._eval; - var body = script; - script = `global.__filename = ${JSON.stringify(name)};\n` + - 'global.exports = exports;\n' + - 'global.module = module;\n' + - 'global.__dirname = __dirname;\n' + - 'global.require = require;\n' + - 'return require("vm").runInThisContext(' + - `${JSON.stringify(body)}, { filename: ` + - `${JSON.stringify(name)}, displayErrors: true });\n`; + const body = process._eval; + const script = `global.__filename = ${JSON.stringify(name)};\n` + + 'global.exports = exports;\n' + + 'global.module = module;\n' + + 'global.__dirname = __dirname;\n' + + 'global.require = require;\n' + + 'return require("vm").runInThisContext(' + + `${JSON.stringify(body)}, { filename: ` + + `${JSON.stringify(name)}, displayErrors: true });\n`; // Defer evaluation for a tick. This is a workaround for deferred // events not firing when evaluating scripts from the command line, // see https://github.com/nodejs/node/issues/1600. process.nextTick(function() { - var result = module._compile(script, `${name}-wrapper`); + const result = module._compile(script, `${name}-wrapper`); if (process._print_eval) console.log(result); }); } diff --git a/lib/internal/module.js b/lib/internal/module.js index 29f88999dbf72f..9918c73626658a 100644 --- a/lib/internal/module.js +++ b/lib/internal/module.js @@ -19,9 +19,11 @@ function makeRequireFunction() { } } - require.resolve = function(request) { + function resolve(request) { return Module._resolveFilename(request, self); - }; + } + + require.resolve = resolve; require.main = process.mainModule; diff --git a/lib/module.js b/lib/module.js index b992ca8faf3add..8e5dc7a81828fb 100644 --- a/lib/module.js +++ b/lib/module.js @@ -11,10 +11,6 @@ const path = require('path'); const internalModuleReadFile = process.binding('fs').internalModuleReadFile; const internalModuleStat = process.binding('fs').internalModuleStat; -const splitRe = process.platform === 'win32' ? /[\/\\]/ : /\//; -const isIndexRe = /^index\.\w+?$/; -const shebangRe = /^\#\!.*/; - // If obj.hasOwnProperty has been overridden, then calling // obj.hasOwnProperty(prop) will break. // See: https://github.com/joyent/node/issues/1707 @@ -84,8 +80,8 @@ function readPackage(requestPath) { return packageMainCache[requestPath]; } - var jsonPath = path.resolve(requestPath, 'package.json'); - var json = internalModuleReadFile(path._makeLong(jsonPath)); + const jsonPath = path.resolve(requestPath, 'package.json'); + const json = internalModuleReadFile(path._makeLong(jsonPath)); if (json === undefined) { return false; @@ -107,7 +103,8 @@ function tryPackage(requestPath, exts) { if (!pkg) return false; var filename = path.resolve(requestPath, pkg); - return tryFile(filename) || tryExtensions(filename, exts) || + return tryFile(filename) || + tryExtensions(filename, exts) || tryExtensions(path.resolve(filename, 'index'), exts); } @@ -128,8 +125,8 @@ function toRealPath(requestPath) { // given a path check a the file exists with any of the set extensions function tryExtensions(p, exts) { - for (var i = 0, EL = exts.length; i < EL; i++) { - var filename = tryFile(p + exts[i]); + for (var i = 0; i < exts.length; i++) { + const filename = tryFile(p + exts[i]); if (filename) { return filename; @@ -142,21 +139,25 @@ var warned = false; Module._findPath = function(request, paths) { if (path.isAbsolute(request)) { paths = ['']; + } else if (!paths || paths.length === 0) { + return false; } - var cacheKey = JSON.stringify({request: request, paths: paths}); + const cacheKey = JSON.stringify({request: request, paths: paths}); if (Module._pathCache[cacheKey]) { return Module._pathCache[cacheKey]; } - const exts = Object.keys(Module._extensions); - const trailingSlash = request.slice(-1) === '/'; + var exts; + const trailingSlash = request.length > 0 && + request.charCodeAt(request.length - 1) === 47/*/*/; // For each path - for (var i = 0, PL = paths.length; i < PL; i++) { + for (var i = 0; i < paths.length; i++) { // Don't search further if path doesn't exist - if (paths[i] && stat(paths[i]) < 1) continue; - var basePath = path.resolve(paths[i], request); + const curPath = paths[i]; + if (curPath && stat(curPath) < 1) continue; + var basePath = path.resolve(curPath, request); var filename; if (!trailingSlash) { @@ -164,11 +165,15 @@ Module._findPath = function(request, paths) { if (rc === 0) { // File. filename = toRealPath(basePath); } else if (rc === 1) { // Directory. + if (exts === undefined) + exts = Object.keys(Module._extensions); filename = tryPackage(basePath, exts); } if (!filename) { // try it with each of the extensions + if (exts === undefined) + exts = Object.keys(Module._extensions); filename = tryExtensions(basePath, exts); } } @@ -179,6 +184,8 @@ Module._findPath = function(request, paths) { if (!filename) { // try it with each of the extensions at "index" + if (exts === undefined) + exts = Object.keys(Module._extensions); filename = tryExtensions(path.resolve(basePath, 'index'), exts); } @@ -198,48 +205,105 @@ Module._findPath = function(request, paths) { return false; }; -// 'from' is the __dirname of the module. -Module._nodeModulePaths = function(from) { - // guarantee that 'from' is absolute. - from = path.resolve(from); - - // note: this approach *only* works when the path is guaranteed - // to be absolute. Doing a fully-edge-case-correct path.split - // that works on both Windows and Posix is non-trivial. - var paths = []; - var parts = from.split(splitRe); - - for (var tip = parts.length - 1; tip >= 0; tip--) { - // don't search in .../node_modules/node_modules - if (parts[tip] === 'node_modules') continue; - var dir = parts.slice(0, tip + 1).concat('node_modules').join(path.sep); - paths.push(dir); - } +// 'node_modules' character codes reversed +var nmChars = [ 115, 101, 108, 117, 100, 111, 109, 95, 101, 100, 111, 110 ]; +var nmLen = nmChars.length; +if (process.platform === 'win32') { + // 'from' is the __dirname of the module. + Module._nodeModulePaths = function(from) { + // guarantee that 'from' is absolute. + from = path.resolve(from); + + // note: this approach *only* works when the path is guaranteed + // to be absolute. Doing a fully-edge-case-correct path.split + // that works on both Windows and Posix is non-trivial. + const paths = []; + var p = 0; + var last = from.length; + for (var i = from.length - 1; i >= 0; --i) { + const code = from.charCodeAt(i); + if (code === 92/*\*/ || code === 47/*/*/) { + if (p !== nmLen) + paths.push(from.slice(0, last) + '\\node_modules'); + last = i; + p = 0; + } else if (p !== -1 && p < nmLen) { + if (nmChars[p] === code) { + ++p; + } else { + p = -1; + } + } + } - return paths; -}; + return paths; + }; +} else { // posix + // 'from' is the __dirname of the module. + Module._nodeModulePaths = function(from) { + // guarantee that 'from' is absolute. + from = path.resolve(from); + // Return early not only to avoid unnecessary work, but to *avoid* returning + // an array of two items for a root: [ '//node_modules', '/node_modules' ] + if (from === '/') + return ['/node_modules']; + + // note: this approach *only* works when the path is guaranteed + // to be absolute. Doing a fully-edge-case-correct path.split + // that works on both Windows and Posix is non-trivial. + const paths = []; + var p = 0; + var last = from.length; + for (var i = from.length - 1; i >= 0; --i) { + const code = from.charCodeAt(i); + if (code === 47/*/*/) { + if (p !== nmLen) + paths.push(from.slice(0, last) + '/node_modules'); + last = i; + p = 0; + } else if (p !== -1 && p < nmLen) { + if (nmChars[p] === code) { + ++p; + } else { + p = -1; + } + } + } + + return paths; + }; +} +// 'index.' character codes +var indexChars = [ 105, 110, 100, 101, 120, 46 ]; +var indexLen = indexChars.length; Module._resolveLookupPaths = function(request, parent) { if (NativeModule.nonInternalExists(request)) { return [request, []]; } - var start = request.substring(0, 2); - if (start !== './' && start !== '..') { + var reqLen = request.length; + // Check for relative path + if (reqLen < 2 || + request.charCodeAt(0) !== 46/*.*/ || + (request.charCodeAt(1) !== 46/*.*/ && + request.charCodeAt(1) !== 47/*/*/)) { var paths = modulePaths; if (parent) { - if (!parent.paths) parent.paths = []; - paths = parent.paths.concat(paths); + if (!parent.paths) + paths = parent.paths = []; + else + paths = parent.paths.concat(paths); } // Maintain backwards compat with certain broken uses of require('.') // by putting the module's directory in front of the lookup paths. if (request === '.') { if (parent && parent.filename) { - paths.splice(0, 0, path.dirname(parent.filename)); + paths.unshift(path.dirname(parent.filename)); } else { - paths.splice(0, 0, path.resolve(request)); + paths.unshift(path.resolve(request)); } } @@ -257,8 +321,39 @@ Module._resolveLookupPaths = function(request, parent) { // Is the parent an index module? // We can assume the parent has a valid extension, // as it already has been accepted as a module. - var isIndex = isIndexRe.test(path.basename(parent.filename)); - var parentIdPath = isIndex ? parent.id : path.dirname(parent.id); + const base = path.basename(parent.filename); + var parentIdPath; + if (base.length > indexLen) { + var i = 0; + for (; i < indexLen; ++i) { + if (indexChars[i] !== base.charCodeAt(i)) + break; + } + if (i === indexLen) { + // We matched 'index.', let's validate the rest + for (; i < base.length; ++i) { + const code = base.charCodeAt(i); + if (code !== 95/*_*/ && + (code < 48/*0*/ || code > 57/*9*/) && + (code < 65/*A*/ || code > 90/*Z*/) && + (code < 97/*a*/ || code > 122/*z*/)) + break; + } + if (i === base.length) { + // Is an index module + parentIdPath = parent.id; + } else { + // Not an index module + parentIdPath = path.dirname(parent.id); + } + } else { + // Not an index module + parentIdPath = path.dirname(parent.id); + } + } else { + // Not an index module + parentIdPath = path.dirname(parent.id); + } var id = path.resolve(parentIdPath, request); // make sure require('./path') and require('path') get distinct ids, even @@ -307,19 +402,22 @@ Module._load = function(request, parent, isMain) { Module._cache[filename] = module; - var hadException = true; + tryModuleLoad(module, filename); + return module.exports; +}; + +function tryModuleLoad(module, filename) { + var threw = true; try { module.load(filename); - hadException = false; + threw = false; } finally { - if (hadException) { + if (threw) { delete Module._cache[filename]; } } - - return module.exports; -}; +} Module._resolveFilename = function(request, parent) { if (NativeModule.nonInternalExists(request)) { @@ -377,8 +475,33 @@ var resolvedArgv; // the file. // Returns exception, if any. Module.prototype._compile = function(content, filename) { - // remove shebang - content = content.replace(shebangRe, ''); + // Remove shebang + var contLen = content.length; + if (contLen >= 2) { + if (content.charCodeAt(0) === 35/*#*/ && + content.charCodeAt(1) === 33/*!*/) { + 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 === 10/*\n*/ || code === 13/*\r*/) + 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); + } + } + } + } // create wrapper function var wrapper = Module.wrap(content); @@ -408,12 +531,12 @@ Module.prototype._compile = function(content, filename) { global.v8debug.Debug.setBreakPoint(compiledWrapper, 0, 0); } } - const dirname = path.dirname(filename); - const require = internalModule.makeRequireFunction.call(this); - const args = [this.exports, require, this, filename, dirname]; - const depth = internalModule.requireDepth; + var dirname = path.dirname(filename); + var require = internalModule.makeRequireFunction.call(this); + var args = [this.exports, require, this, filename, dirname]; + var depth = internalModule.requireDepth; if (depth === 0) stat.cache = new Map(); - const result = compiledWrapper.apply(this.exports, args); + var result = compiledWrapper.apply(this.exports, args); if (depth === 0) stat.cache = null; return result; }; diff --git a/test/message/core_line_numbers.out b/test/message/core_line_numbers.out index c4909774a0e6ec..a1667df3d4ec28 100644 --- a/test/message/core_line_numbers.out +++ b/test/message/core_line_numbers.out @@ -9,7 +9,7 @@ RangeError: Invalid input at Module._compile (module.js:*:*) at Object.Module._extensions..js (module.js:*:*) at Module.load (module.js:*:*) + at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*:*) at Function.Module.runMain (module.js:*:*) at startup (node.js:*:*) - at node.js:*:* diff --git a/test/message/error_exit.out b/test/message/error_exit.out index 71690ba3d0dfb4..cd25248165d81a 100644 --- a/test/message/error_exit.out +++ b/test/message/error_exit.out @@ -8,6 +8,7 @@ AssertionError: 1 == 2 at Module._compile (module.js:*:*) at Object.Module._extensions..js (module.js:*:*) at Module.load (module.js:*:*) + at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*:*) at Function.Module.runMain (module.js:*:*) at startup (node.js:*:*) diff --git a/test/message/undefined_reference_in_new_context.out b/test/message/undefined_reference_in_new_context.out index 3364c9bd34c08f..df8b9b62d80899 100644 --- a/test/message/undefined_reference_in_new_context.out +++ b/test/message/undefined_reference_in_new_context.out @@ -11,6 +11,6 @@ ReferenceError: foo is not defined at Module._compile (module.js:*) at *..js (module.js:*) at Module.load (module.js:*) + at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*:*) at Function.Module.runMain (module.js:*:*) - at startup (node.js:*:*) diff --git a/test/message/vm_display_runtime_error.out b/test/message/vm_display_runtime_error.out index f9d8354703bc67..f57d27ac99e827 100644 --- a/test/message/vm_display_runtime_error.out +++ b/test/message/vm_display_runtime_error.out @@ -10,7 +10,7 @@ Error: boo! at Module._compile (module.js:*) at Object.Module._extensions..js (module.js:*) at Module.load (module.js:*) + at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*) at Function.Module.runMain (module.js:*) at startup (node.js:*) - at node.js:* diff --git a/test/message/vm_display_syntax_error.out b/test/message/vm_display_syntax_error.out index 14df3a8284a64f..32d99d5888143f 100644 --- a/test/message/vm_display_syntax_error.out +++ b/test/message/vm_display_syntax_error.out @@ -9,6 +9,7 @@ SyntaxError: Unexpected number at Module._compile (module.js:*) at Object.Module._extensions..js (module.js:*) at Module.load (module.js:*) + at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*) at Function.Module.runMain (module.js:*) at startup (node.js:*) @@ -22,6 +23,7 @@ SyntaxError: Unexpected number at Module._compile (module.js:*) at Object.Module._extensions..js (module.js:*) at Module.load (module.js:*) + at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*) at Function.Module.runMain (module.js:*) at startup (node.js:*) diff --git a/test/message/vm_dont_display_runtime_error.out b/test/message/vm_dont_display_runtime_error.out index ae8cf71142afae..76aa83393a5fca 100644 --- a/test/message/vm_dont_display_runtime_error.out +++ b/test/message/vm_dont_display_runtime_error.out @@ -10,7 +10,7 @@ Error: boo! at Module._compile (module.js:*) at Object.Module._extensions..js (module.js:*) at Module.load (module.js:*) + at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*) at Function.Module.runMain (module.js:*) at startup (node.js:*) - at node.js:* diff --git a/test/message/vm_dont_display_syntax_error.out b/test/message/vm_dont_display_syntax_error.out index f2e4a9495231e0..f6e60be2508ebd 100644 --- a/test/message/vm_dont_display_syntax_error.out +++ b/test/message/vm_dont_display_syntax_error.out @@ -9,6 +9,7 @@ SyntaxError: Unexpected number at Module._compile (module.js:*) at Object.Module._extensions..js (module.js:*) at Module.load (module.js:*) + at tryModuleLoad (module.js:*:*) at Function.Module._load (module.js:*) at Function.Module.runMain (module.js:*) at startup (node.js:*)