From 8bf527eda5f4b611b33e336e23325b28af5aad6b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 9 Oct 2019 01:23:18 +0200 Subject: [PATCH 1/6] 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% --- benchmark/fs/bench-opendir.js | 35 +++++++++++++ lib/internal/fs/dir.js | 16 ++++++ src/node_dir.cc | 92 ++++++++++++++++++++--------------- src/node_dir.h | 4 +- 4 files changed, 105 insertions(+), 42 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..0df0dcd209ec16 --- /dev/null +++ b/benchmark/fs/bench-opendir.js @@ -0,0 +1,35 @@ +'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' ] +}); + +function main({ n, dir, mode }) { + const fullPath = path.resolve(__dirname, '../../', dir); + + (async () => { + 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 { + 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..9920575cfc6fb0 100644 --- a/lib/internal/fs/dir.js +++ b/lib/internal/fs/dir.js @@ -24,6 +24,7 @@ const { const kDirHandle = Symbol('kDirHandle'); const kDirPath = Symbol('kDirPath'); +const kDirBufferedEntries = Symbol('kDirBufferedEntries'); const kDirClosed = Symbol('kDirClosed'); const kDirOptions = Symbol('kDirOptions'); const kDirReadPromisified = Symbol('kDirReadPromisified'); @@ -33,6 +34,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; @@ -59,11 +61,19 @@ class Dir { throw new ERR_INVALID_CALLBACK(callback); } + if (this[kDirBufferedEntries].length > 0) { + const [ name, type ] = this[kDirBufferedEntries].splice(0, 2); + 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 +88,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 +105,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; }; From 6d209fce082ce2ae73dc9555badcdeedb0079b41 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Oct 2019 21:46:10 +0200 Subject: [PATCH 2/6] fixup! fs: buffer dir entries in opendir() --- benchmark/fs/bench-opendir.js | 32 +++++++++++++++----------------- lib/internal/fs/dir.js | 12 ++++++++++-- test/parallel/test-fs-opendir.js | 14 ++++++++++++-- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/benchmark/fs/bench-opendir.js b/benchmark/fs/bench-opendir.js index 0df0dcd209ec16..84d8e6598774db 100644 --- a/benchmark/fs/bench-opendir.js +++ b/benchmark/fs/bench-opendir.js @@ -10,26 +10,24 @@ const bench = common.createBenchmark(main, { mode: [ 'async', 'sync' ] }); -function main({ n, dir, mode }) { +async function main({ n, dir, mode }) { const fullPath = path.resolve(__dirname, '../../', dir); - (async () => { - bench.start(); + 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 { - const dir = fs.opendirSync(fullPath); - while (dir.readSync() !== null) - counter++; - dir.closeSync(); - } + 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 { + const dir = fs.opendirSync(fullPath); + while (dir.readSync() !== null) + counter++; + dir.closeSync(); } + } - bench.end(counter); - })(); + bench.end(counter); } diff --git a/lib/internal/fs/dir.js b/lib/internal/fs/dir.js index 9920575cfc6fb0..2a5f564b832a8c 100644 --- a/lib/internal/fs/dir.js +++ b/lib/internal/fs/dir.js @@ -27,6 +27,7 @@ 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'); @@ -42,7 +43,7 @@ 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); } @@ -51,6 +52,10 @@ class Dir { } read(callback) { + return this[kDirReadImpl](true, callback); + } + + [kDirReadImpl](maybeSync, callback) { if (this[kDirClosed] === true) { throw new ERR_DIR_CLOSED(); } @@ -63,7 +68,10 @@ class Dir { if (this[kDirBufferedEntries].length > 0) { const [ name, type ] = this[kDirBufferedEntries].splice(0, 2); - getDirent(this[kDirPath], name, type, callback); + if (maybeSync) + process.nextTick(getDirent, this[kDirPath], name, type, callback); + else + getDirent(this[kDirPath], name, type, callback); return; } 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 From c66840d0fa641907417ba5d75139e8e16a2d4ad3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Oct 2019 21:49:35 +0200 Subject: [PATCH 3/6] doc: fs dir modifications may not be reflected by dir.read --- doc/api/fs.md | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index d7a4ebcc2e8604..86e76f46f579af 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -362,8 +362,10 @@ Asynchronously read the next directory entry via readdir(3) as an After the read is completed, a `Promise` is returned that will be resolved with an [`fs.Dirent`][], or `null` if there are no more directory entries to read. -_Directory entries returned by this function are in no particular order as -provided by the operating system's underlying directory mechanisms._ +Directory entries returned by this function are in no particular order as +provided by the operating system's underlying directory mechanisms. +Entries added or removed while iterating over the directory may or may not be +included in the iteration results. ### dir.read(callback)