From e0b983f1ad3bbd1b683173146032ad940fd0a560 Mon Sep 17 00:00:00 2001 From: Brian White Date: Wed, 22 Mar 2017 10:24:37 -0400 Subject: [PATCH 01/15] benchmark: add filename arg for fs.WriteStream --- benchmark/fs/write-stream-throughput.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/benchmark/fs/write-stream-throughput.js b/benchmark/fs/write-stream-throughput.js index 3e54c09199409c..f0133ee49e12d1 100644 --- a/benchmark/fs/write-stream-throughput.js +++ b/benchmark/fs/write-stream-throughput.js @@ -8,6 +8,7 @@ var fs = require('fs'); var bench = common.createBenchmark(main, { dur: [5], + file: [''], type: ['buf', 'asc', 'utf'], size: [2, 1024, 65535, 1024 * 1024] }); @@ -16,6 +17,7 @@ function main(conf) { var dur = +conf.dur; var type = conf.type; var size = +conf.size; + var file = (conf.file.length === 0 ? filename : conf.file); var encoding; var chunk; @@ -35,27 +37,30 @@ function main(conf) { throw new Error('invalid type'); } - try { fs.unlinkSync(filename); } catch (e) {} + try { fs.unlinkSync(file); } catch (e) {} var started = false; var ending = false; var ended = false; + var written = 0; setTimeout(function() { ending = true; f.end(); }, dur * 1000); - var f = fs.createWriteStream(filename); + var f = fs.createWriteStream(file); f.on('drain', write); f.on('open', write); f.on('close', done); f.on('finish', function() { - ended = true; - var written = fs.statSync(filename).size / 1024; - try { fs.unlinkSync(filename); } catch (e) {} bench.end(written / 1024); + ended = true; + try { fs.unlinkSync(file); } catch (e) {} }); + function wroteChunk() { + written += size; + } function write() { // don't try to write after we end, even if a 'drain' event comes. @@ -68,7 +73,7 @@ function main(conf) { bench.start(); } - while (false !== f.write(chunk, encoding)); + while (f.write(chunk, encoding, wroteChunk) !== false); } function done() { From 0f16b5a86640ad6ad5204ef70cfe943f9def0d95 Mon Sep 17 00:00:00 2001 From: Brian White Date: Wed, 22 Mar 2017 10:28:22 -0400 Subject: [PATCH 02/15] benchmark: fs benchmark cleanup/refactor --- benchmark/fs/read-stream-throughput.js | 23 +++++++++--------- benchmark/fs/readfile.js | 32 ++++++++++--------------- benchmark/fs/write-stream-throughput.js | 22 ++++++++--------- 3 files changed, 36 insertions(+), 41 deletions(-) diff --git a/benchmark/fs/read-stream-throughput.js b/benchmark/fs/read-stream-throughput.js index bc55e44c1a18d7..63a14d4a29fabe 100644 --- a/benchmark/fs/read-stream-throughput.js +++ b/benchmark/fs/read-stream-throughput.js @@ -1,16 +1,17 @@ -// test the throughput of the fs.WriteStream class. 'use strict'; -var path = require('path'); -var common = require('../common.js'); -var filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); -var fs = require('fs'); -var filesize = 1000 * 1024 * 1024; -var assert = require('assert'); +const path = require('path'); +const common = require('../common.js'); +const filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); +const fs = require('fs'); +const filesize = 1000 * 1024 * 1024; +const assert = require('assert'); -var type, encoding, size; +var type; +var encoding; +var size; -var bench = common.createBenchmark(main, { +const bench = common.createBenchmark(main, { type: ['buf', 'asc', 'utf'], size: [1024, 4096, 65535, 1024 * 1024] }); @@ -53,9 +54,9 @@ function runTest() { }); rs.on('end', function() { - try { fs.unlinkSync(filename); } catch (e) {} // MB/sec bench.end(bytes / (1024 * 1024)); + try { fs.unlinkSync(filename); } catch (e) {} }); } @@ -81,7 +82,7 @@ function makeFile() { function write() { do { w--; - } while (false !== ws.write(buf) && w > 0); + } while (ws.write(buf) !== false && w > 0); if (w === 0) ws.end(); } diff --git a/benchmark/fs/readfile.js b/benchmark/fs/readfile.js index 9fc8316743e285..1fc1e20e4edb75 100644 --- a/benchmark/fs/readfile.js +++ b/benchmark/fs/readfile.js @@ -1,35 +1,31 @@ -// Call fs.readFile over and over again really fast. -// Then see how many times it got called. -// Yes, this is a silly benchmark. Most benchmarks are silly. 'use strict'; -var path = require('path'); -var common = require('../common.js'); -var filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); -var fs = require('fs'); +const path = require('path'); +const common = require('../common.js'); +const filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); +const fs = require('fs'); -var bench = common.createBenchmark(main, { +const bench = common.createBenchmark(main, { dur: [5], len: [1024, 16 * 1024 * 1024], concurrent: [1, 10] }); function main(conf) { - var len = +conf.len; + const len = +conf.len; + const dur = +conf.dur * 1000; + var cur = +conf.concurrent; + var reads = 0; + try { fs.unlinkSync(filename); } catch (e) {} - var data = Buffer.alloc(len, 'x'); - fs.writeFileSync(filename, data); - data = null; + fs.writeFileSync(filename, Buffer.alloc(len, 'x')); - var reads = 0; - var bench_ended = false; bench.start(); setTimeout(function() { - bench_ended = true; bench.end(reads); try { fs.unlinkSync(filename); } catch (e) {} process.exit(0); - }, +conf.dur * 1000); + }, dur); function read() { fs.readFile(filename, afterRead); @@ -43,10 +39,8 @@ function main(conf) { throw new Error('wrong number of bytes returned'); reads++; - if (!bench_ended) - read(); + read(); } - var cur = +conf.concurrent; while (cur--) read(); } diff --git a/benchmark/fs/write-stream-throughput.js b/benchmark/fs/write-stream-throughput.js index f0133ee49e12d1..9414304d9a1f9b 100644 --- a/benchmark/fs/write-stream-throughput.js +++ b/benchmark/fs/write-stream-throughput.js @@ -1,12 +1,11 @@ -// test the throughput of the fs.WriteStream class. 'use strict'; -var path = require('path'); -var common = require('../common.js'); -var filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); -var fs = require('fs'); +const path = require('path'); +const common = require('../common.js'); +const filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); +const fs = require('fs'); -var bench = common.createBenchmark(main, { +const bench = common.createBenchmark(main, { dur: [5], file: [''], type: ['buf', 'asc', 'utf'], @@ -14,10 +13,10 @@ var bench = common.createBenchmark(main, { }); function main(conf) { - var dur = +conf.dur; - var type = conf.type; - var size = +conf.size; - var file = (conf.file.length === 0 ? filename : conf.file); + const dur = +conf.dur * 1000; + const type = conf.type; + const size = +conf.size; + const file = (conf.file.length === 0 ? filename : conf.file); var encoding; var chunk; @@ -43,10 +42,11 @@ function main(conf) { var ending = false; var ended = false; var written = 0; + setTimeout(function() { ending = true; f.end(); - }, dur * 1000); + }, dur); var f = fs.createWriteStream(file); f.on('drain', write); From cc7a93e91f5449085ea9aaa8320e13982e9d33a7 Mon Sep 17 00:00:00 2001 From: Brian White Date: Wed, 22 Mar 2017 10:30:19 -0400 Subject: [PATCH 03/15] test: include stderr in npm test assertions --- test/parallel/test-npm-install.js | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/test/parallel/test-npm-install.js b/test/parallel/test-npm-install.js index 5195dbc3ef9fbd..99291c5f59a54a 100644 --- a/test/parallel/test-npm-install.js +++ b/test/parallel/test-npm-install.js @@ -6,7 +6,7 @@ if (!common.hasCrypto) { } const path = require('path'); -const spawn = require('child_process').spawn; +const execFile = require('child_process').execFile; const assert = require('assert'); const fs = require('fs'); @@ -47,17 +47,21 @@ env['NPM_CONFIG_PREFIX'] = path.join(npmSandbox, 'npm-prefix'); env['NPM_CONFIG_TMP'] = path.join(npmSandbox, 'npm-tmp'); env['HOME'] = path.join(npmSandbox, 'home'); -const proc = spawn(process.execPath, args, { +execFile(process.execPath, args, { cwd: installDir, env: env -}); - -function handleExit(code, signalCode) { - assert.strictEqual(code, 0, `npm install got error code ${code}`); - assert.strictEqual(signalCode, null, `unexpected signal: ${signalCode}`); +}, (err, stdout, stderr) => { + if (err) { + const code = err.code; + const signal = err.signal; + stderr += ''; + var msg = `npm install got error code ${code}\nstderr:\n${stderr}`; + assert.strictEqual(code, 0, msg); + msg = `unexpected signal: ${signal}\nstderr:\n${stderr}`; + assert.strictEqual(signal, null, msg); + assert.ifError(err); + } assert.doesNotThrow(function() { fs.accessSync(installDir + '/node_modules/package-name'); }); -} - -proc.on('exit', common.mustCall(handleExit)); +}); From ac679c0a9561221516d97c435905093dcfe5bc99 Mon Sep 17 00:00:00 2001 From: Brian White Date: Thu, 23 Mar 2017 02:01:29 -0400 Subject: [PATCH 04/15] fs: optimize fs.readFile() Most significant changes: * Reuse FSReqWrap across the multiple binding calls * Make fast paths for no options, unspecified open flag, and string path --- lib/fs.js | 73 +++++++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 83c0d8085de3d0..491e5c894aae88 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -43,6 +43,7 @@ const internalUtil = require('internal/util'); const assertEncoding = internalFS.assertEncoding; const stringToFlags = internalFS.stringToFlags; const getPathFromURL = internalURL.getPathFromURL; +const O_RDONLY = constants.O_RDONLY | 0; Object.defineProperty(exports, 'constants', { configurable: false, @@ -319,39 +320,44 @@ fs.existsSync = function(path) { } }; -fs.readFile = function(path, options, callback) { - callback = maybeCallback(arguments[arguments.length - 1]); - options = getOptions(options, { flag: 'r' }); +const defaultReadFileOpts = Object.create(null, { flag: { value: O_RDONLY } }); +fs.readFile = function(path, options, callback_) { + var flag; + var encoding; + var callback = maybeCallback(arguments[arguments.length - 1]); + if (callback === callback_) { + options = getOptions(options, defaultReadFileOpts); + flag = (options.flag ? stringToFlags(options.flag) : O_RDONLY); + encoding = options.encoding; + } else { + flag = O_RDONLY; + } - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; - var context = new ReadFileContext(callback, options.encoding); - context.isUserFd = isFd(path); // file descriptor ownership - var req = new FSReqWrap(); - req.context = context; - req.oncomplete = readFileAfterOpen; - + var context = new ReadFileContext(callback, encoding, isFd(path)); if (context.isUserFd) { - process.nextTick(function() { - req.oncomplete(null, path); + process.nextTick(() => { + context.req.oncomplete(null, path); }); return; } - binding.open(pathModule._makeLong(path), - stringToFlags(options.flag || 'r'), - 0o666, - req); + binding.open(pathModule._makeLong(path), flag, 0o666, context.req); }; -const kReadFileBufferLength = 8 * 1024; +function ReadFileContext(callback, encoding, isUserFd) { + this.req = new FSReqWrap(); + this.req.context = this; + this.req.oncomplete = readFileAfterOpen; -function ReadFileContext(callback, encoding) { this.fd = undefined; - this.isUserFd = undefined; + this.isUserFd = isUserFd; // file descriptor ownership this.size = undefined; this.callback = callback; this.buffers = null; @@ -361,6 +367,7 @@ function ReadFileContext(callback, encoding) { this.err = null; } +const kReadFileBufferLength = 8 * 1024; ReadFileContext.prototype.read = function() { var buffer; var offset; @@ -376,27 +383,22 @@ ReadFileContext.prototype.read = function() { length = this.size - this.pos; } - var req = new FSReqWrap(); - req.oncomplete = readFileAfterRead; - req.context = this; - - binding.read(this.fd, buffer, offset, length, -1, req); + this.req.oncomplete = readFileAfterRead; + binding.read(this.fd, buffer, offset, length, -1, this.req); }; ReadFileContext.prototype.close = function(err) { - var req = new FSReqWrap(); - req.oncomplete = readFileAfterClose; - req.context = this; + this.req.oncomplete = readFileAfterClose; this.err = err; if (this.isUserFd) { - process.nextTick(function() { - req.oncomplete(null); + process.nextTick(() => { + this.req.oncomplete(null); }); return; } - binding.close(this.fd, req); + binding.close(this.fd, this.req); }; function readFileAfterOpen(err, fd) { @@ -408,11 +410,8 @@ function readFileAfterOpen(err, fd) { } context.fd = fd; - - var req = new FSReqWrap(); - req.oncomplete = readFileAfterStat; - req.context = context; - binding.fstat(fd, req); + context.req.oncomplete = readFileAfterStat; + binding.fstat(fd, context.req); } function readFileAfterStat(err) { @@ -470,7 +469,7 @@ function readFileAfterRead(err, bytesRead) { function readFileAfterClose(err) { var context = this.context; - var buffer = null; + var buffer; var callback = context.callback; if (context.err || err) From 30bb04ea31b752b3557fc1f3117497d4d4887b6e Mon Sep 17 00:00:00 2001 From: Brian White Date: Thu, 23 Mar 2017 02:02:53 -0400 Subject: [PATCH 05/15] src: cleanup/reorganize fs code --- src/node_file.cc | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 9d00bb475d21da..7c543fd47f4ef5 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -190,7 +190,6 @@ void After(uv_fs_t *req) { argc = 2; switch (req->fs_type) { - // These all have no data to pass. case UV_FS_ACCESS: case UV_FS_CLOSE: case UV_FS_RENAME: @@ -206,7 +205,6 @@ void After(uv_fs_t *req) { case UV_FS_FCHMOD: case UV_FS_CHOWN: case UV_FS_FCHOWN: - // These, however, don't. argc = 1; break; @@ -224,10 +222,8 @@ void After(uv_fs_t *req) { break; case UV_FS_OPEN: - argv[1] = Integer::New(env->isolate(), req->result); - break; - case UV_FS_WRITE: + case UV_FS_READ: // Buffer interface argv[1] = Integer::New(env->isolate(), req->result); break; @@ -248,21 +244,6 @@ void After(uv_fs_t *req) { break; case UV_FS_READLINK: - link = StringBytes::Encode(env->isolate(), - static_cast(req->ptr), - req_wrap->encoding_); - if (link.IsEmpty()) { - argv[0] = UVException(env->isolate(), - UV_EINVAL, - req_wrap->syscall(), - "Invalid character encoding for link", - req->path, - req_wrap->data()); - } else { - argv[1] = link; - } - break; - case UV_FS_REALPATH: link = StringBytes::Encode(env->isolate(), static_cast(req->ptr), @@ -279,11 +260,6 @@ void After(uv_fs_t *req) { } break; - case UV_FS_READ: - // Buffer interface - argv[1] = Integer::New(env->isolate(), req->result); - break; - case UV_FS_SCANDIR: { int r; @@ -403,7 +379,6 @@ class fs_req_wrap { void Access(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args.GetIsolate()); - HandleScope scope(env->isolate()); if (args.Length() < 2) return TYPE_ERROR("path and mode are required"); From b1e5723739dc04e4d10b293e3e075dff2c59a031 Mon Sep 17 00:00:00 2001 From: Brian White Date: Thu, 23 Mar 2017 02:03:51 -0400 Subject: [PATCH 06/15] fs: remove confusing comment --- lib/fs.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 491e5c894aae88..f29594c060f89e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -590,9 +590,6 @@ fs.readFileSync = function(path, options) { }; -// Yes, the follow could be easily DRYed up but I provide the explicit -// list to make the arguments clear. - fs.close = function(fd, callback) { var req = new FSReqWrap(); req.oncomplete = makeCallback(callback); From 457e30b5222b389d5ec30063fa6af6665f11c99e Mon Sep 17 00:00:00 2001 From: Brian White Date: Thu, 23 Mar 2017 02:13:23 -0400 Subject: [PATCH 07/15] fs: optimize read/write streams Major changes: * Use binding methods directly -- this allows us to avoid any repeated argument validation and to reuse FSReqWrap instances * Move callbacks outside of stream functions * Use symbol check as primary instance check, with instanceof as a fallback * Make fast paths for default open flag, file mode, string file path, and default options object * Make FSReqWrap not inherit from Object.prototype --- benchmark/fs/read-stream-create.js | 17 ++ lib/fs.js | 323 ++++++++++++++++++----------- 2 files changed, 218 insertions(+), 122 deletions(-) create mode 100644 benchmark/fs/read-stream-create.js diff --git a/benchmark/fs/read-stream-create.js b/benchmark/fs/read-stream-create.js new file mode 100644 index 00000000000000..ad0ec50914a664 --- /dev/null +++ b/benchmark/fs/read-stream-create.js @@ -0,0 +1,17 @@ +'use strict'; + +const common = require('../common.js'); +const createReadStream = require('fs').createReadStream; + +const bench = common.createBenchmark(main, { + n: [2e6] +}); + +function main(conf) { + var n = +conf.n; + + bench.start(); + for (var i = 0; i < n; ++i) + createReadStream(null, { fd: 0 }); + bench.end(n); +} diff --git a/lib/fs.js b/lib/fs.js index f29594c060f89e..290b43bf30620e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -44,6 +44,16 @@ const assertEncoding = internalFS.assertEncoding; const stringToFlags = internalFS.stringToFlags; const getPathFromURL = internalURL.getPathFromURL; const O_RDONLY = constants.O_RDONLY | 0; +const O_CREAT = constants.O_CREAT | 0; +const O_TRUNC = constants.O_TRUNC | 0; +const O_WRONLY = constants.O_WRONLY | 0; + +function StorageObject() {} +StorageObject.prototype = Object.create(null); + +const emptyObject = Object.create(null); + +FSReqWrap.prototype = Object.create(null); Object.defineProperty(exports, 'constants', { configurable: false, @@ -1576,8 +1586,8 @@ fs.realpathSync = function realpathSync(p, options) { return maybeCachedResult; } - const seenLinks = Object.create(null); - const knownHard = Object.create(null); + const seenLinks = new StorageObject(); + const knownHard = new StorageObject(); const original = p; // current character position in p @@ -1683,9 +1693,9 @@ fs.realpathSync = function realpathSync(p, options) { fs.realpath = function realpath(p, options, callback) { callback = maybeCallback(typeof options === 'function' ? options : callback); if (!options) - options = emptyObj; + options = emptyObject; else - options = getOptions(options, emptyObj); + options = getOptions(options, emptyObject); if (typeof p !== 'string') { if (handleError((p = getPathFromURL(p)), callback)) return; @@ -1696,8 +1706,8 @@ fs.realpath = function realpath(p, options, callback) { return; p = pathModule.resolve(p); - const seenLinks = Object.create(null); - const knownHard = Object.create(null); + const seenLinks = new StorageObject(); + const knownHard = new StorageObject(); // current character position in p var pos; @@ -1846,33 +1856,58 @@ function allocNewPool(poolSize) { fs.createReadStream = function(path, options) { - return new ReadStream(path, options); + return new ReadStream(path, options, ReadStreamSym); }; util.inherits(ReadStream, Readable); fs.ReadStream = ReadStream; -function ReadStream(path, options) { - if (!(this instanceof ReadStream)) - return new ReadStream(path, options); - - // a little bit bigger buffer and water marks by default - options = copyObject(getOptions(options, {})); - if (options.highWaterMark === undefined) - options.highWaterMark = 64 * 1024; +const ReadStreamSym = Symbol('ReadStreamInstance'); +const ReqWrapSym = Symbol('ReqWrapInstance'); +const ReadStreamPoolSym = Symbol('ReadStreamPool'); +const ReadStreamStartSym = Symbol('ReadStreamStart'); +const defaultRSOptions = Object.create(null, { + highWaterMark: { value: 64 * 1024 } +}); +function ReadStream(path, options, instSymbol) { + if (instSymbol !== ReadStreamSym && !(this instanceof ReadStream)) + return new ReadStream(path, options, ReadStreamSym); + + if (options) { + options = copyObject(getOptions(options, defaultRSOptions)); + // a little bit bigger buffer and water marks by default + if (options.highWaterMark === undefined) + options.highWaterMark = 64 * 1024; + } else { + options = defaultRSOptions; + } Readable.call(this, options); - handleError((this.path = getPathFromURL(path))); + if (typeof path === 'object' && path !== null) + handleError((this.path = getPathFromURL(path))); + else + this.path = path; this.fd = options.fd === undefined ? null : options.fd; - this.flags = options.flags === undefined ? 'r' : options.flags; - this.mode = options.mode === undefined ? 0o666 : options.mode; + this.flags = options.flags === undefined ? O_RDONLY : options.flags; + if (options.mode === undefined) + this.mode = 0o666; + else if (typeof options.mode === 'string') + this.mode = parseInt(options.mode, 8); + else if (typeof options.mode === 'number') + this.mode = options.mode; this.start = options.start; this.end = options.end; this.autoClose = options.autoClose === undefined ? true : options.autoClose; this.pos = undefined; this.bytesRead = 0; + this.closed = false; + this[ReadStreamPoolSym] = undefined; + this[ReadStreamStartSym] = undefined; + this[ReqWrapSym] = new FSReqWrap(); + this[ReqWrapSym].stream = this; + this[ReqWrapSym].oncomplete = onrsread; if (this.start !== undefined) { if (typeof this.start !== 'number') { @@ -1903,29 +1938,55 @@ function ReadStream(path, options) { fs.FileReadStream = fs.ReadStream; // support the legacy name -ReadStream.prototype.open = function() { - var self = this; - fs.open(this.path, this.flags, this.mode, function(er, fd) { - if (er) { - if (self.autoClose) { - self.destroy(); - } - self.emit('error', er); - return; +function onrsopen(er, fd) { + const stream = this.stream; + if (er) { + if (stream.autoClose) { + stream.destroy(); } + stream.emit('error', er); + return; + } - self.fd = fd; - self.emit('open', fd); - // start the flow of data. - self.read(); - }); -}; + this.oncomplete = onrsread; + stream.fd = fd; + stream.emit('open', fd); + // start the flow of data. + stream.read(); +} + +ReadStream.prototype.open = function() { + var flags = this.flags; + if (typeof flags !== 'number') + flags = stringToFlags(flags); + this[ReqWrapSym].oncomplete = onrsopen; + binding.open(pathModule._makeLong(this.path), + flags, + this.mode, + this[ReqWrapSym]); +}; + +function onrsread(er, bytesRead) { + const stream = this.stream; + if (er) { + if (stream.autoClose) + stream.destroy(); + stream.emit('error', er); + } else if (bytesRead > 0) { + stream.bytesRead += bytesRead; + const start = stream[ReadStreamStartSym]; + stream.push(stream[ReadStreamPoolSym].slice(start, start + bytesRead)); + } else { + stream.push(null); + } +} ReadStream.prototype._read = function(n) { - if (typeof this.fd !== 'number') - return this.once('open', function() { + if (typeof this.fd !== 'number') { + return this.once('open', () => { this._read(n); }); + } if (this.destroyed) return; @@ -1938,9 +1999,9 @@ ReadStream.prototype._read = function(n) { // Grab another reference to the pool in the case that while we're // in the thread pool another read() finishes up the pool, and // allocates a new one. - var thisPool = pool; + this[ReadStreamPoolSym] = pool; var toRead = Math.min(pool.length - pool.used, n); - var start = pool.used; + this[ReadStreamStartSym] = pool.used; if (this.pos !== undefined) toRead = Math.min(this.end - this.pos + 1, toRead); @@ -1951,30 +2012,12 @@ ReadStream.prototype._read = function(n) { return this.push(null); // the actual read. - var self = this; - fs.read(this.fd, pool, pool.used, toRead, this.pos, onread); + binding.read(this.fd, pool, pool.used, toRead, this.pos, this[ReqWrapSym]); // move the pool positions, and internal position for reading. if (this.pos !== undefined) this.pos += toRead; pool.used += toRead; - - function onread(er, bytesRead) { - if (er) { - if (self.autoClose) { - self.destroy(); - } - self.emit('error', er); - } else { - var b = null; - if (bytesRead > 0) { - self.bytesRead += bytesRead; - b = thisPool.slice(start, start + bytesRead); - } - - self.push(b); - } - } }; @@ -1986,6 +2029,13 @@ ReadStream.prototype.destroy = function() { }; +function onrsclose(er) { + if (er) + this.stream.emit('error', er); + else + this.stream.emit('close'); +} + ReadStream.prototype.close = function(cb) { if (cb) this.once('close', cb); @@ -2000,40 +2050,61 @@ ReadStream.prototype.close = function(cb) { this.closed = true; - fs.close(this.fd, (er) => { - if (er) - this.emit('error', er); - else - this.emit('close'); - }); + this[ReqWrapSym].oncomplete = onrsclose; + binding.close(this.fd, this[ReqWrapSym]); this.fd = null; }; fs.createWriteStream = function(path, options) { - return new WriteStream(path, options); + return new WriteStream(path, options, WriteStreamSym); }; util.inherits(WriteStream, Writable); fs.WriteStream = WriteStream; -function WriteStream(path, options) { - if (!(this instanceof WriteStream)) - return new WriteStream(path, options); - options = copyObject(getOptions(options, {})); +const WriteStreamSym = Symbol('WriteStreamInstance'); +const WriteStreamCbSym = Symbol('WriteStreamCallback'); +const WriteStreamDataSym = Symbol('WriteStreamData'); +const ReqWrapEvSym = Symbol('ReqWrapInstanceEv'); +const defaultWSOptions = emptyObject; +const defaultWSFlags = O_CREAT | O_TRUNC | O_WRONLY; +function WriteStream(path, options, instSymbol) { + if (instSymbol !== WriteStreamSym && !(this instanceof WriteStream)) + return new WriteStream(path, options, WriteStreamSym); + + if (options) + options = copyObject(getOptions(options, defaultWSOptions)); + else + options = defaultWSOptions; Writable.call(this, options); - handleError((this.path = getPathFromURL(path))); + if (typeof path === 'object' && path !== null) + handleError((this.path = getPathFromURL(path))); + else + this.path = path; this.fd = options.fd === undefined ? null : options.fd; - this.flags = options.flags === undefined ? 'w' : options.flags; - this.mode = options.mode === undefined ? 0o666 : options.mode; + this.flags = options.flags === undefined ? defaultWSFlags : options.flags; + if (options.mode === undefined) + this.mode = 0o666; + else if (typeof options.mode === 'string') + this.mode = parseInt(options.mode, 8); + else if (typeof options.mode === 'number') + this.mode = options.mode; this.start = options.start; - this.autoClose = options.autoClose === undefined ? true : !!options.autoClose; + this.autoClose = options.autoClose === undefined ? true : options.autoClose; this.pos = undefined; this.bytesWritten = 0; + this[WriteStreamCbSym] = undefined; + this[WriteStreamDataSym] = undefined; + this[ReqWrapSym] = new FSReqWrap(); + this[ReqWrapEvSym] = new FSReqWrap(); + this[ReqWrapSym].stream = this[ReqWrapEvSym].stream = this; + this[ReqWrapSym].oncomplete = onwswrite; + this[ReqWrapEvSym].oncomplete = onwswritev; if (this.start !== undefined) { if (typeof this.start !== 'number') { @@ -2063,86 +2134,94 @@ function WriteStream(path, options) { fs.FileWriteStream = fs.WriteStream; // support the legacy name -WriteStream.prototype.open = function() { - fs.open(this.path, this.flags, this.mode, function(er, fd) { - if (er) { - if (this.autoClose) { - this.destroy(); - } - this.emit('error', er); - return; +function onwsopen(er, fd) { + const stream = this.stream; + if (er) { + if (stream.autoClose) { + stream.destroy(); } + stream.emit('error', er); + return; + } - this.fd = fd; - this.emit('open', fd); - }.bind(this)); -}; + this.oncomplete = onwswrite; + stream.fd = fd; + stream.emit('open', fd); +} +WriteStream.prototype.open = function() { + var flags = this.flags; + if (typeof flags !== 'number') + flags = stringToFlags(flags); + this[ReqWrapSym].oncomplete = onwsopen; + binding.open(pathModule._makeLong(this.path), + flags, + this.mode, + this[ReqWrapSym]); +}; + +function onwswrite(er, bytes) { + const stream = this.stream; + stream[WriteStreamDataSym] = undefined; + if (er) { + if (stream.autoClose) { + stream.destroy(); + } + return stream[WriteStreamCbSym](er); + } + stream.bytesWritten += bytes; + stream[WriteStreamCbSym](); +} WriteStream.prototype._write = function(data, encoding, cb) { if (!(data instanceof Buffer)) return this.emit('error', new Error('Invalid data')); - if (typeof this.fd !== 'number') - return this.once('open', function() { + if (typeof this.fd !== 'number') { + return this.once('open', () => { this._write(data, encoding, cb); }); + } - var self = this; - fs.write(this.fd, data, 0, data.length, this.pos, function(er, bytes) { - if (er) { - if (self.autoClose) { - self.destroy(); - } - return cb(er); - } - self.bytesWritten += bytes; - cb(); - }); + this[WriteStreamCbSym] = cb; + this[WriteStreamDataSym] = data; + binding.writeBuffer(this.fd, data, 0, data.length, this.pos, + this[ReqWrapSym]); if (this.pos !== undefined) this.pos += data.length; }; - -function writev(fd, chunks, position, callback) { - function wrapper(err, written) { - // Retain a reference to chunks so that they can't be GC'ed too soon. - callback(err, written || 0, chunks); +function onwswritev(er, bytes) { + const stream = this.stream; + stream[WriteStreamDataSym] = undefined; + if (er) { + stream.destroy(); + return stream[WriteStreamCbSym](er); } - - const req = new FSReqWrap(); - req.oncomplete = wrapper; - binding.writeBuffers(fd, chunks, position, req); + stream.bytesWritten += bytes; + stream[WriteStreamCbSym](); } - WriteStream.prototype._writev = function(data, cb) { - if (typeof this.fd !== 'number') - return this.once('open', function() { + if (typeof this.fd !== 'number') { + return this.once('open', () => { this._writev(data, cb); }); + } - const self = this; - const len = data.length; - const chunks = new Array(len); + const chunks = new Array(data.length); var size = 0; - for (var i = 0; i < len; i++) { - var chunk = data[i].chunk; - + for (var i = 0; i < data.length; i++) { + const chunk = data[i].chunk; chunks[i] = chunk; size += chunk.length; } - writev(this.fd, chunks, this.pos, function(er, bytes) { - if (er) { - self.destroy(); - return cb(er); - } - self.bytesWritten += bytes; - cb(); - }); + this[WriteStreamCbSym] = cb; + this[WriteStreamDataSym] = chunks; + binding.writeBuffers(this.fd, chunks, this.pos, this[ReqWrapEvSym]); if (this.pos !== undefined) this.pos += size; From d5c7653b0630a76dbaea143f1bd6bd1abbde344b Mon Sep 17 00:00:00 2001 From: Brian White Date: Fri, 24 Mar 2017 21:39:20 -0400 Subject: [PATCH 08/15] fs: optimize readFileSync() and openSync() --- lib/fs.js | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 290b43bf30620e..922b07ebe4f967 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -544,9 +544,17 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) { } fs.readFileSync = function(path, options) { - options = getOptions(options, { flag: 'r' }); + var flag; + var encoding; + if (options) { + options = getOptions(options, defaultReadFileOpts); + flag = (options.flag ? stringToFlags(options.flag) : O_RDONLY); + encoding = options.encoding; + } else { + flag = O_RDONLY; + } var isUserFd = isFd(path); // file descriptor ownership - var fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666); + var fd = isUserFd ? path : fs.openSync(path, flag, 0o666); // Use stats array directly to avoid creating an fs.Stats instance just for // our internal use. @@ -595,7 +603,8 @@ fs.readFileSync = function(path, options) { buffer = buffer.slice(0, pos); } - if (options.encoding) buffer = buffer.toString(options.encoding); + if (encoding) + buffer = buffer.toString(encoding); return buffer; }; @@ -639,9 +648,12 @@ fs.open = function(path, flags, mode, callback_) { fs.openSync = function(path, flags, mode) { mode = modeNum(mode, 0o666); - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); - return binding.open(pathModule._makeLong(path), stringToFlags(flags), mode); + if (typeof flags !== 'number') + flags = stringToFlags(flags); + return binding.open(pathModule._makeLong(path), flags, mode); }; fs.read = function(fd, buffer, offset, length, position, callback) { @@ -1566,12 +1578,11 @@ if (isWindows) { nextPart = function nextPart(p, i) { return p.indexOf('/', i); }; } -const emptyObj = Object.create(null); fs.realpathSync = function realpathSync(p, options) { if (!options) - options = emptyObj; + options = emptyObject; else - options = getOptions(options, emptyObj); + options = getOptions(options, emptyObject); if (typeof p !== 'string') { handleError((p = getPathFromURL(p))); if (typeof p !== 'string') From 12854147df9c6653993ff71d460ac1850093aba5 Mon Sep 17 00:00:00 2001 From: Brian White Date: Fri, 24 Mar 2017 22:02:41 -0400 Subject: [PATCH 09/15] fs: optimize fd checking --- lib/fs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/fs.js b/lib/fs.js index 922b07ebe4f967..6f00d00c4efd08 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -178,7 +178,7 @@ function nullCheck(path, callback) { } function isFd(path) { - return (path >>> 0) === path; + return (typeof path === 'number' && Math.round(path) === path); } // Constructor for file stats. From 5df97a3664535e3d9db8f0a892ce7ea6a69d5d4a Mon Sep 17 00:00:00 2001 From: Brian White Date: Sat, 25 Mar 2017 05:41:03 -0400 Subject: [PATCH 10/15] fs: optimize appendFile*() --- lib/fs.js | 49 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 6f00d00c4efd08..6f466074672518 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -47,6 +47,7 @@ const O_RDONLY = constants.O_RDONLY | 0; const O_CREAT = constants.O_CREAT | 0; const O_TRUNC = constants.O_TRUNC | 0; const O_WRONLY = constants.O_WRONLY | 0; +const O_APPEND = constants.O_APPEND | 0; function StorageObject() {} StorageObject.prototype = Object.create(null); @@ -1320,29 +1321,45 @@ fs.writeFileSync = function(path, data, options) { } }; +const appendFlags = O_APPEND | O_CREAT | O_WRONLY; +const defaultAppendOpts = Object.create(null, { + encoding: { value: 'utf8' }, + mode: { value: 0o666 }, + flag: { value: appendFlags } +}); fs.appendFile = function(path, data, options, callback) { callback = maybeCallback(arguments[arguments.length - 1]); - options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); - - // Don't make changes directly on options object - options = copyObject(options); - - // force append behavior when using a supplied file descriptor - if (!options.flag || isFd(path)) - options.flag = 'a'; + if (options) { + var options_ = getOptions(options, defaultAppendOpts); + // force append behavior when using a supplied file descriptor + if (options_ !== defaultAppendOpts && (!options_.flag || isFd(path))) { + // Don't make changes directly on options object + options = copyObject(options_); + options.flag = appendFlags; + } else { + options = options_; + } + } else { + options = defaultAppendOpts; + } fs.writeFile(path, data, options, callback); }; fs.appendFileSync = function(path, data, options) { - options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); - - // Don't make changes directly on options object - options = copyObject(options); - - // force append behavior when using a supplied file descriptor - if (!options.flag || isFd(path)) - options.flag = 'a'; + if (options) { + var options_ = getOptions(options, defaultAppendOpts); + // force append behavior when using a supplied file descriptor + if (options_ !== defaultAppendOpts && (!options_.flag || isFd(path))) { + // Don't make changes directly on options object + options = copyObject(options_); + options.flag = appendFlags; + } else { + options = options_; + } + } else { + options = defaultAppendOpts; + } fs.writeFileSync(path, data, options); }; From 87592ab3894290a6ea6c9b092970161683314d65 Mon Sep 17 00:00:00 2001 From: Brian White Date: Sat, 25 Mar 2017 05:42:39 -0400 Subject: [PATCH 11/15] fs: add optimizations for more methods --- lib/fs.js | 208 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 138 insertions(+), 70 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 6f466074672518..dc960ad2bb0f76 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -267,11 +267,15 @@ Object.defineProperties(fs, { }); function handleError(val, callback) { + if (typeof val === 'string') + return false; if (val instanceof Error) { if (typeof callback === 'function') { process.nextTick(callback, val); return true; - } else throw val; + } else { + throw val; + } } return false; } @@ -284,8 +288,10 @@ fs.access = function(path, mode, callback) { throw new TypeError('"callback" argument must be a function'); } - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; @@ -297,7 +303,8 @@ fs.access = function(path, mode, callback) { }; fs.accessSync = function(path, mode) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); if (mode === undefined) @@ -309,8 +316,10 @@ fs.accessSync = function(path, mode) { }; fs.exists = function(path, callback) { - if (handleError((path = getPathFromURL(path)), cb)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), cb)) + return; + } if (!nullCheck(path, cb)) return; var req = new FSReqWrap(); req.oncomplete = cb; @@ -322,7 +331,8 @@ fs.exists = function(path, callback) { fs.existsSync = function(path) { try { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); binding.stat(pathModule._makeLong(path)); return true; @@ -633,18 +643,19 @@ function modeNum(m, def) { fs.open = function(path, flags, mode, callback_) { var callback = makeCallback(arguments[arguments.length - 1]); mode = modeNum(mode, 0o666); + if (typeof flags !== 'number') + flags = stringToFlags(flags); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; - binding.open(pathModule._makeLong(path), - stringToFlags(flags), - mode, - req); + binding.open(pathModule._makeLong(path), flags, mode, req); }; fs.openSync = function(path, flags, mode) { @@ -748,11 +759,15 @@ fs.writeSync = function(fd, buffer, offset, length, position) { fs.rename = function(oldPath, newPath, callback) { callback = makeCallback(callback); - if (handleError((oldPath = getPathFromURL(oldPath)), callback)) - return; + if (typeof oldPath !== 'string') { + if (handleError((oldPath = getPathFromURL(oldPath)), callback)) + return; + } - if (handleError((newPath = getPathFromURL(newPath)), callback)) - return; + if (typeof newPath !== 'string') { + if (handleError((newPath = getPathFromURL(newPath)), callback)) + return; + } if (!nullCheck(oldPath, callback)) return; if (!nullCheck(newPath, callback)) return; @@ -764,8 +779,10 @@ fs.rename = function(oldPath, newPath, callback) { }; fs.renameSync = function(oldPath, newPath) { - handleError((oldPath = getPathFromURL(oldPath))); - handleError((newPath = getPathFromURL(newPath))); + if (typeof oldPath !== 'string') + handleError((oldPath = getPathFromURL(oldPath))); + if (typeof newPath !== 'string') + handleError((newPath = getPathFromURL(newPath))); nullCheck(oldPath); nullCheck(newPath); return binding.rename(pathModule._makeLong(oldPath), @@ -837,8 +854,10 @@ fs.ftruncateSync = function(fd, len) { fs.rmdir = function(path, callback) { callback = maybeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -846,7 +865,8 @@ fs.rmdir = function(path, callback) { }; fs.rmdirSync = function(path) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); return binding.rmdir(pathModule._makeLong(path)); }; @@ -874,8 +894,10 @@ fs.fsyncSync = function(fd) { fs.mkdir = function(path, mode, callback) { if (typeof mode === 'function') callback = mode; callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -885,7 +907,8 @@ fs.mkdir = function(path, mode, callback) { }; fs.mkdirSync = function(path, mode) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); return binding.mkdir(pathModule._makeLong(path), modeNum(mode, 0o777)); @@ -894,8 +917,10 @@ fs.mkdirSync = function(path, mode) { fs.readdir = function(path, options, callback) { callback = makeCallback(typeof options === 'function' ? options : callback); options = getOptions(options, {}); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -904,7 +929,8 @@ fs.readdir = function(path, options, callback) { fs.readdirSync = function(path, options) { options = getOptions(options, {}); - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); return binding.readdir(pathModule._makeLong(path), options.encoding); }; @@ -917,8 +943,10 @@ fs.fstat = function(fd, callback) { fs.lstat = function(path, callback) { callback = makeStatsCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -927,8 +955,10 @@ fs.lstat = function(path, callback) { fs.stat = function(path, callback) { callback = makeStatsCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -941,14 +971,16 @@ fs.fstatSync = function(fd) { }; fs.lstatSync = function(path) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); binding.lstat(pathModule._makeLong(path)); return statsFromValues(); }; fs.statSync = function(path) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); binding.stat(pathModule._makeLong(path)); return statsFromValues(); @@ -957,8 +989,10 @@ fs.statSync = function(path) { fs.readlink = function(path, options, callback) { callback = makeCallback(typeof options === 'function' ? options : callback); options = getOptions(options, {}); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -967,7 +1001,8 @@ fs.readlink = function(path, options, callback) { fs.readlinkSync = function(path, options) { options = getOptions(options, {}); - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); return binding.readlink(pathModule._makeLong(path), options.encoding); }; @@ -991,11 +1026,15 @@ fs.symlink = function(target, path, type_, callback_) { var type = (typeof type_ === 'string' ? type_ : null); var callback = makeCallback(arguments[arguments.length - 1]); - if (handleError((target = getPathFromURL(target)), callback)) - return; + if (typeof target !== 'string') { + if (handleError((target = getPathFromURL(target)), callback)) + return; + } - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(target, callback)) return; if (!nullCheck(path, callback)) return; @@ -1011,8 +1050,10 @@ fs.symlink = function(target, path, type_, callback_) { fs.symlinkSync = function(target, path, type) { type = (typeof type === 'string' ? type : null); - handleError((target = getPathFromURL(target))); - handleError((path = getPathFromURL(path))); + if (typeof target !== 'string') + handleError((target = getPathFromURL(target))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(target); nullCheck(path); @@ -1024,11 +1065,15 @@ fs.symlinkSync = function(target, path, type) { fs.link = function(existingPath, newPath, callback) { callback = makeCallback(callback); - if (handleError((existingPath = getPathFromURL(existingPath)), callback)) - return; + if (typeof existingPath !== 'string') { + if (handleError((existingPath = getPathFromURL(existingPath)), callback)) + return; + } - if (handleError((newPath = getPathFromURL(newPath)), callback)) - return; + if (typeof newPath !== 'string') { + if (handleError((newPath = getPathFromURL(newPath)), callback)) + return; + } if (!nullCheck(existingPath, callback)) return; if (!nullCheck(newPath, callback)) return; @@ -1042,8 +1087,10 @@ fs.link = function(existingPath, newPath, callback) { }; fs.linkSync = function(existingPath, newPath) { - handleError((existingPath = getPathFromURL(existingPath))); - handleError((newPath = getPathFromURL(newPath))); + if (typeof existingPath !== 'string') + handleError((existingPath = getPathFromURL(existingPath))); + if (typeof newPath !== 'string') + handleError((newPath = getPathFromURL(newPath))); nullCheck(existingPath); nullCheck(newPath); return binding.link(pathModule._makeLong(existingPath), @@ -1052,8 +1099,10 @@ fs.linkSync = function(existingPath, newPath) { fs.unlink = function(path, callback) { callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -1061,7 +1110,8 @@ fs.unlink = function(path, callback) { }; fs.unlinkSync = function(path) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); return binding.unlink(pathModule._makeLong(path)); }; @@ -1118,8 +1168,10 @@ if (constants.O_SYMLINK !== undefined) { fs.chmod = function(path, mode, callback) { callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -1129,7 +1181,8 @@ fs.chmod = function(path, mode, callback) { }; fs.chmodSync = function(path, mode) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); return binding.chmod(pathModule._makeLong(path), modeNum(mode)); }; @@ -1164,8 +1217,10 @@ fs.fchownSync = function(fd, uid, gid) { fs.chown = function(path, uid, gid, callback) { callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -1173,7 +1228,8 @@ fs.chown = function(path, uid, gid, callback) { }; fs.chownSync = function(path, uid, gid) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); return binding.chown(pathModule._makeLong(path), uid, gid); }; @@ -1202,8 +1258,10 @@ fs._toUnixTimestamp = toUnixTimestamp; fs.utimes = function(path, atime, mtime, callback) { callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; + if (typeof path !== 'string') { + if (handleError((path = getPathFromURL(path)), callback)) + return; + } if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -1214,7 +1272,8 @@ fs.utimes = function(path, atime, mtime, callback) { }; fs.utimesSync = function(path, atime, mtime) { - handleError((path = getPathFromURL(path))); + if (typeof path !== 'string') + handleError((path = getPathFromURL(path))); nullCheck(path); atime = toUnixTimestamp(atime); mtime = toUnixTimestamp(mtime); @@ -1391,7 +1450,8 @@ FSWatcher.prototype.start = function(filename, persistent, recursive, encoding) { - handleError((filename = getPathFromURL(filename))); + if (typeof filename !== 'string') + handleError((filename = getPathFromURL(filename))); nullCheck(filename); var err = this._handle.start(pathModule._makeLong(filename), persistent, @@ -1410,7 +1470,8 @@ FSWatcher.prototype.close = function() { }; fs.watch = function(filename, options, listener) { - handleError((filename = getPathFromURL(filename))); + if (typeof filename !== 'string') + handleError((filename = getPathFromURL(filename))); nullCheck(filename); if (typeof options === 'function') { @@ -1480,7 +1541,8 @@ util.inherits(StatWatcher, EventEmitter); StatWatcher.prototype.start = function(filename, persistent, interval) { - handleError((filename = getPathFromURL(filename))); + if (typeof filename !== 'string') + handleError((filename = getPathFromURL(filename))); nullCheck(filename); this._handle.start(pathModule._makeLong(filename), persistent, interval); }; @@ -1494,7 +1556,8 @@ StatWatcher.prototype.stop = function() { const statWatchers = new Map(); fs.watchFile = function(filename, options, listener) { - handleError((filename = getPathFromURL(filename))); + if (typeof filename !== 'string') + handleError((filename = getPathFromURL(filename))); nullCheck(filename); filename = pathModule.resolve(filename); var stat; @@ -1531,7 +1594,8 @@ fs.watchFile = function(filename, options, listener) { }; fs.unwatchFile = function(filename, listener) { - handleError((filename = getPathFromURL(filename))); + if (typeof filename !== 'string') + handleError((filename = getPathFromURL(filename))); nullCheck(filename); filename = pathModule.resolve(filename); var stat = statWatchers.get(filename); @@ -1902,10 +1966,14 @@ function ReadStream(path, options, instSymbol) { return new ReadStream(path, options, ReadStreamSym); if (options) { - options = copyObject(getOptions(options, defaultRSOptions)); - // a little bit bigger buffer and water marks by default - if (options.highWaterMark === undefined) + var options_ = getOptions(options, defaultRSOptions); + if (options_ !== defaultRSOptions && options.highWaterMark === undefined) { + options = copyObject(options_); + // a little bit bigger buffer and water marks by default options.highWaterMark = 64 * 1024; + } else { + options = options_; + } } else { options = defaultRSOptions; } @@ -2103,7 +2171,7 @@ function WriteStream(path, options, instSymbol) { return new WriteStream(path, options, WriteStreamSym); if (options) - options = copyObject(getOptions(options, defaultWSOptions)); + options = getOptions(options, defaultWSOptions); else options = defaultWSOptions; From 22345d4727af0e93202da75b4a6d809a7c7d57fe Mon Sep 17 00:00:00 2001 From: Brian White Date: Sun, 26 Mar 2017 05:28:14 -0400 Subject: [PATCH 12/15] fs: use faster shallow object cloner --- lib/fs.js | 9 +-------- src/node_util.cc | 11 +++++++++++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index dc960ad2bb0f76..83c103b972494e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -28,7 +28,7 @@ const constants = process.binding('constants').fs; const { S_IFMT, S_IFREG, S_IFLNK } = constants; const util = require('util'); const pathModule = require('path'); -const { isUint8Array } = process.binding('util'); +const { isUint8Array, shallowClone: copyObject } = process.binding('util'); const binding = process.binding('fs'); const fs = exports; @@ -93,13 +93,6 @@ function getOptions(options, defaultOptions) { return options; } -function copyObject(source) { - const target = {}; - for (const key in source) - target[key] = source[key]; - return target; -} - function rethrow() { // TODO(thefourtheye) Throw error instead of warning in major version > 7 process.emitWarning( diff --git a/src/node_util.cc b/src/node_util.cc index 5f3de643d4c2f3..41479e3510d619 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -147,6 +147,15 @@ void WatchdogHasPendingSigint(const FunctionCallbackInfo& args) { } +void ShallowClone(const FunctionCallbackInfo& args) { + if (!args[0]->IsObject()) { + Environment* env = Environment::GetCurrent(args); + return env->ThrowTypeError("obj must be an object"); + } + args.GetReturnValue().Set(args[0].As()->Clone()); +} + + void Initialize(Local target, Local unused, Local context) { @@ -192,6 +201,8 @@ void Initialize(Local target, env->SetMethod(target, "startSigintWatchdog", StartSigintWatchdog); env->SetMethod(target, "stopSigintWatchdog", StopSigintWatchdog); env->SetMethod(target, "watchdogHasPendingSigint", WatchdogHasPendingSigint); + + env->SetMethod(target, "shallowClone", ShallowClone); } } // namespace util From 85f2c1f44dbdd4e6871eb32351ab823c4404d9aa Mon Sep 17 00:00:00 2001 From: Brian White Date: Tue, 28 Mar 2017 19:46:28 -0400 Subject: [PATCH 13/15] fs: more misc optimizations The main focus of this commit is to: * Avoid options object copying where possible * Use V8's object cloning only for default options objects where we are in control of the object's configuration since the object clone will also copy properties' configuration, frozen/sealed state, etc. * Refactor writeFile*()/appendFile*() to reuse shared logic without the redundant argument checking and to reuse FSReqWrap instances more --- lib/fs.js | 291 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 173 insertions(+), 118 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 83c103b972494e..8d04a289b9f7c9 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -28,7 +28,7 @@ const constants = process.binding('constants').fs; const { S_IFMT, S_IFREG, S_IFLNK } = constants; const util = require('util'); const pathModule = require('path'); -const { isUint8Array, shallowClone: copyObject } = process.binding('util'); +const { isUint8Array, shallowClone } = process.binding('util'); const binding = process.binding('fs'); const fs = exports; @@ -74,13 +74,8 @@ const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG); const errnoException = util._errnoException; function getOptions(options, defaultOptions) { - if (options === null || options === undefined || - typeof options === 'function') { - return defaultOptions; - } - if (typeof options === 'string') { - defaultOptions = util._extend({}, defaultOptions); + defaultOptions = shallowClone(defaultOptions); defaultOptions.encoding = options; options = defaultOptions; } else if (typeof options !== 'object') { @@ -334,12 +329,14 @@ fs.existsSync = function(path) { } }; -const defaultReadFileOpts = Object.create(null, { flag: { value: O_RDONLY } }); -fs.readFile = function(path, options, callback_) { +const defaultReadFileOpts = Object.create(null, { + flag: { value: O_RDONLY, writable: true } +}); +fs.readFile = function(path, options, callback) { var flag; var encoding; - var callback = maybeCallback(arguments[arguments.length - 1]); - if (callback === callback_) { + callback = maybeCallback(arguments[arguments.length - 1]); + if (options != null && typeof options !== 'function') { options = getOptions(options, defaultReadFileOpts); flag = (options.flag ? stringToFlags(options.flag) : O_RDONLY); encoding = options.encoding; @@ -550,7 +547,7 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) { fs.readFileSync = function(path, options) { var flag; var encoding; - if (options) { + if (options != null) { options = getOptions(options, defaultReadFileOpts); flag = (options.flag ? stringToFlags(options.flag) : O_RDONLY); encoding = options.encoding; @@ -908,8 +905,14 @@ fs.mkdirSync = function(path, mode) { }; fs.readdir = function(path, options, callback) { - callback = makeCallback(typeof options === 'function' ? options : callback); - options = getOptions(options, {}); + var enc; + if (typeof options === 'function') { + callback = makeCallback(options); + } else { + callback = makeCallback(callback); + if (options != null) + enc = getOptions(options, emptyObject).encoding; + } if (typeof path !== 'string') { if (handleError((path = getPathFromURL(path)), callback)) return; @@ -917,15 +920,17 @@ fs.readdir = function(path, options, callback) { if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; - binding.readdir(pathModule._makeLong(path), options.encoding, req); + binding.readdir(pathModule._makeLong(path), enc, req); }; fs.readdirSync = function(path, options) { - options = getOptions(options, {}); + var enc; + if (options != null) + enc = getOptions(options, emptyObject).encoding; if (typeof path !== 'string') handleError((path = getPathFromURL(path))); nullCheck(path); - return binding.readdir(pathModule._makeLong(path), options.encoding); + return binding.readdir(pathModule._makeLong(path), enc); }; fs.fstat = function(fd, callback) { @@ -980,8 +985,14 @@ fs.statSync = function(path) { }; fs.readlink = function(path, options, callback) { - callback = makeCallback(typeof options === 'function' ? options : callback); - options = getOptions(options, {}); + var enc; + if (typeof options === 'function') { + callback = makeCallback(options); + } else { + callback = makeCallback(callback); + if (options != null) + enc = getOptions(options, emptyObject).encoding; + } if (typeof path !== 'string') { if (handleError((path = getPathFromURL(path)), callback)) return; @@ -989,15 +1000,17 @@ fs.readlink = function(path, options, callback) { if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; - binding.readlink(pathModule._makeLong(path), options.encoding, req); + binding.readlink(pathModule._makeLong(path), enc, req); }; fs.readlinkSync = function(path, options) { - options = getOptions(options, {}); + var enc; + if (options != null) + enc = getOptions(options, emptyObject).encoding; if (typeof path !== 'string') handleError((path = getPathFromURL(path))); nullCheck(path); - return binding.readlink(pathModule._makeLong(path), options.encoding); + return binding.readlink(pathModule._makeLong(path), enc); }; function preprocessSymlinkDestination(path, type, linkPath) { @@ -1287,25 +1300,26 @@ fs.futimesSync = function(fd, atime, mtime) { binding.futimes(fd, atime, mtime); }; -function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) { - var callback = maybeCallback(arguments[arguments.length - 1]); - - // write(fd, buffer, offset, length, position, callback) - fs.write(fd, buffer, offset, length, position, function(writeErr, written) { - if (writeErr) { +function writeAll(fd, isUserFd, buffer, offset, length, position, callback) { + var req = new FSReqWrap(); + req.oncomplete = (err, written) => { + if (err) { if (isUserFd) { - callback(writeErr); + callback(err); } else { - fs.close(fd, function() { - callback(writeErr); - }); + req.oncomplete = () => { + callback(err); + }; + binding.close(fd, req); } } else { + written || (written = 0); if (written === length) { if (isUserFd) { callback(null); } else { - fs.close(fd, callback); + req.oncomplete = callback; + binding.close(fd, req); } } else { offset += written; @@ -1313,107 +1327,140 @@ function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) { if (position !== null) { position += written; } - writeAll(fd, isUserFd, buffer, offset, length, position, callback); + binding.writeBuffer(fd, buffer, offset, length, position, req); } } - }); + }; + binding.writeBuffer(fd, buffer, offset, length, position, req); } -fs.writeFile = function(path, data, options, callback) { - callback = maybeCallback(arguments[arguments.length - 1]); - options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' }); - const flag = options.flag || 'w'; - - if (isFd(path)) { - writeFd(path, true); +function writeFile_(path, isUserFd, data, enc, mode, flag, callback) { + if (isUserFd) { + writeFd(path); return; } - fs.open(path, flag, options.mode, function(openErr, fd) { + fs.open(path, flag, mode, (openErr, fd) => { if (openErr) { callback(openErr); } else { - writeFd(fd, false); + writeFd(fd); } }); - function writeFd(fd, isUserFd) { + function writeFd(fd) { var buffer = isUint8Array(data) ? - data : Buffer.from('' + data, options.encoding || 'utf8'); - var position = /a/.test(flag) ? null : 0; + data : Buffer.from('' + data, enc || 'utf8'); + var position = (flag & O_APPEND ? null : 0); writeAll(fd, isUserFd, buffer, 0, buffer.length, position, callback); } -}; - -fs.writeFileSync = function(path, data, options) { - options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' }); - const flag = options.flag || 'w'; - - var isUserFd = isFd(path); // file descriptor ownership - var fd = isUserFd ? path : fs.openSync(path, flag, options.mode); +} - if (!isUint8Array(data)) { - data = Buffer.from('' + data, options.encoding || 'utf8'); +const writeFlags = O_TRUNC | O_CREAT | O_WRONLY; +const defaultWriteOpts = Object.create(null, { + encoding: { value: 'utf8', writable: true }, + mode: { value: 0o666, writable: true }, + flag: { value: writeFlags, writable: true } +}); +fs.writeFile = function(path, data, options, callback) { + callback = maybeCallback(arguments[arguments.length - 1]); + var flag = writeFlags; + if (options != null && typeof options !== 'function') { + options = getOptions(options, defaultWriteOpts); + if (options.flag) { + if (typeof options.flag !== 'number') + flag = stringToFlags(options.flag); + else + flag = options.flag; + } + } else { + options = defaultWriteOpts; } + writeFile_(path, isFd(path), data, options.encoding, options.mode, flag, + callback); +}; + +function writeFileSync_(path, isUserFd, data, enc, mode, flag) { + if (!isUint8Array(data)) + data = Buffer.from('' + data, enc || 'utf8'); + var fd = isUserFd ? path : fs.openSync(path, flag, mode); var offset = 0; var length = data.length; - var position = /a/.test(flag) ? null : 0; + var position = (flag & O_APPEND ? null : 0); try { while (length > 0) { - var written = fs.writeSync(fd, data, offset, length, position); + var written = binding.writeBuffer(fd, data, offset, length, position); offset += written; length -= written; - if (position !== null) { + if (position !== null) position += written; - } } } finally { - if (!isUserFd) fs.closeSync(fd); + if (!isUserFd) binding.close(fd); } +} + +fs.writeFileSync = function(path, data, options) { + var flag = writeFlags; + if (options != null) { + options = getOptions(options, defaultWriteOpts); + if (options.flag) { + if (typeof options.flag !== 'number') + flag = stringToFlags(options.flag); + else + flag = options.flag; + } + } else { + options = defaultWriteOpts; + } + writeFileSync_(path, isFd(path), data, options.encoding, options.mode, flag); }; const appendFlags = O_APPEND | O_CREAT | O_WRONLY; const defaultAppendOpts = Object.create(null, { - encoding: { value: 'utf8' }, - mode: { value: 0o666 }, - flag: { value: appendFlags } + encoding: { value: 'utf8', writable: true }, + mode: { value: 0o666, writable: true }, + flag: { value: appendFlags, writable: true } }); fs.appendFile = function(path, data, options, callback) { callback = maybeCallback(arguments[arguments.length - 1]); - if (options) { - var options_ = getOptions(options, defaultAppendOpts); + var flag = appendFlags; + var isUserFd = isFd(path); + if (options != null && typeof options !== 'function') { + options = getOptions(options, defaultAppendOpts); // force append behavior when using a supplied file descriptor - if (options_ !== defaultAppendOpts && (!options_.flag || isFd(path))) { - // Don't make changes directly on options object - options = copyObject(options_); - options.flag = appendFlags; - } else { - options = options_; + if (options.flag && !isUserFd) { + if (typeof options.flag !== 'number') + flag = stringToFlags(options.flag); + else + flag = options.flag; } } else { options = defaultAppendOpts; } - fs.writeFile(path, data, options, callback); + writeFile_(path, isUserFd, data, options.encoding, options.mode, flag, + callback); }; fs.appendFileSync = function(path, data, options) { - if (options) { - var options_ = getOptions(options, defaultAppendOpts); + var flag = appendFlags; + var isUserFd = isFd(path); + if (options != null) { + options = getOptions(options, defaultAppendOpts); // force append behavior when using a supplied file descriptor - if (options_ !== defaultAppendOpts && (!options_.flag || isFd(path))) { - // Don't make changes directly on options object - options = copyObject(options_); - options.flag = appendFlags; - } else { - options = options_; + if (options.flag && !isUserFd) { + if (typeof options.flag !== 'number') + flag = stringToFlags(options.flag); + else + flag = options.flag; } } else { options = defaultAppendOpts; } - fs.writeFileSync(path, data, options); + writeFileSync_(path, isUserFd, data, options.encoding, options.mode, flag); }; function FSWatcher() { @@ -1467,25 +1514,25 @@ fs.watch = function(filename, options, listener) { handleError((filename = getPathFromURL(filename))); nullCheck(filename); + var persistent = true; + var recursive = false; + var encoding; if (typeof options === 'function') { listener = options; + } else if (options != null) { + options = getOptions(options, emptyObject); + if (options.persistent !== undefined) + persistent = options.persistent; + if (options.recursive !== undefined) + recursive = options.recursive; + encoding = options.encoding; } - options = getOptions(options, {}); - - // Don't make changes directly on options object - options = copyObject(options); - - if (options.persistent === undefined) options.persistent = true; - if (options.recursive === undefined) options.recursive = false; const watcher = new FSWatcher(); - watcher.start(filename, - options.persistent, - options.recursive, - options.encoding); + watcher.start(filename, persistent, recursive, encoding); if (listener) { - watcher.addListener('change', listener); + watcher.on('change', listener); } return watcher; @@ -1653,10 +1700,10 @@ if (isWindows) { } fs.realpathSync = function realpathSync(p, options) { - if (!options) - options = emptyObject; - else + if (options != null) options = getOptions(options, emptyObject); + else + options = emptyObject; if (typeof p !== 'string') { handleError((p = getPathFromURL(p))); if (typeof p !== 'string') @@ -1777,10 +1824,10 @@ fs.realpathSync = function realpathSync(p, options) { fs.realpath = function realpath(p, options, callback) { callback = maybeCallback(typeof options === 'function' ? options : callback); - if (!options) - options = emptyObject; - else + if (options != null && typeof options !== 'function') options = getOptions(options, emptyObject); + else + options = emptyObject; if (typeof p !== 'string') { if (handleError((p = getPathFromURL(p)), callback)) return; @@ -1908,8 +1955,14 @@ fs.realpath = function realpath(p, options, callback) { }; fs.mkdtemp = function(prefix, options, callback) { - callback = makeCallback(typeof options === 'function' ? options : callback); - options = getOptions(options, {}); + var enc; + if (typeof options === 'function') { + callback = makeCallback(options); + } else { + callback = makeCallback(callback); + if (options != null) + enc = getOptions(options, emptyObject).encoding; + } if (!prefix || typeof prefix !== 'string') throw new TypeError('filename prefix is required'); if (!nullCheck(prefix, callback)) { @@ -1919,16 +1972,18 @@ fs.mkdtemp = function(prefix, options, callback) { var req = new FSReqWrap(); req.oncomplete = callback; - binding.mkdtemp(prefix + 'XXXXXX', options.encoding, req); + binding.mkdtemp(prefix + 'XXXXXX', enc, req); }; fs.mkdtempSync = function(prefix, options) { if (!prefix || typeof prefix !== 'string') throw new TypeError('filename prefix is required'); - options = getOptions(options, {}); + var enc; + if (options != null) + enc = getOptions(options, emptyObject).encoding; nullCheck(prefix); - return binding.mkdtemp(prefix + 'XXXXXX', options.encoding); + return binding.mkdtemp(prefix + 'XXXXXX', enc); }; @@ -1952,26 +2007,26 @@ const ReqWrapSym = Symbol('ReqWrapInstance'); const ReadStreamPoolSym = Symbol('ReadStreamPool'); const ReadStreamStartSym = Symbol('ReadStreamStart'); const defaultRSOptions = Object.create(null, { - highWaterMark: { value: 64 * 1024 } + // a little bit bigger buffer and water marks by default + highWaterMark: { value: 64 * 1024, writable: true } }); function ReadStream(path, options, instSymbol) { if (instSymbol !== ReadStreamSym && !(this instanceof ReadStream)) return new ReadStream(path, options, ReadStreamSym); - if (options) { - var options_ = getOptions(options, defaultRSOptions); - if (options_ !== defaultRSOptions && options.highWaterMark === undefined) { - options = copyObject(options_); - // a little bit bigger buffer and water marks by default - options.highWaterMark = 64 * 1024; - } else { - options = options_; - } + var readableOpts = defaultRSOptions; + if (options != null) { + options = getOptions(options, defaultRSOptions); + readableOpts = { + encoding: options.encoding, + highWaterMark: (options.highWaterMark !== undefined ? + options.highWaterMark : 64 * 1024) + }; } else { options = defaultRSOptions; } - Readable.call(this, options); + Readable.call(this, readableOpts); if (typeof path === 'object' && path !== null) handleError((this.path = getPathFromURL(path))); @@ -2163,7 +2218,7 @@ function WriteStream(path, options, instSymbol) { if (instSymbol !== WriteStreamSym && !(this instanceof WriteStream)) return new WriteStream(path, options, WriteStreamSym); - if (options) + if (options != null) options = getOptions(options, defaultWSOptions); else options = defaultWSOptions; From b46177c639e877851e90b47db9e3e160f3637d48 Mon Sep 17 00:00:00 2001 From: Brian White Date: Tue, 28 Mar 2017 19:46:58 -0400 Subject: [PATCH 14/15] test: fix fs tests affected by optimizations --- test/parallel/test-fs-read-stream-err.js | 20 +++++++------- test/parallel/test-fs-sync-fd-leak.js | 11 ++++---- test/parallel/test-fs-write-file-sync.js | 16 +++++------ .../test-fs-write-stream-change-open.js | 23 ++++++++-------- test/parallel/test-fs-write-stream-err.js | 27 +++++++------------ test/parallel/test-npm-install.js | 2 +- 6 files changed, 44 insertions(+), 55 deletions(-) diff --git a/test/parallel/test-fs-read-stream-err.js b/test/parallel/test-fs-read-stream-err.js index a9b4838cf1834f..e4f9953618f1a2 100644 --- a/test/parallel/test-fs-read-stream-err.js +++ b/test/parallel/test-fs-read-stream-err.js @@ -36,23 +36,23 @@ stream.on('error', common.mustCall((err_) => { })); })); -fs.close = common.mustCall((fd_, cb) => { - assert.strictEqual(fd_, stream.fd); - process.nextTick(cb); +process.binding('fs').close = common.mustCall((fd, req) => { + assert.strictEqual(fd, stream.fd); + process.nextTick(req.oncomplete.bind(req)); }); -const read = fs.read; -fs.read = function() { +const read = process.binding('fs').read; +process.binding('fs').read = function() { // first time is ok. - read.apply(fs, arguments); + read.apply(null, arguments); // then it breaks - fs.read = common.mustCall(function() { - const cb = arguments[arguments.length - 1]; + process.binding('fs').read = common.mustCall(function() { + const req = arguments[arguments.length - 1]; process.nextTick(() => { - cb(err); + req.oncomplete(err); }); // and should not be called again! - fs.read = () => { + process.binding('fs').read = () => { throw new Error('BOOM!'); }; }); diff --git a/test/parallel/test-fs-sync-fd-leak.js b/test/parallel/test-fs-sync-fd-leak.js index 7e785ea3a2a82b..4a8abf10a764a3 100644 --- a/test/parallel/test-fs-sync-fd-leak.js +++ b/test/parallel/test-fs-sync-fd-leak.js @@ -24,21 +24,20 @@ require('../common'); const assert = require('assert'); const fs = require('fs'); -// ensure that (read|write|append)FileSync() closes the file descriptor -fs.openSync = function() { +process.binding('fs').open = function() { return 42; }; -fs.closeSync = function(fd) { +// ensure that (read|write|append)FileSync() closes the file descriptor +process.binding('fs').close = function(fd) { assert.strictEqual(fd, 42); close_called++; }; -fs.readSync = function() { +process.binding('fs').read = function() { throw new Error('BAM'); }; -fs.writeSync = function() { +process.binding('fs').writeBuffer = function() { throw new Error('BAM'); }; - process.binding('fs').fstat = function() { throw new Error('BAM'); }; diff --git a/test/parallel/test-fs-write-file-sync.js b/test/parallel/test-fs-write-file-sync.js index d888fcd3a3b7c1..10c83705d2c398 100644 --- a/test/parallel/test-fs-write-file-sync.js +++ b/test/parallel/test-fs-write-file-sync.js @@ -28,12 +28,12 @@ let openCount = 0; let mode; let content; -// Need to hijack fs.open/close to make sure that things -// get closed once they're opened. -fs._openSync = fs.openSync; -fs.openSync = openSync; -fs._closeSync = fs.closeSync; -fs.closeSync = closeSync; +// Need to hijack binding to make sure that things get closed once they're +// opened. +const open = process.binding('fs').open; +process.binding('fs').open = openSync; +const close = process.binding('fs').close; +process.binding('fs').close = closeSync; // Reset the umask for testing process.umask(0o000); @@ -85,10 +85,10 @@ assert.strictEqual(openCount, 0); function openSync() { openCount++; - return fs._openSync.apply(fs, arguments); + return open.apply(null, arguments); } function closeSync() { openCount--; - return fs._closeSync.apply(fs, arguments); + return close.apply(null, arguments); } diff --git a/test/parallel/test-fs-write-stream-change-open.js b/test/parallel/test-fs-write-stream-change-open.js index 50860f2e405f18..a5d7806f6f311d 100644 --- a/test/parallel/test-fs-write-stream-change-open.js +++ b/test/parallel/test-fs-write-stream-change-open.js @@ -30,25 +30,24 @@ const file = path.join(common.tmpDir, 'write.txt'); common.refreshTmpDir(); const stream = fs.WriteStream(file); -const _fs_close = fs.close; -const _fs_open = fs.open; - -// change the fs.open with an identical function after the WriteStream -// has pushed it onto its internal action queue, but before it's -// returned. This simulates AOP-style extension of the fs lib. -fs.open = function() { - return _fs_open.apply(fs, arguments); +const close = process.binding('fs').close; +const open = process.binding('fs').open; + +// change the binding.open with an identical function after the WriteStream +// has pushed it onto its internal action queue, but before it's returned. +process.binding('fs').open = function() { + return open.apply(null, arguments); }; -fs.close = function(fd) { +process.binding('fs').close = function(fd) { assert.ok(fd, 'fs.close must not be called with an undefined fd.'); - fs.close = _fs_close; - fs.open = _fs_open; + process.binding('fs').close = close; + process.binding('fs').open = open; }; stream.write('foo'); stream.end(); process.on('exit', function() { - assert.strictEqual(fs.open, _fs_open); + assert.strictEqual(process.binding('fs').open, open); }); diff --git a/test/parallel/test-fs-write-stream-err.js b/test/parallel/test-fs-write-stream-err.js index 762f6188e0a080..45a2c5aa512d19 100644 --- a/test/parallel/test-fs-write-stream-err.js +++ b/test/parallel/test-fs-write-stream-err.js @@ -31,44 +31,35 @@ const stream = fs.createWriteStream(common.tmpDir + '/out', { }); const err = new Error('BAM'); -const write = fs.write; +const writeBuffer = process.binding('fs').writeBuffer; let writeCalls = 0; -fs.write = function() { +process.binding('fs').writeBuffer = common.mustCall(function() { switch (writeCalls++) { case 0: - console.error('first write'); // first time is ok. - return write.apply(fs, arguments); + return writeBuffer.apply(null, arguments); case 1: // then it breaks - console.error('second write'); - const cb = arguments[arguments.length - 1]; + const req = arguments[arguments.length - 1]; return process.nextTick(function() { - cb(err); + req.oncomplete(err); }); - default: - // and should not be called again! - throw new Error('BOOM!'); } -}; +}, 2); -fs.close = common.mustCall(function(fd_, cb) { - console.error('fs.close', fd_, stream.fd); - assert.strictEqual(fd_, stream.fd); - process.nextTick(cb); +process.binding('fs').close = common.mustCall(function(fd, req) { + assert.strictEqual(fd, stream.fd); + process.nextTick(req.oncomplete.bind(req)); }); stream.on('error', common.mustCall(function(err_) { - console.error('error handler'); assert.strictEqual(stream.fd, null); assert.strictEqual(err_, err); })); stream.write(Buffer.allocUnsafe(256), function() { - console.error('first cb'); stream.write(Buffer.allocUnsafe(256), common.mustCall(function(err_) { - console.error('second cb'); assert.strictEqual(err_, err); })); }); diff --git a/test/parallel/test-npm-install.js b/test/parallel/test-npm-install.js index 99291c5f59a54a..57bb6a89958f38 100644 --- a/test/parallel/test-npm-install.js +++ b/test/parallel/test-npm-install.js @@ -55,7 +55,7 @@ execFile(process.execPath, args, { const code = err.code; const signal = err.signal; stderr += ''; - var msg = `npm install got error code ${code}\nstderr:\n${stderr}`; + let msg = `npm install got error code ${code}\nstderr:\n${stderr}`; assert.strictEqual(code, 0, msg); msg = `unexpected signal: ${signal}\nstderr:\n${stderr}`; assert.strictEqual(signal, null, msg); From 53a7974984fc686f551e30085aadd563a866fc35 Mon Sep 17 00:00:00 2001 From: Brian White Date: Tue, 28 Mar 2017 19:47:17 -0400 Subject: [PATCH 15/15] benchmark: add a few more fs benchmarks --- benchmark/fs/read-stream-throughput-dur.js | 49 ++++++++++++++ benchmark/fs/writeFile.js | 75 ++++++++++++++++++++++ benchmark/fs/writeFileSync.js | 53 +++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 benchmark/fs/read-stream-throughput-dur.js create mode 100644 benchmark/fs/writeFile.js create mode 100644 benchmark/fs/writeFileSync.js diff --git a/benchmark/fs/read-stream-throughput-dur.js b/benchmark/fs/read-stream-throughput-dur.js new file mode 100644 index 00000000000000..745f4805ade647 --- /dev/null +++ b/benchmark/fs/read-stream-throughput-dur.js @@ -0,0 +1,49 @@ +'use strict'; + +if (process.platform === 'win32') + throw new Error('This benchmark is not compatible with Windows'); + +const common = require('../common.js'); +const fs = require('fs'); + +const bench = common.createBenchmark(main, { + type: ['buf', 'asc', 'utf'], + dur: [5], + size: [1024, 4096, 65535, 1024 * 1024] +}); + +function main(conf) { + const dur = +conf.dur * 1000; + const hwm = +conf.size; + var bytes = 0; + var encoding; + + switch (conf.type) { + case 'buf': + encoding = null; + break; + case 'asc': + encoding = 'ascii'; + break; + case 'utf': + encoding = 'utf8'; + break; + default: + throw new Error('invalid type'); + } + + setTimeout(() => { + // MB/sec + bench.end(bytes / (1024 * 1024)); + process.exit(0); + }, dur); + + fs.createReadStream('/dev/zero', { + highWaterMark: hwm, + encoding: encoding + }).on('open', function() { + bench.start(); + }).on('data', function(chunk) { + bytes += chunk.length; + }); +} diff --git a/benchmark/fs/writeFile.js b/benchmark/fs/writeFile.js new file mode 100644 index 00000000000000..175dff53664f05 --- /dev/null +++ b/benchmark/fs/writeFile.js @@ -0,0 +1,75 @@ +'use strict'; + +const path = require('path'); +const common = require('../common.js'); +const filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); +const fs = require('fs'); +const writeFile = fs.writeFile; + +const bench = common.createBenchmark(main, { + dur: [5], + file: [''], + type: ['buf', 'asc', 'utf'], + size: [2, 1024, 64 * 1024, 1024 * 1024] +}); + +function main(conf) { + const dur = +conf.dur * 1000; + const size = +conf.size; + const file = (conf.file.length === 0 ? filename : conf.file); + var writes = 0; + var ended = false; + var encoding; + var data; + + try { fs.unlinkSync(file); } catch (e) {} + process.on('exit', function() { + try { fs.unlinkSync(file); } catch (e) {} + }); + + setTimeout(function() { + bench.end(writes); + ended = true; + }, dur); + + switch (conf.type) { + case 'buf': + data = Buffer.alloc(size, 'b'); + (function() { + function afterWrite(err) { + if (err) + throw err; + if (ended) + return; + ++writes; + writeFile(file, data, afterWrite); + } + bench.start(); + writeFile(file, data, afterWrite); + })(); + return; + case 'asc': + data = new Array(size + 1).join('a'); + encoding = 'ascii'; + break; + case 'utf': + data = new Array(Math.ceil(size / 2) + 1).join('ü'); + encoding = 'utf8'; + break; + default: + throw new Error('invalid type'); + } + + (function() { + function afterWrite(err) { + if (err) + throw err; + if (ended) + return; + ++writes; + writeFile(file, data, encoding, afterWrite); + } + bench.start(); + writeFile(file, data, encoding, afterWrite); + })(); +} diff --git a/benchmark/fs/writeFileSync.js b/benchmark/fs/writeFileSync.js new file mode 100644 index 00000000000000..46a4ddd9646b0a --- /dev/null +++ b/benchmark/fs/writeFileSync.js @@ -0,0 +1,53 @@ +'use strict'; + +const path = require('path'); +const common = require('../common.js'); +const filename = path.resolve(__dirname, '.removeme-benchmark-garbage'); +const fs = require('fs'); +const writeFileSync = fs.writeFileSync; + +const bench = common.createBenchmark(main, { + n: [1e5], + file: [''], + type: ['buf', 'asc', 'utf'], + size: [2, 1024, 64 * 1024, 1024 * 1024] +}); + +function main(conf) { + const n = +conf.n; + const size = +conf.size; + const file = (conf.file.length === 0 ? filename : conf.file); + var encoding; + var data; + var i; + + try { fs.unlinkSync(file); } catch (e) {} + process.on('exit', function() { + try { fs.unlinkSync(file); } catch (e) {} + }); + + switch (conf.type) { + case 'buf': + data = Buffer.alloc(size, 'b'); + bench.start(); + for (i = 0; i < n; ++i) + writeFileSync(file, data); + bench.end(n); + return; + case 'asc': + data = new Array(size + 1).join('a'); + encoding = 'ascii'; + break; + case 'utf': + data = new Array(Math.ceil(size / 2) + 1).join('ü'); + encoding = 'utf8'; + break; + default: + throw new Error('invalid type'); + } + + bench.start(); + for (i = 0; i < n; ++i) + writeFileSync(file, data, encoding); + bench.end(n); +}