fs: add bufferSize option to fs.opendir()

Add an option that controls the size of the internal buffer. Fixes: #29941 PR-URL: #30114 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
nodejs · Oct 31, 2019 · 07b5584 · 07b5584
1 parent e4ab6fc
commit 07b5584
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 18 deletions.
diff --git a/benchmark/fs/bench-opendir.js b/benchmark/fs/bench-opendir.js
@@ -7,22 +7,24 @@ 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();
 
   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
@@ -2625,11 +2625,18 @@ Functions based on `fs.open()` exhibit this behavior as well:
 ## fs.opendir(path\[, options\], callback)
 <!-- YAML
 added: v12.12.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/30114
+    description: The `bufferSize` option was introduced.
 -->
 
 * `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\])
 <!-- YAML
 added: v12.12.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/30114
+    description: The `bufferSize` option was introduced.
 -->
 
 * `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\])
 <!-- YAML
 added: v12.12.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/30114
+    description: The `bufferSize` option was introduced.
 -->
 
 * `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
@@ -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
@@ -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<Object> 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<Value>& 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<Number>()->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
@@ -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<uv_dirent_t> dirents_;
   bool closing_ = false;
   bool closed_ = false;
 };

diff --git 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
@@ -182,3 +182,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();
+}