From 216e200fa9ad6ed0ed5a89a1d07200df6b195d5d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 Oct 2019 01:23:18 +0200 Subject: [PATCH] fs: buffer dir entries in opendir() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Read up to 32 directory entries in one batch when `dir.readSync()` or `dir.read()` are called. This increases performance significantly, although it introduces quite a bit of edge case complexity. confidence improvement accuracy (*) (**) (***) fs/bench-opendir.js mode='async' dir='lib' n=100 *** 155.93 % ±30.05% ±40.34% ±53.21% fs/bench-opendir.js mode='async' dir='test/parallel' n=100 *** 479.65 % ±56.81% ±76.47% ±101.32% fs/bench-opendir.js mode='sync' dir='lib' n=100 10.38 % ±14.39% ±19.16% ±24.96% fs/bench-opendir.js mode='sync' dir='test/parallel' n=100 *** 63.13 % ±12.84% ±17.18% ±22.58% PR-URL: https://github.com/nodejs/node/pull/29893 Reviewed-By: Colin Ihrig Reviewed-By: David Carlier Reviewed-By: James M Snell Reviewed-By: Jeremiah Senkpiel --- benchmark/fs/bench-opendir.js | 51 ++++++++++++++++++ lib/internal/fs/dir.js | 27 +++++++++- src/node_dir.cc | 92 ++++++++++++++++++-------------- src/node_dir.h | 4 +- test/parallel/test-fs-opendir.js | 14 ++++- 5 files changed, 143 insertions(+), 45 deletions(-) create mode 100644 benchmark/fs/bench-opendir.js diff --git a/benchmark/fs/bench-opendir.js b/benchmark/fs/bench-opendir.js new file mode 100644 index 00000000000000..419c3a231a850b --- /dev/null +++ b/benchmark/fs/bench-opendir.js @@ -0,0 +1,51 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const path = require('path'); + +const bench = common.createBenchmark(main, { + n: [100], + dir: [ 'lib', 'test/parallel'], + mode: [ 'async', 'sync', 'callback' ] +}); + +async function main({ n, dir, mode }) { + const fullPath = path.resolve(__dirname, '../../', dir); + + bench.start(); + + let counter = 0; + for (let i = 0; i < n; i++) { + if (mode === 'async') { + // eslint-disable-next-line no-unused-vars + for await (const entry of await fs.promises.opendir(fullPath)) + counter++; + } else if (mode === 'callback') { + const dir = await fs.promises.opendir(fullPath); + await new Promise((resolve, reject) => { + function read() { + dir.read((err, entry) => { + if (err) { + reject(err); + } else if (entry === null) { + resolve(dir.close()); + } else { + counter++; + read(); + } + }); + } + + read(); + }); + } else { + const dir = fs.opendirSync(fullPath); + while (dir.readSync() !== null) + counter++; + dir.closeSync(); + } + } + + bench.end(counter); +} diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index 175c632fc79948..fedd7ff8eddb2b 100644 --- a/lib/internal/fs/dir.js +++ b/lib/internal/fs/dir.js @@ -24,8 +24,10 @@ const { const kDirHandle = Symbol('kDirHandle'); const kDirPath = Symbol('kDirPath'); +const kDirBufferedEntries = Symbol('kDirBufferedEntries'); const kDirClosed = Symbol('kDirClosed'); const kDirOptions = Symbol('kDirOptions'); +const kDirReadImpl = Symbol('kDirReadImpl'); const kDirReadPromisified = Symbol('kDirReadPromisified'); const kDirClosePromisified = Symbol('kDirClosePromisified'); @@ -33,6 +35,7 @@ class Dir { constructor(handle, path, options) { if (handle == null) throw new ERR_MISSING_ARGS('handle'); this[kDirHandle] = handle; + this[kDirBufferedEntries] = []; this[kDirPath] = path; this[kDirClosed] = false; @@ -40,7 +43,8 @@ class Dir { encoding: 'utf8' }); - this[kDirReadPromisified] = internalUtil.promisify(this.read).bind(this); + this[kDirReadPromisified] = + internalUtil.promisify(this[kDirReadImpl]).bind(this, false); this[kDirClosePromisified] = internalUtil.promisify(this.close).bind(this); } @@ -49,6 +53,10 @@ class Dir { } read(callback) { + return this[kDirReadImpl](true, callback); + } + + [kDirReadImpl](maybeSync, callback) { if (this[kDirClosed] === true) { throw new ERR_DIR_CLOSED(); } @@ -59,11 +67,22 @@ class Dir { throw new ERR_INVALID_CALLBACK(callback); } + if (this[kDirBufferedEntries].length > 0) { + const [ name, type ] = this[kDirBufferedEntries].splice(0, 2); + if (maybeSync) + process.nextTick(getDirent, this[kDirPath], name, type, callback); + else + getDirent(this[kDirPath], name, type, callback); + return; + } + const req = new FSReqCallback(); req.oncomplete = (err, result) => { if (err || result === null) { return callback(err, result); } + + this[kDirBufferedEntries] = result.slice(2); getDirent(this[kDirPath], result[0], result[1], callback); }; @@ -78,6 +97,11 @@ class Dir { throw new ERR_DIR_CLOSED(); } + if (this[kDirBufferedEntries].length > 0) { + const [ name, type ] = this[kDirBufferedEntries].splice(0, 2); + return getDirent(this[kDirPath], name, type); + } + const ctx = { path: this[kDirPath] }; const result = this[kDirHandle].read( this[kDirOptions].encoding, @@ -90,6 +114,7 @@ class Dir { return result; } + this[kDirBufferedEntries] = result.slice(2); return getDirent(this[kDirPath], result[0], result[1]); } diff --git a/src/node_dir.cc b/src/node_dir.cc index c9df7e67e8323a..382f00f56e627c 100644 --- a/src/node_dir.cc +++ b/src/node_dir.cc @@ -59,8 +59,8 @@ DirHandle::DirHandle(Environment* env, Local obj, uv_dir_t* dir) dir_(dir) { MakeWeak(); - dir_->nentries = 1; - dir_->dirents = &dirent_; + dir_->nentries = arraysize(dirents_); + dir_->dirents = dirents_; } DirHandle* DirHandle::New(Environment* env, uv_dir_t* dir) { @@ -160,7 +160,37 @@ void DirHandle::Close(const FunctionCallbackInfo& args) { } } -void AfterDirReadSingle(uv_fs_t* req) { +static MaybeLocal DirentListToArray( + Environment* env, + uv_dirent_t* ents, + int num, + enum encoding encoding, + Local* err_out) { + MaybeStackBuffer, 96> entries(num * 3); + + // Return an array of all read filenames. + int j = 0; + for (int i = 0; i < num; i++) { + Local filename; + Local error; + const size_t namelen = strlen(ents[i].name); + if (!StringBytes::Encode(env->isolate(), + ents[i].name, + namelen, + encoding, + &error).ToLocal(&filename)) { + *err_out = error; + return MaybeLocal(); + } + + entries[j++] = filename; + entries[j++] = Integer::New(env->isolate(), ents[i].type); + } + + return Array::New(env->isolate(), entries.out(), j); +} + +static void AfterDirRead(uv_fs_t* req) { FSReqBase* req_wrap = FSReqBase::from_req(req); FSReqAfterScope after(req_wrap, req); @@ -170,7 +200,6 @@ void AfterDirReadSingle(uv_fs_t* req) { Environment* env = req_wrap->env(); Isolate* isolate = env->isolate(); - Local error; if (req->result == 0) { // Done @@ -182,26 +211,17 @@ void AfterDirReadSingle(uv_fs_t* req) { uv_dir_t* dir = static_cast(req->ptr); req->ptr = nullptr; - // Single entries are returned without an array wrapper - const uv_dirent_t& ent = dir->dirents[0]; - - MaybeLocal filename = - StringBytes::Encode(isolate, - ent.name, - req_wrap->encoding(), - &error); - if (filename.IsEmpty()) + Local error; + Local js_array; + if (!DirentListToArray(env, + dir->dirents, + req->result, + req_wrap->encoding(), + &error).ToLocal(&js_array)) { return req_wrap->Reject(error); + } - - Local result = Array::New(isolate, 2); - result->Set(env->context(), - 0, - filename.ToLocalChecked()).FromJust(); - result->Set(env->context(), - 1, - Integer::New(isolate, ent.type)).FromJust(); - req_wrap->Resolve(result); + req_wrap->Resolve(js_array); } @@ -217,10 +237,10 @@ void DirHandle::Read(const FunctionCallbackInfo& args) { DirHandle* dir; ASSIGN_OR_RETURN_UNWRAP(&dir, args.Holder()); - FSReqBase* req_wrap_async = static_cast(GetReqWrap(env, args[1])); + FSReqBase* req_wrap_async = GetReqWrap(env, args[1]); if (req_wrap_async != nullptr) { // dir.read(encoding, req) AsyncCall(env, req_wrap_async, args, "readdir", encoding, - AfterDirReadSingle, uv_fs_readdir, dir->dir()); + AfterDirRead, uv_fs_readdir, dir->dir()); } else { // dir.read(encoding, undefined, ctx) CHECK_EQ(argc, 3); FSReqWrapSync req_wrap_sync; @@ -240,28 +260,20 @@ void DirHandle::Read(const FunctionCallbackInfo& args) { } CHECK_GE(req_wrap_sync.req.result, 0); - const uv_dirent_t& ent = dir->dir()->dirents[0]; Local error; - MaybeLocal filename = - StringBytes::Encode(isolate, - ent.name, - encoding, - &error); - if (filename.IsEmpty()) { + Local js_array; + if (!DirentListToArray(env, + dir->dir()->dirents, + req_wrap_sync.req.result, + encoding, + &error).ToLocal(&js_array)) { Local ctx = args[2].As(); - ctx->Set(env->context(), env->error_string(), error).FromJust(); + USE(ctx->Set(env->context(), env->error_string(), error)); return; } - Local result = Array::New(isolate, 2); - result->Set(env->context(), - 0, - filename.ToLocalChecked()).FromJust(); - result->Set(env->context(), - 1, - Integer::New(isolate, ent.type)).FromJust(); - args.GetReturnValue().Set(result); + args.GetReturnValue().Set(js_array); } } diff --git a/src/node_dir.h b/src/node_dir.h index e099fe55107064..03e4a06efcecbe 100644 --- a/src/node_dir.h +++ b/src/node_dir.h @@ -25,7 +25,6 @@ class DirHandle : public AsyncWrap { static void Close(const v8::FunctionCallbackInfo& args); inline uv_dir_t* dir() { return dir_; } - AsyncWrap* GetAsyncWrap() { return this; } void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackFieldWithSize("dir", sizeof(*dir_)); @@ -46,7 +45,8 @@ class DirHandle : public AsyncWrap { void GCClose(); uv_dir_t* dir_; - uv_dirent_t dirent_; + // Up to 32 directory entries are read through a single libuv call. + uv_dirent_t dirents_[32]; bool closing_ = false; bool closed_ = false; }; diff --git a/test/parallel/test-fs-opendir.js b/test/parallel/test-fs-opendir.js index c9a6d657ed890d..f2c5d033451261 100644 --- a/test/parallel/test-fs-opendir.js +++ b/test/parallel/test-fs-opendir.js @@ -58,17 +58,27 @@ const dirclosedError = { // Check the opendir async version fs.opendir(testDir, common.mustCall(function(err, dir) { assert.ifError(err); - dir.read(common.mustCall(function(err, dirent) { + let sync = true; + dir.read(common.mustCall((err, dirent) => { + assert(!sync); assert.ifError(err); // Order is operating / file system dependent assert(files.includes(dirent.name), `'files' should include ${dirent}`); assertDirent(dirent); - dir.close(common.mustCall(function(err) { + let syncInner = true; + dir.read(common.mustCall((err, dirent) => { + assert(!syncInner); assert.ifError(err); + + dir.close(common.mustCall(function(err) { + assert.ifError(err); + })); })); + syncInner = false; })); + sync = false; })); // opendir() on file should throw ENOTDIR