From ac9b36b9ee2dc05a4725a82ec6d561a2f97f52d1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 25 Oct 2019 16:17:07 +0200 Subject: [PATCH 1/7] fs: add `bufferSize` option to `fs.opendir()` Add an option that controls the size of the internal buffer. Fixes: https://github.com/nodejs/node/issues/29941 --- benchmark/fs/bench-opendir.js | 12 +++++++----- doc/api/fs.md | 21 +++++++++++++++++++++ lib/internal/fs/dir.js | 19 ++++++++++++++++--- src/node_dir.cc | 26 ++++++++++++++++++-------- src/node_dir.h | 4 ++-- 5 files changed, 64 insertions(+), 18 deletions(-) diff --git a/benchmark/fs/bench-opendir.js b/benchmark/fs/bench-opendir.js index 419c3a231a850b..20e178d9cc5236 100644 --- a/benchmark/fs/bench-opendir.js +++ b/benchmark/fs/bench-opendir.js @@ -7,10 +7,11 @@ const path = require('path'); const bench = common.createBenchmark(main, { n: [100], dir: [ 'lib', 'test/parallel'], - mode: [ 'async', 'sync', 'callback' ] + mode: [ 'async', 'sync', 'callback' ], + bufferSize: [ 4, 32, 1024 ] }); -async function main({ n, dir, mode }) { +async function main({ n, dir, mode, bufferSize }) { const fullPath = path.resolve(__dirname, '../../', dir); bench.start(); @@ -18,11 +19,12 @@ async function main({ n, dir, mode }) { let counter = 0; for (let i = 0; i < n; i++) { if (mode === 'async') { + const dir = await fs.promises.opendir(fullPath, { bufferSize }); // eslint-disable-next-line no-unused-vars - for await (const entry of await fs.promises.opendir(fullPath)) + for await (const entry of dir) counter++; } else if (mode === 'callback') { - const dir = await fs.promises.opendir(fullPath); + const dir = await fs.promises.opendir(fullPath, { bufferSize }); await new Promise((resolve, reject) => { function read() { dir.read((err, entry) => { @@ -40,7 +42,7 @@ async function main({ n, dir, mode }) { read(); }); } else { - const dir = fs.opendirSync(fullPath); + const dir = fs.opendirSync(fullPath, { bufferSize }); while (dir.readSync() !== null) counter++; dir.closeSync(); diff --git a/doc/api/fs.md b/doc/api/fs.md index 66cc7d7a7ccbd7..e362ef900f5d26 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2625,11 +2625,18 @@ Functions based on `fs.open()` exhibit this behavior as well: ## fs.opendir(path\[, options\], callback) * `path` {string|Buffer|URL} * `options` {Object} * `encoding` {string|null} **Default:** `'utf8'` + * `bufferSize` {number} Size of the internal buffer when reading + directory entries. Higher values lead to better performance but higher + memory usage. **Default:** `32` * `callback` {Function} * `err` {Error} * `dir` {fs.Dir} @@ -2645,11 +2652,18 @@ directory and subsequent read operations. ## fs.opendirSync(path\[, options\]) * `path` {string|Buffer|URL} * `options` {Object} * `encoding` {string|null} **Default:** `'utf8'` + * `bufferSize` {number} Size of the internal buffer when reading + directory entries. Higher values lead to better performance but higher + memory usage. **Default:** `32` * Returns: {fs.Dir} Synchronously open a directory. See opendir(3). @@ -4829,11 +4843,18 @@ a colon, Node.js will open a file system stream, as described by ### fsPromises.opendir(path\[, options\]) * `path` {string|Buffer|URL} * `options` {Object} * `encoding` {string|null} **Default:** `'utf8'` + * `bufferSize` {number} Size of the internal buffer when reading + directory entries. Higher values lead to better performance but higher + memory usage. **Default:** `32` * Returns: {Promise} containing {fs.Dir} Asynchronously open a directory. See opendir(3). diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index fedd7ff8eddb2b..813116faa0b10d 100644 --- a/lib/internal/fs/dir.js +++ b/lib/internal/fs/dir.js @@ -9,6 +9,7 @@ const { codes: { ERR_DIR_CLOSED, ERR_INVALID_CALLBACK, + ERR_INVALID_OPT_VALUE, ERR_MISSING_ARGS } } = require('internal/errors'); @@ -39,9 +40,19 @@ class Dir { this[kDirPath] = path; this[kDirClosed] = false; - this[kDirOptions] = getOptions(options, { - encoding: 'utf8' - }); + this[kDirOptions] = { + bufferSize: 32, + ...getOptions(options, { + encoding: 'utf8' + }) + }; + + if (typeof this[kDirOptions].bufferSize !== 'number' || + !Number.isInteger(this[kDirOptions].bufferSize) || + this[kDirOptions].bufferSize <= 0) { + throw new ERR_INVALID_OPT_VALUE('bufferSize', + this[kDirOptions].bufferSize); + } this[kDirReadPromisified] = internalUtil.promisify(this[kDirReadImpl]).bind(this, false); @@ -88,6 +99,7 @@ class Dir { this[kDirHandle].read( this[kDirOptions].encoding, + this[kDirOptions].bufferSize, req ); } @@ -105,6 +117,7 @@ class Dir { const ctx = { path: this[kDirPath] }; const result = this[kDirHandle].read( this[kDirOptions].encoding, + this[kDirOptions].bufferSize, undefined, ctx ); diff --git a/src/node_dir.cc b/src/node_dir.cc index 382f00f56e627c..ffef27f4521403 100644 --- a/src/node_dir.cc +++ b/src/node_dir.cc @@ -36,6 +36,7 @@ using v8::Isolate; using v8::Local; using v8::MaybeLocal; using v8::Null; +using v8::Number; using v8::Object; using v8::ObjectTemplate; using v8::String; @@ -59,8 +60,8 @@ DirHandle::DirHandle(Environment* env, Local obj, uv_dir_t* dir) dir_(dir) { MakeWeak(); - dir_->nentries = arraysize(dirents_); - dir_->dirents = dirents_; + dir_->nentries = 0; + dir_->dirents = nullptr; } DirHandle* DirHandle::New(Environment* env, uv_dir_t* dir) { @@ -230,22 +231,31 @@ void DirHandle::Read(const FunctionCallbackInfo& args) { Isolate* isolate = env->isolate(); const int argc = args.Length(); - CHECK_GE(argc, 2); + CHECK_GE(argc, 3); const enum encoding encoding = ParseEncoding(isolate, args[0], UTF8); DirHandle* dir; ASSIGN_OR_RETURN_UNWRAP(&dir, args.Holder()); - FSReqBase* req_wrap_async = GetReqWrap(env, args[1]); - if (req_wrap_async != nullptr) { // dir.read(encoding, req) + CHECK(args[1]->IsNumber()); + uint64_t buffer_size = args[1].As()->Value(); + + if (buffer_size != dir->dirents_.size()) { + dir->dirents_.resize(buffer_size); + dir->dir_->nentries = buffer_size; + dir->dir_->dirents = dir->dirents_.data(); + } + + FSReqBase* req_wrap_async = GetReqWrap(env, args[2]); + if (req_wrap_async != nullptr) { // dir.read(encoding, bufferSize, req) AsyncCall(env, req_wrap_async, args, "readdir", encoding, AfterDirRead, uv_fs_readdir, dir->dir()); - } else { // dir.read(encoding, undefined, ctx) - CHECK_EQ(argc, 3); + } else { // dir.read(encoding, bufferSize, undefined, ctx) + CHECK_EQ(argc, 4); FSReqWrapSync req_wrap_sync; FS_DIR_SYNC_TRACE_BEGIN(readdir); - int err = SyncCall(env, args[2], &req_wrap_sync, "readdir", uv_fs_readdir, + int err = SyncCall(env, args[3], &req_wrap_sync, "readdir", uv_fs_readdir, dir->dir()); FS_DIR_SYNC_TRACE_END(readdir); if (err < 0) { diff --git a/src/node_dir.h b/src/node_dir.h index 03e4a06efcecbe..ae6d0eb170d679 100644 --- a/src/node_dir.h +++ b/src/node_dir.h @@ -45,8 +45,8 @@ class DirHandle : public AsyncWrap { void GCClose(); uv_dir_t* dir_; - // Up to 32 directory entries are read through a single libuv call. - uv_dirent_t dirents_[32]; + // Multiple entries are read through a single libuv call. + std::vector dirents_; bool closing_ = false; bool closed_ = false; }; From 0c5404581f9a63ae1ef53221225a114fa3b99a4c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 25 Oct 2019 16:46:20 +0200 Subject: [PATCH 2/7] fixup! fs: add `bufferSize` option to `fs.opendir()` --- doc/api/fs.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index e362ef900f5d26..e77211b3a92f71 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2626,8 +2626,8 @@ Functions based on `fs.open()` exhibit this behavior as well: @@ -2653,8 +2653,8 @@ directory and subsequent read operations. @@ -4844,8 +4844,8 @@ a colon, Node.js will open a file system stream, as described by From f403c2e30a26a86d393510d20a92128a00939ff1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 25 Oct 2019 22:35:06 +0200 Subject: [PATCH 3/7] fixup! fs: add `bufferSize` option to `fs.opendir()` --- doc/api/fs.md | 18 +++++++++--------- test/benchmark/test-benchmark-fs.js | 1 + 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index e77211b3a92f71..a547820fcdc66d 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2634,9 +2634,9 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} * `encoding` {string|null} **Default:** `'utf8'` - * `bufferSize` {number} Size of the internal buffer when reading - directory entries. Higher values lead to better performance but higher - memory usage. **Default:** `32` + * `bufferSize` {number} Number of directory entries that are buffered + internally when reading from the directory. Higher values lead to better + performance but higher memory usage. **Default:** `32` * `callback` {Function} * `err` {Error} * `dir` {fs.Dir} @@ -2661,9 +2661,9 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} * `encoding` {string|null} **Default:** `'utf8'` - * `bufferSize` {number} Size of the internal buffer when reading - directory entries. Higher values lead to better performance but higher - memory usage. **Default:** `32` + * `bufferSize` {number} Number of directory entries that are buffered + internally when reading from the directory. Higher values lead to better + performance but higher memory usage. **Default:** `32` * Returns: {fs.Dir} Synchronously open a directory. See opendir(3). @@ -4852,9 +4852,9 @@ changes: * `path` {string|Buffer|URL} * `options` {Object} * `encoding` {string|null} **Default:** `'utf8'` - * `bufferSize` {number} Size of the internal buffer when reading - directory entries. Higher values lead to better performance but higher - memory usage. **Default:** `32` + * `bufferSize` {number} Number of directory entries that are buffered + internally when reading from the directory. Higher values lead to better + performance but higher memory usage. **Default:** `32` * Returns: {Promise} containing {fs.Dir} Asynchronously open a directory. See opendir(3). diff --git a/test/benchmark/test-benchmark-fs.js b/test/benchmark/test-benchmark-fs.js index 3168e6f0a0ce1f..cf382407235304 100644 --- a/test/benchmark/test-benchmark-fs.js +++ b/test/benchmark/test-benchmark-fs.js @@ -7,6 +7,7 @@ const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); runBenchmark('fs', [ + 'bufferSize=32', 'concurrent=1', 'dir=.github', 'dur=0.1', From 04a26c41d37126786c8cb716f24e5dd75f95b162 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 25 Oct 2019 22:48:58 +0200 Subject: [PATCH 4/7] fixup! fs: add `bufferSize` option to `fs.opendir()` --- test/parallel/test-fs-opendir.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js index f2c5d033451261..61661004783d7b 100644 --- a/test/parallel/test-fs-opendir.js +++ b/test/parallel/test-fs-opendir.js @@ -182,3 +182,19 @@ async function doAsyncIterThrowTest() { await assert.rejects(async () => dir.read(), dirclosedError); } doAsyncIterThrowTest().then(common.mustCall()); + +// Check error thrown on invalid values of bufferSize +for (const bufferSize of [-1, 0, 0.5, 1.5, Infinity, NaN, '', '1', null]) { + assert.throws(() => fs.opendirSync(testDir, { bufferSize }), + { + message: /The value ".*" is invalid for option "bufferSize"/, + code: 'ERR_INVALID_OPT_VALUE' + }); +} + +// Check that it passing a positive integer as bufferSize works +{ + const dir = fs.opendirSync(testDir, { bufferSize: 1024 }); + assertDirent(dir.readSync()); + dir.close(); +} From 2119b4c615d189095e9f3f4890a7a2598d7c6885 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 25 Oct 2019 23:17:14 +0200 Subject: [PATCH 5/7] fixup! fs: add `bufferSize` option to `fs.opendir()` Co-Authored-By: Richard Lau --- test/parallel/test-fs-opendir.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js index 61661004783d7b..7aeeba096438b0 100644 --- a/test/parallel/test-fs-opendir.js +++ b/test/parallel/test-fs-opendir.js @@ -192,7 +192,7 @@ for (const bufferSize of [-1, 0, 0.5, 1.5, Infinity, NaN, '', '1', null]) { }); } -// Check that it passing a positive integer as bufferSize works +// Check that passing a positive integer as bufferSize works { const dir = fs.opendirSync(testDir, { bufferSize: 1024 }); assertDirent(dir.readSync()); From dc990cf02077006c5d8716d750dd6b21a226cb77 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 27 Oct 2019 01:49:32 +0200 Subject: [PATCH 6/7] fixup! fs: add `bufferSize` option to `fs.opendir()` --- lib/internal/fs/dir.js | 10 ++++------ test/parallel/test-fs-opendir.js | 19 +++++++++++++------ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index 813116faa0b10d..538b6a9c8dbee8 100644 --- a/lib/internal/fs/dir.js +++ b/lib/internal/fs/dir.js @@ -22,6 +22,9 @@ const { getValidatedPath, handleErrorFromBinding } = require('internal/fs/utils'); +const { + validateUint32 +} = require('internal/validators'); const kDirHandle = Symbol('kDirHandle'); const kDirPath = Symbol('kDirPath'); @@ -47,12 +50,7 @@ class Dir { }) }; - if (typeof this[kDirOptions].bufferSize !== 'number' || - !Number.isInteger(this[kDirOptions].bufferSize) || - this[kDirOptions].bufferSize <= 0) { - throw new ERR_INVALID_OPT_VALUE('bufferSize', - this[kDirOptions].bufferSize); - } + validateUint32(this[kDirOptions].bufferSize, 'options.bufferSize', true); this[kDirReadPromisified] = internalUtil.promisify(this[kDirReadImpl]).bind(this, false); diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js index 7aeeba096438b0..05fded527fe7f1 100644 --- a/test/parallel/test-fs-opendir.js +++ b/test/parallel/test-fs-opendir.js @@ -184,12 +184,19 @@ async function doAsyncIterThrowTest() { doAsyncIterThrowTest().then(common.mustCall()); // Check error thrown on invalid values of bufferSize -for (const bufferSize of [-1, 0, 0.5, 1.5, Infinity, NaN, '', '1', null]) { - assert.throws(() => fs.opendirSync(testDir, { bufferSize }), - { - message: /The value ".*" is invalid for option "bufferSize"/, - code: 'ERR_INVALID_OPT_VALUE' - }); +for (const bufferSize of [-1, 0, 0.5, 1.5, Infinity, NaN]) { + assert.throws( + () => fs.opendirSync(testDir, { bufferSize }), + { + code: 'ERR_OUT_OF_RANGE' + }); +} +for (const bufferSize of ['', '1', null]) { + assert.throws( + () => fs.opendirSync(testDir, { bufferSize }), + { + code: 'ERR_INVALID_ARG_TYPE' + }); } // Check that passing a positive integer as bufferSize works From 51f44298f2b69d7e9cc5164b23b4a2875734f4ee Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 28 Oct 2019 12:28:03 +0100 Subject: [PATCH 7/7] fixup! fixup! fs: add `bufferSize` option to `fs.opendir()` --- lib/internal/fs/dir.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index 538b6a9c8dbee8..90ab31fe5abc73 100644 --- a/lib/internal/fs/dir.js +++ b/lib/internal/fs/dir.js @@ -9,7 +9,6 @@ const { codes: { ERR_DIR_CLOSED, ERR_INVALID_CALLBACK, - ERR_INVALID_OPT_VALUE, ERR_MISSING_ARGS } } = require('internal/errors');