Skip to content

Commit 5c93aab

Browse files
addaleaxaddaleax
committed
fs: buffer dir entries in opendir()
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: #29893 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
1 parent c0305af commit 5c93aab

File tree

5 files changed

+143
-45
lines changed

5 files changed

+143
-45
lines changed

‎benchmark/fs/bench-opendir.js‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const fs = require('fs');
5+
const path = require('path');
6+
7+
const bench = common.createBenchmark(main, {
8+
n: [100],
9+
dir: [ 'lib', 'test/parallel'],
10+
mode: [ 'async', 'sync', 'callback' ]
11+
});
12+
13+
async function main({ n, dir, mode }) {
14+
const fullPath = path.resolve(__dirname, '../../', dir);
15+
16+
bench.start();
17+
18+
let counter = 0;
19+
for (let i = 0; i < n; i++) {
20+
if (mode === 'async') {
21+
// eslint-disable-next-line no-unused-vars
22+
for await (const entry of await fs.promises.opendir(fullPath))
23+
counter++;
24+
} else if (mode === 'callback') {
25+
const dir = await fs.promises.opendir(fullPath);
26+
await new Promise((resolve, reject) => {
27+
function read() {
28+
dir.read((err, entry) => {
29+
if (err) {
30+
reject(err);
31+
} else if (entry === null) {
32+
resolve(dir.close());
33+
} else {
34+
counter++;
35+
read();
36+
}
37+
});
38+
}
39+
40+
read();
41+
});
42+
} else {
43+
const dir = fs.opendirSync(fullPath);
44+
while (dir.readSync() !== null)
45+
counter++;
46+
dir.closeSync();
47+
}
48+
}
49+
50+
bench.end(counter);
51+
}

‎lib/internal/fs/dir.js‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,23 +24,27 @@ const {
2424

2525
const kDirHandle = Symbol('kDirHandle');
2626
const kDirPath = Symbol('kDirPath');
27+
const kDirBufferedEntries = Symbol('kDirBufferedEntries');
2728
const kDirClosed = Symbol('kDirClosed');
2829
const kDirOptions = Symbol('kDirOptions');
30+
const kDirReadImpl = Symbol('kDirReadImpl');
2931
const kDirReadPromisified = Symbol('kDirReadPromisified');
3032
const kDirClosePromisified = Symbol('kDirClosePromisified');
3133

3234
class Dir {
3335
constructor(handle, path, options) {
3436
if (handle == null) throw new ERR_MISSING_ARGS('handle');
3537
this[kDirHandle] = handle;
38+
this[kDirBufferedEntries] = [];
3639
this[kDirPath] = path;
3740
this[kDirClosed] = false;
3841

3942
this[kDirOptions] = getOptions(options, {
4043
encoding: 'utf8'
4144
});
4245

43-
this[kDirReadPromisified] = internalUtil.promisify(this.read).bind(this);
46+
this[kDirReadPromisified] =
47+
internalUtil.promisify(this[kDirReadImpl]).bind(this, false);
4448
this[kDirClosePromisified] = internalUtil.promisify(this.close).bind(this);
4549
}
4650

@@ -49,6 +53,10 @@ class Dir {
4953
}
5054

5155
read(callback) {
56+
return this[kDirReadImpl](true, callback);
57+
}
58+
59+
[kDirReadImpl](maybeSync, callback) {
5260
if (this[kDirClosed] === true) {
5361
throw new ERR_DIR_CLOSED();
5462
}
@@ -59,11 +67,22 @@ class Dir {
5967
throw new ERR_INVALID_CALLBACK(callback);
6068
}
6169

70+
if (this[kDirBufferedEntries].length > 0) {
71+
const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
72+
if (maybeSync)
73+
process.nextTick(getDirent, this[kDirPath], name, type, callback);
74+
else
75+
getDirent(this[kDirPath], name, type, callback);
76+
return;
77+
}
78+
6279
const req = new FSReqCallback();
6380
req.oncomplete = (err, result) => {
6481
if (err || result === null) {
6582
return callback(err, result);
6683
}
84+
85+
this[kDirBufferedEntries] = result.slice(2);
6786
getDirent(this[kDirPath], result[0], result[1], callback);
6887
};
6988

@@ -78,6 +97,11 @@ class Dir {
7897
throw new ERR_DIR_CLOSED();
7998
}
8099

100+
if (this[kDirBufferedEntries].length > 0) {
101+
const [ name, type ] = this[kDirBufferedEntries].splice(0, 2);
102+
return getDirent(this[kDirPath], name, type);
103+
}
104+
81105
const ctx = { path: this[kDirPath] };
82106
const result = this[kDirHandle].read(
83107
this[kDirOptions].encoding,
@@ -90,6 +114,7 @@ class Dir {
90114
return result;
91115
}
92116

117+
this[kDirBufferedEntries] = result.slice(2);
93118
return getDirent(this[kDirPath], result[0], result[1]);
94119
}
95120

‎src/node_dir.cc‎

Lines changed: 52 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ DirHandle::DirHandle(Environment* env, Local<Object> obj, uv_dir_t* dir)
5959
dir_(dir) {
6060
MakeWeak();
6161

62-
dir_->nentries = 1;
63-
dir_->dirents = &dirent_;
62+
dir_->nentries = arraysize(dirents_);
63+
dir_->dirents = dirents_;
6464
}
6565

6666
DirHandle* DirHandle::New(Environment* env, uv_dir_t* dir) {
@@ -160,7 +160,37 @@ void DirHandle::Close(const FunctionCallbackInfo<Value>& args) {
160160
}
161161
}
162162

163-
void AfterDirReadSingle(uv_fs_t* req) {
163+
static MaybeLocal<Array> DirentListToArray(
164+
Environment* env,
165+
uv_dirent_t* ents,
166+
int num,
167+
enum encoding encoding,
168+
Local<Value>* err_out) {
169+
MaybeStackBuffer<Local<Value>, 96> entries(num * 3);
170+
171+
// Return an array of all read filenames.
172+
int j = 0;
173+
for (int i = 0; i < num; i++) {
174+
Local<Value> filename;
175+
Local<Value> error;
176+
const size_t namelen = strlen(ents[i].name);
177+
if (!StringBytes::Encode(env->isolate(),
178+
ents[i].name,
179+
namelen,
180+
encoding,
181+
&error).ToLocal(&filename)) {
182+
*err_out = error;
183+
return MaybeLocal<Array>();
184+
}
185+
186+
entries[j++] = filename;
187+
entries[j++] = Integer::New(env->isolate(), ents[i].type);
188+
}
189+
190+
return Array::New(env->isolate(), entries.out(), j);
191+
}
192+
193+
static void AfterDirRead(uv_fs_t* req) {
164194
FSReqBase* req_wrap = FSReqBase::from_req(req);
165195
FSReqAfterScope after(req_wrap, req);
166196

@@ -170,7 +200,6 @@ void AfterDirReadSingle(uv_fs_t* req) {
170200

171201
Environment* env = req_wrap->env();
172202
Isolate* isolate = env->isolate();
173-
Local<Value> error;
174203

175204
if (req->result == 0) {
176205
// Done
@@ -182,26 +211,17 @@ void AfterDirReadSingle(uv_fs_t* req) {
182211
uv_dir_t* dir = static_cast<uv_dir_t*>(req->ptr);
183212
req->ptr = nullptr;
184213

185-
// Single entries are returned without an array wrapper
186-
const uv_dirent_t& ent = dir->dirents[0];
187-
188-
MaybeLocal<Value> filename =
189-
StringBytes::Encode(isolate,
190-
ent.name,
191-
req_wrap->encoding(),
192-
&error);
193-
if (filename.IsEmpty())
214+
Local<Value> error;
215+
Local<Array> js_array;
216+
if (!DirentListToArray(env,
217+
dir->dirents,
218+
req->result,
219+
req_wrap->encoding(),
220+
&error).ToLocal(&js_array)) {
194221
return req_wrap->Reject(error);
222+
}
195223

196-
197-
Local<Array> result = Array::New(isolate, 2);
198-
result->Set(env->context(),
199-
0,
200-
filename.ToLocalChecked()).FromJust();
201-
result->Set(env->context(),
202-
1,
203-
Integer::New(isolate, ent.type)).FromJust();
204-
req_wrap->Resolve(result);
224+
req_wrap->Resolve(js_array);
205225
}
206226

207227

@@ -217,10 +237,10 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
217237
DirHandle* dir;
218238
ASSIGN_OR_RETURN_UNWRAP(&dir, args.Holder());
219239

220-
FSReqBase* req_wrap_async = static_cast<FSReqBase*>(GetReqWrap(env, args[1]));
240+
FSReqBase* req_wrap_async = GetReqWrap(env, args[1]);
221241
if (req_wrap_async != nullptr) { // dir.read(encoding, req)
222242
AsyncCall(env, req_wrap_async, args, "readdir", encoding,
223-
AfterDirReadSingle, uv_fs_readdir, dir->dir());
243+
AfterDirRead, uv_fs_readdir, dir->dir());
224244
} else { // dir.read(encoding, undefined, ctx)
225245
CHECK_EQ(argc, 3);
226246
FSReqWrapSync req_wrap_sync;
@@ -240,28 +260,20 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
240260
}
241261

242262
CHECK_GE(req_wrap_sync.req.result, 0);
243-
const uv_dirent_t& ent = dir->dir()->dirents[0];
244263

245264
Local<Value> error;
246-
MaybeLocal<Value> filename =
247-
StringBytes::Encode(isolate,
248-
ent.name,
249-
encoding,
250-
&error);
251-
if (filename.IsEmpty()) {
265+
Local<Array> js_array;
266+
if (!DirentListToArray(env,
267+
dir->dir()->dirents,
268+
req_wrap_sync.req.result,
269+
encoding,
270+
&error).ToLocal(&js_array)) {
252271
Local<Object> ctx = args[2].As<Object>();
253-
ctx->Set(env->context(), env->error_string(), error).FromJust();
272+
USE(ctx->Set(env->context(), env->error_string(), error));
254273
return;
255274
}
256275

257-
Local<Array> result = Array::New(isolate, 2);
258-
result->Set(env->context(),
259-
0,
260-
filename.ToLocalChecked()).FromJust();
261-
result->Set(env->context(),
262-
1,
263-
Integer::New(isolate, ent.type)).FromJust();
264-
args.GetReturnValue().Set(result);
276+
args.GetReturnValue().Set(js_array);
265277
}
266278
}
267279

‎src/node_dir.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ class DirHandle : public AsyncWrap {
2525
static void Close(const v8::FunctionCallbackInfo<Value>& args);
2626

2727
inline uv_dir_t* dir() { return dir_; }
28-
AsyncWrap* GetAsyncWrap() { return this; }
2928

3029
void MemoryInfo(MemoryTracker* tracker) const override {
3130
tracker->TrackFieldWithSize("dir", sizeof(*dir_));
@@ -46,7 +45,8 @@ class DirHandle : public AsyncWrap {
4645
void GCClose();
4746

4847
uv_dir_t* dir_;
49-
uv_dirent_t dirent_;
48+
// Up to 32 directory entries are read through a single libuv call.
49+
uv_dirent_t dirents_[32];
5050
bool closing_ = false;
5151
bool closed_ = false;
5252
};

‎test/parallel/test-fs-opendir.js‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,17 +58,27 @@ const dirclosedError = {
5858
// Check the opendir async version
5959
fs.opendir(testDir, common.mustCall(function(err, dir) {
6060
assert.ifError(err);
61-
dir.read(common.mustCall(function(err, dirent) {
61+
let sync = true;
62+
dir.read(common.mustCall((err, dirent) => {
63+
assert(!sync);
6264
assert.ifError(err);
6365

6466
// Order is operating / file system dependent
6567
assert(files.includes(dirent.name), `'files' should include ${dirent}`);
6668
assertDirent(dirent);
6769

68-
dir.close(common.mustCall(function(err) {
70+
let syncInner = true;
71+
dir.read(common.mustCall((err, dirent) => {
72+
assert(!syncInner);
6973
assert.ifError(err);
74+
75+
dir.close(common.mustCall(function(err) {
76+
assert.ifError(err);
77+
}));
7078
}));
79+
syncInner = false;
7180
}));
81+
sync = false;
7282
}));
7383

7484
// opendir() on file should throw ENOTDIR

0 commit comments

Comments
 (0)