8000 fs: improve error performance of `fs.dir` · nodejs/node@128e514 · GitHub
[go: up one dir, main page]
Skip to content

Commit 128e514

Browse files
anonrigaduh95
anonrig
authored and
aduh95
committed
fs: improve error performance of fs.dir
PR-URL: #53667 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
1 parent 9fd976b commit 128e514

File tree

3 files changed

+43
-54
lines changed

3 files changed

+43
-54
lines changed

‎lib/internal/fs/dir.js‎

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ const {
2626
getDirent,
2727
getOptions,
2828
getValidatedPath,
29-
handleErrorFromBinding,
3029
} = require('internal/fs/utils');
3130
const {
3231
validateFunction,
@@ -155,27 +154,26 @@ class Dir {
155154

156155
readSyncRecursive(dirent) {
157156
const path = pathModule.join(dirent.parentPath, dirent.name);
158-
const ctx = { path };
159157
const handle = dirBinding.opendir(
160158
pathModule.toNamespacedPath(path),
161159
this.#options.encoding,
162-
undefined,
163-
ctx,
164160
);
165-
handleErrorFromBinding(ctx);
161+
162+
// Terminate early, since it's already thrown.
163+
if (handle === undefined) {
164+
return;
165+
}
166+
166167
const result = handle.read(
167168
this.#options.encoding,
168169
this.#options.bufferSize,
169-
undefined,
170-
ctx,
171170
);
172171

173172
if (result) {
174173
this.processReadResult(path, result);
175174
}
176175

177-
handle.close(undefined, ctx);
178-
handleErrorFromBinding(ctx);
176+
handle.close();
179177
}
180178

181179
readSync() {
@@ -195,14 +193,10 @@ class Dir {
195193
return dirent;
196194
}
197195

198-
const ctx = { path: this.#path };
199196
const result = this.#handle.read(
200197
this.#options.encoding,
201198
this.#options.bufferSize,
202-
undefined,
203-
ctx,
204199
);
205-
handleErrorFromBinding(ctx);
206200

207201
if (result === null) {
208202
return result;
@@ -257,10 +251,7 @@ class Dir {
257251
}
258252

259253
this.#closed = true;
260-
const ctx = { path: this.#path };
261-
const result = this.#handle.close(undefined, ctx);
262-
handleErrorFromBinding(ctx);
263-
return result;
254+
this.#handle.close();
264255
}
265256

266257
async* entries() {

‎src/node_dir.cc‎

Lines changed: 30 additions & 34 deletions
< 6D40 tr class="diff-line-row">
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include "node_external_reference.h"
44
#include "node_file-inl.h"
55
#include "node_process-inl.h"
6+
#include "path.h"
67
#include "permission/permission.h"
78
#include "util.h"
89

@@ -183,26 +184,24 @@ void AfterClose(uv_fs_t* req) {
183184
void DirHandle::Close(const FunctionCallbackInfo<Value>& args) {
184185
Environment* env = Environment::GetCurrent(args);
185186

186-
const int argc = args.Length();
187-
CHECK_GE(argc, 1);
187+
CHECK_GE(args.Length(), 0); // [req]
188188

189189
DirHandle* dir;
190190
ASSIGN_OR_RETURN_UNWRAP(&dir, args.This());
191191

192192
dir->closing_ = false;
193193
dir->closed_ = true;
194194

195-
FSReqBase* req_wrap_async = GetReqWrap(args, 0);
196-
if (req_wrap_async != nullptr) { // close(req)
195+
if (!args[0]->IsUndefined()) { // close(req)
196+
FSReqBase* req_wrap_async = GetReqWrap(args, 0);
197+
CHECK_NOT_NULL(req_wrap_async);
197198
FS_DIR_ASYNC_TRACE_BEGIN0(UV_FS_CLOSEDIR, req_wrap_async)
198199
AsyncCall(env, req_wrap_async, args, "closedir", UTF8, AfterClose,
199200
uv_fs_closedir, dir->dir());
200-
} else { // close(undefined, ctx)
201-
CHECK_EQ(argc, 2);
202-
FSReqWrapSync req_wrap_sync;
201+
} else { // close()
202+
FSReqWrapSync req_wrap_sync("closedir");
203203
FS_DIR_SYNC_TRACE_BEGIN(closedir);
204-
SyncCall(env, args[1], &req_wrap_sync, "closedir", uv_fs_closedir,
205-
dir->dir());
204+
SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_closedir, dir->dir());
206205
FS_DIR_SYNC_TRACE_END(closedir);
207206
}
208207
}
@@ -282,8 +281,7 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
282281
Environment* env = Environment::GetCurrent(args);
283282
Isolate* isolate = env->isolate();
284283

285-
const int argc = args.Length();
286-
CHECK_GE(argc, 3);
284+
CHECK_GE(args.Length(), 2); // encoding, bufferSize, [callback]
287285

288286
const enum encoding encoding = ParseEncoding(isolate, args[0], UTF8);
289287

@@ -299,27 +297,25 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
299297
dir->dir_->dirents = dir->dirents_.data();
300298
}
301299

302-
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
303-
if (req_wrap_async != nullptr) { // dir.read(encoding, bufferSize, req)
300+
if (!args[2]->IsUndefined()) { // dir.read(encoding, bufferSize, req)
301+
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
302+
CHECK_NOT_NULL(req_wrap_async);
304303
FS_DIR_ASYNC_TRACE_BEGIN0(UV_FS_READDIR, req_wrap_async)
305304
AsyncCall(env, req_wrap_async, args, "readdir", encoding,
306305
AfterDirRead, uv_fs_readdir, dir->dir());
307-
} else { // dir.read(encoding, bufferSize, undefined, ctx)
308-
CHECK_EQ(argc, 4);
309-
FSReqWrapSync req_wrap_sync;
306+
} else { // dir.read(encoding, bufferSize)
307+
FSReqWrapSync req_wrap_sync("readdir");
310308
FS_DIR_SYNC_TRACE_BEGIN(readdir);
311-
int err = SyncCall(env, args[3], &req_wrap_sync, "readdir", uv_fs_readdir,
312-
dir->dir());
309+
int err =
310+
SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_readdir, dir->dir());
313311
FS_DIR_SYNC_TRACE_END(readdir);
314312
if (err < 0) {
315-
return; // syscall failed, no need to continue, error info is in ctx
313+
return; // syscall failed, no need to continue, error is already thrown
316314
}
317315

318316
if (req_wrap_sync.req.result == 0) {
319317
// Done
320-
Local<Value> done = Null(isolate);
321-
args.GetReturnValue().Set(done);
322-
return;
318+
return args.GetReturnValue().SetNull();
323319
}
324320

325321
CHECK_GE(req_wrap_sync.req.result, 0);
@@ -332,8 +328,9 @@ void DirHandle::Read(const FunctionCallbackInfo<Value>& args) {
332328
encoding,
333329
&error)
334330
.ToLocal(&js_array)) {
335-
Local<Object> ctx = args[2].As<Object>();
336-
USE(ctx->Set(env->context(), env->error_string(), error));
331+
// TODO(anonrig): Initializing BufferValue here is wasteful.
332+
BufferValue error_payload(isolate, error);
333+
env->ThrowError(error_payload.out());
337334
return;
338335
}
339336

@@ -362,16 +359,16 @@ static void OpenDir(const FunctionCallbackInfo<Value>& args) {
362359
Environment* env = Environment::GetCurrent(args);
363360
Isolate* isolate = env->isolate();
364361

365-
const int argc = args.Length();
366-
CHECK_GE(argc, 3);
362+
CHECK_GE(args.Length(), 2); // path, encoding, [callback]
367363

368364
BufferValue path(isolate, args[0]);
369365
CHECK_NOT_NULL(*path);
370366

371367
const enum encoding encoding = ParseEncoding(isolate, args[1], UTF8);
372368

373-
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
374-
if (req_wrap_async != nullptr) { // openDir(path, encoding, req)
369+
if (!args[2]->IsUndefined()) { // openDir(path, encoding, req)
370+
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
371+
CHECK_NOT_NULL(req_wrap_async);
375372
ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS(
376373
env,
377374
req_wrap_async,
@@ -381,17 +378,16 @@ static void OpenDir(const FunctionCallbackInfo<Value>& args) {
381378
UV_FS_OPENDIR, req_wrap_async, "path", TRACE_STR_COPY(*path))
382379
AsyncCall(env, req_wrap_async, args, "opendir", encoding, AfterOpenDir,
383380
uv_fs_opendir, *path);
384-
} else { // openDir(path, encoding, undefined, ctx)
385-
CHECK_EQ(argc, 4);
381+
} else { // openDir(path, encoding)
386382
THROW_IF_INSUFFICIENT_PERMISSIONS(
387383
env, permission::PermissionScope::kFileSystemRead, path.ToStringView());
388-
FSReqWrapSync req_wrap_sync;
384+
FSReqWrapSync req_wrap_sync("opendir", *path);
389385
FS_DIR_SYNC_TRACE_BEGIN(opendir);
390-
int result = SyncCall(env, args[3], &req_wrap_sync, "opendir",
391-
uv_fs_opendir, *path);
386+
int result =
387+
SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_opendir, *path);
392388
FS_DIR_SYNC_TRACE_END(opendir);
393389
if (result < 0) {
394-
return; // syscall failed, no need to continue, error info is in ctx
390+
return; // syscall failed, no need to continue, error is already thrown
395391
}
396392

397393
uv_fs_t* req = &req_wrap_sync.req;

‎typings/internalBinding/fs_dir.d.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@ declare namespace InternalFSDirBinding {
66
type StringOrBuffer = string | Buffer;
77

88
class DirHandle {
9-
read(encoding: string, bufferSize: number, _: unknown, ctx: ReadFileContext): string[] | undefined;
10-
close(_: unknown, ctx: ReadFileContext): void;
9+
read(encoding: string, bufferSize: number, callback: FSReqCallback): string[] | undefined;
10+
read(encoding: string, bufferSize: number): string[] | undefined;
11+
close(callback: FSReqCallback): void;
12+
close(): void;
1113
}
1214

1315
function opendir(path: StringOrBuffer, encoding: string, req: FSReqCallback): DirHandle;
14-
function opendir(path: StringOrBuffer, encoding: string, _: undefined, ctx: ReadFileContext): DirHandle;
16+
function opendir(path: StringOrBuffer, encoding: string): DirHandle;
1517
function opendirSync(path: StringOrBuffer): DirHandle;
1618
}
1719

0 commit comments

Comments
 (0)
0