From 785aa86b944b8b0055048395c57a2092266f9719 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 25 Oct 2019 16:17:07 +0200 Subject: [PATCH] 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 PR-URL: https://github.com/nodejs/node/pull/30114 Reviewed-By: Richard Lau Reviewed-By: Rich Trott Reviewed-By: James M Snell Reviewed-By: Colin Ihrig --- benchmark/fs/bench-opendir.js | 12 +++++++----- doc/api/fs.md | 21 +++++++++++++++++++++ lib/internal/fs/dir.js | 16 +++++++++++++--- src/node_dir.cc | 26 ++++++++++++++++++-------- src/node_dir.h | 4 ++-- test/benchmark/test-benchmark-fs.js | 1 + test/parallel/test-fs-opendir.js | 23 +++++++++++++++++++++++ 7 files changed, 85 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 33d1d072e0e05b..c86fd74e82be46 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} 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} @@ -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} 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). @@ -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} 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/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index fedd7ff8eddb2b..90ab31fe5abc73 100644 --- a/lib/internal/fs/dir.js +++ b/lib/internal/fs/dir.js @@ -21,6 +21,9 @@ const { getValidatedPath, handleErrorFromBinding } = require('internal/fs/utils'); +const { + validateUint32 +} = require('internal/validators'); const kDirHandle = Symbol('kDirHandle'); const kDirPath = Symbol('kDirPath'); @@ -39,9 +42,14 @@ class Dir { this[kDirPath] = path; this[kDirClosed] = false; - this[kDirOptions] = getOptions(options, { - encoding: 'utf8' - }); + this[kDirOptions] = { + bufferSize: 32, + ...getOptions(options, { + encoding: 'utf8' + }) + }; + + validateUint32(this[kDirOptions].bufferSize, 'options.bufferSize', true); this[kDirReadPromisified] = internalUtil.promisify(this[kDirReadImpl]).bind(this, false); @@ -88,6 +96,7 @@ class Dir { this[kDirHandle].read( this[kDirOptions].encoding, + this[kDirOptions].bufferSize, req ); } @@ -105,6 +114,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 4b9ff9b807e1b6..9d145131beb94c 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; }; diff --git a/test/benchmark/test-benchmark-fs.js b/test/benchmark/test-benchmark-fs.js index 8811b8c8710b9d..a072124ef85bee 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', diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js index c979dda589eac6..04e6e890c33b39 100644 --- a/test/parallel/test-fs-opendir.js +++ b/test/parallel/test-fs-opendir.js @@ -186,3 +186,26 @@ 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]) { + 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 +{ + const dir = fs.opendirSync(testDir, { bufferSize: 1024 }); + assertDirent(dir.readSync()); + dir.close(); +}