Skip to content

Commit 216e200

Browse files
addaleaxtargos
addaleax
authored and
targos
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 e16e3d5 commit 216e200

5 files changed

Lines changed: 143 additions & 45 deletions

File tree

‎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+
constcommon=require('../common');
4+
constfs=require('fs');
5+
constpath=require('path');
6+
7+
constbench=common.createBenchmark(main,{
8+
n: [100],
9+
dir: ['lib','test/parallel'],
10+
mode: ['async','sync','callback']
11+
});
12+
13+
asyncfunctionmain({ n, dir, mode }){
14+
constfullPath=path.resolve(__dirname,'../../',dir);
15+
16+
bench.start();
17+
18+
letcounter=0;
19+
for(leti=0;i<n;i++){
20+
if(mode==='async'){
21+
// eslint-disable-next-line no-unused-vars
22+
forawait(constentryofawaitfs.promises.opendir(fullPath))
23+
counter++;
24+
}elseif(mode==='callback'){
25+
constdir=awaitfs.promises.opendir(fullPath);
26+
awaitnewPromise((resolve,reject)=>{
27+
functionread(){
28+
dir.read((err,entry)=>{
29+
if(err){
30+
reject(err);
31+
}elseif(entry===null){
32+
resolve(dir.close());
33+
}else{
34+
counter++;
35+
read();
36+
}
37+
});
38+
}
39+
40+
read();
41+
});
42+
}else{
43+
constdir=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
constkDirHandle=Symbol('kDirHandle');
2626
constkDirPath=Symbol('kDirPath');
27+
constkDirBufferedEntries=Symbol('kDirBufferedEntries');
2728
constkDirClosed=Symbol('kDirClosed');
2829
constkDirOptions=Symbol('kDirOptions');
30+
constkDirReadImpl=Symbol('kDirReadImpl');
2931
constkDirReadPromisified=Symbol('kDirReadPromisified');
3032
constkDirClosePromisified=Symbol('kDirClosePromisified');
3133

3234
classDir{
3335
constructor(handle,path,options){
3436
if(handle==null)thrownewERR_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+
returnthis[kDirReadImpl](true,callback);
57+
}
58+
59+
[kDirReadImpl](maybeSync,callback){
5260
if(this[kDirClosed]===true){
5361
thrownewERR_DIR_CLOSED();
5462
}
@@ -59,11 +67,22 @@ class Dir {
5967
thrownewERR_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
constreq=newFSReqCallback();
6380
req.oncomplete=(err,result)=>{
6481
if(err||result===null){
6582
returncallback(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
thrownewERR_DIR_CLOSED();
7998
}
8099

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

117+
this[kDirBufferedEntries]=result.slice(2);
93118
returngetDirent(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-
voidAfterDirReadSingle(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+
constsize_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+
returnArray::New(env->isolate(), entries.out(), j);
191+
}
192+
193+
staticvoidAfterDirRead(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-
constuv_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-
constuv_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
staticvoidClose(const v8::FunctionCallbackInfo<Value>& args);
2626

2727
inlineuv_dir_t* dir() { return dir_; }
28-
AsyncWrap* GetAsyncWrap() { returnthis; }
2928

3029
voidMemoryInfo(MemoryTracker* tracker) constoverride {
3130
tracker->TrackFieldWithSize("dir", sizeof(*dir_));
@@ -46,7 +45,8 @@ class DirHandle : public AsyncWrap {
4645
voidGCClose();
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+
letsync=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+
letsyncInner=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)