Skip to content

Commit

Permalink
fs: buffer dir entries in opendir()
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
addaleax authored and targos committed Nov 10, 2019
1 parent e16e3d5 commit 216e200
Show file tree
Hide file tree
Showing 5 changed files with 143 additions and 45 deletions.
51 changes: 51 additions & 0 deletions benchmark/fs/bench-opendir.js
Original file line number Diff line number Diff line change
@@ -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);
}
27 changes: 26 additions & 1 deletion lib/internal/fs/dir.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,27 @@ 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');

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;

this[kDirOptions] = getOptions(options, {
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);
}

Expand All @@ -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();
}
Expand All @@ -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);
};

Expand All @@ -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,
Expand All @@ -90,6 +114,7 @@ class Dir {
return result;
}

this[kDirBufferedEntries] = result.slice(2);
return getDirent(this[kDirPath], result[0], result[1]);
}

Expand Down
92 changes: 52 additions & 40 deletions src/node_dir.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ DirHandle::DirHandle(Environment* env, Local<Object> 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) {
Expand Down Expand Up @@ -160,7 +160,37 @@ void DirHandle::Close(const FunctionCallbackInfo<Value>& args) {
}
}

void AfterDirReadSingle(uv_fs_t* req) {
static MaybeLocal<Array> DirentListToArray(
Environment* env,
uv_dirent_t* ents,
int num,
enum encoding encoding,
Local<Value>* err_out) {
MaybeStackBuffer<Local<Value>, 96> entries(num * 3);

// Return an array of all read filenames.
int j = 0;
for (int i = 0; i < num; i++) {
Local<Value> filename;
Local<Value> 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<Array>();
}

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

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

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

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

// Single entries are returned without an array wrapper
const uv_dirent_t& ent = dir->dirents[0];

MaybeLocal<Value> filename =
StringBytes::Encode(isolate,
ent.name,
req_wrap->encoding(),
&error);
if (filename.IsEmpty())
Local<Value> error;
Local<Array> js_array;
if (!DirentListToArray(env,
dir->dirents,
req->result,
req_wrap->encoding(),
&error).ToLocal(&js_array)) {
return req_wrap->Reject(error);
}


Local<Array> 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);
}


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

FSReqBase* req_wrap_async = static_cast<FSReqBase*>(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;
Expand All @@ -240,28 +260,20 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
}

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

Local<Value> error;
MaybeLocal<Value> filename =
StringBytes::Encode(isolate,
ent.name,
encoding,
&error);
if (filename.IsEmpty()) {
Local<Array> js_array;
if (!DirentListToArray(env,
dir->dir()->dirents,
req_wrap_sync.req.result,
encoding,
&error).ToLocal(&js_array)) {
Local<Object> ctx = args[2].As<Object>();
ctx->Set(env->context(), env->error_string(), error).FromJust();
USE(ctx->Set(env->context(), env->error_string(), error));
return;
}

Local<Array> 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);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/node_dir.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ class DirHandle : public AsyncWrap {
static void Close(const v8::FunctionCallbackInfo<Value>& args);

inline uv_dir_t* dir() { return dir_; }
AsyncWrap* GetAsyncWrap() { return this; }

void MemoryInfo(MemoryTracker* tracker) const override {
tracker->TrackFieldWithSize("dir", sizeof(*dir_));
Expand All @@ -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;
};
Expand Down
14 changes: 12 additions & 2 deletions test/parallel/test-fs-opendir.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 216e200

Please sign in to comment.