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 <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Author: addaleax

benchmark/fs/bench-opendir.js

@@ -7,22 +7,24 @@ const path = require('path');
 constbench=common.createBenchmark(main,{
 n: [100],
 dir: ['lib','test/parallel'],
-mode: ['async','sync','callback']
+mode: ['async','sync','callback'],
+bufferSize: [4,32,1024]
 });
 
-asyncfunctionmain({ n, dir, mode }){
+asyncfunctionmain({ n, dir, mode, bufferSize}){
 constfullPath=path.resolve(__dirname,'../../',dir);
 
 bench.start();
 
 letcounter=0;
 for(leti=0;i<n;i++){
 if(mode==='async'){
+constdir=awaitfs.promises.opendir(fullPath,{ bufferSize });
 // eslint-disable-next-line no-unused-vars
-forawait(constentryofawaitfs.promises.opendir(fullPath))
+forawait(constentryofdir)
 counter++;
 }elseif(mode==='callback'){
-constdir=awaitfs.promises.opendir(fullPath);
+constdir=awaitfs.promises.opendir(fullPath,{ bufferSize });
 awaitnewPromise((resolve,reject)=>{
 functionread(){
 dir.read((err,entry)=>{
@@ -40,7 +42,7 @@ async function main({ n, dir, mode }) {
 read();
 });
 }else{
-constdir=fs.opendirSync(fullPath);
+constdir=fs.opendirSync(fullPath,{ bufferSize });
 while(dir.readSync()!==null)
 counter++;
 dir.closeSync();

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).

lib/internal/fs/dir.js

@@ -21,6 +21,9 @@ const {
  getValidatedPath,
  handleErrorFromBinding
 }=require('internal/fs/utils');
+const{
+ validateUint32
+}=require('internal/validators');
 
 constkDirHandle=Symbol('kDirHandle');
 constkDirPath=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 {
 constctx={path: this[kDirPath]};
 constresult=this[kDirHandle].read(
 this[kDirOptions].encoding,
+this[kDirOptions].bufferSize,
 undefined,
 ctx
 );

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();
 
 constint argc = args.Length();
-CHECK_GE(argc, 2);
+CHECK_GE(argc, 3);
 
 constenum 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) {

src/node_dir.h

@@ -45,8 +45,8 @@ class DirHandle : public AsyncWrap {
 voidGCClose();
 
 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;
 };

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',

test/parallel/test-fs-opendir.js

@@ -186,3 +186,26 @@ async function doAsyncIterThrowTest() {
 awaitassert.rejects(async()=>dir.read(),dirclosedError);
 }
 doAsyncIterThrowTest().then(common.mustCall());
+
+// Check error thrown on invalid values of bufferSize
+for(constbufferSizeof[-1,0,0.5,1.5,Infinity,NaN]){
+assert.throws(
+()=>fs.opendirSync(testDir,{ bufferSize }),
+{
+code: 'ERR_OUT_OF_RANGE'
+});
+}
+for(constbufferSizeof['','1',null]){
+assert.throws(
+()=>fs.opendirSync(testDir,{ bufferSize }),
+{
+code: 'ERR_INVALID_ARG_TYPE'
+});
+}
+
+// Check that passing a positive integer as bufferSize works
+{
+constdir=fs.opendirSync(testDir,{bufferSize: 1024});
+assertDirent(dir.readSync());
+dir.close();
+}