Skip to content

Commit

Permalink
fs: improve error performance of renameSync
Browse files Browse the repository at this point in the history
PR-URL: #49962
Refs: nodejs/performance#106
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
  • Loading branch information
anonrig authored and targos committed Oct 23, 2023
1 parent a38d131 commit 610036c
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 12 deletions.
49 changes: 49 additions & 0 deletions benchmark/fs/bench-renameSync.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict';

const common = require('../common');
const fs = require('fs');
const assert = require('assert');
const tmpdir = require('../../test/common/tmpdir');

const bench = common.createBenchmark(main, {
type: ['invalid', 'valid'],
n: [2e3],
});

function main({ n, type }) {
switch (type) {
case 'invalid': {
let hasError = false;
bench.start();
for (let i = 0; i < n; i++) {
try {
fs.renameSync(tmpdir.resolve(`.non-existing-file-${i}`), tmpdir.resolve(`.new-file-${i}`));
} catch {
hasError = true;
}
}
bench.end(n);
assert(hasError);
break;
}
case 'valid': {
tmpdir.refresh();
for (let i = 0; i < n; i++) {
fs.writeFileSync(tmpdir.resolve(`.existing-file-${i}`), 'bench', 'utf8');
}

bench.start();
for (let i = 0; i < n; i++) {
fs.renameSync(
tmpdir.resolve(`.existing-file-${i}`),
tmpdir.resolve(`.new-existing-file-${i}`),
);
}

bench.end(n);
break;
}
default:
throw new Error('Invalid type');
}
}
8 changes: 4 additions & 4 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -1034,10 +1034,10 @@ function rename(oldPath, newPath, callback) {
function renameSync(oldPath, newPath) {
oldPath = getValidatedPath(oldPath, 'oldPath');
newPath = getValidatedPath(newPath, 'newPath');
const ctx = { path: oldPath, dest: newPath };
binding.rename(pathModule.toNamespacedPath(oldPath),
pathModule.toNamespacedPath(newPath), undefined, ctx);
handleErrorFromBinding(ctx);
binding.rename(
pathModule.toNamespacedPath(oldPath),
pathModule.toNamespacedPath(newPath),
);
}

/**
Expand Down
15 changes: 7 additions & 8 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1432,7 +1432,7 @@ static void Rename(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = env->isolate();

const int argc = args.Length();
CHECK_GE(argc, 3);
CHECK_GE(argc, 2);

BufferValue old_path(isolate, args[0]);
CHECK_NOT_NULL(*old_path);
Expand All @@ -1449,8 +1449,8 @@ static void Rename(const FunctionCallbackInfo<Value>& args) {
permission::PermissionScope::kFileSystemWrite,
new_path.ToStringView());

FSReqBase* req_wrap_async = GetReqWrap(args, 2);
if (req_wrap_async != nullptr) {
if (argc > 2) { // rename(old_path, new_path, req)
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
FS_ASYNC_TRACE_BEGIN2(UV_FS_RENAME,
req_wrap_async,
"old_path",
Expand All @@ -1460,12 +1460,11 @@ static void Rename(const FunctionCallbackInfo<Value>& args) {
AsyncDestCall(env, req_wrap_async, args, "rename", *new_path,
new_path.length(), UTF8, AfterNoArgs, uv_fs_rename,
*old_path, *new_path);
} else {
CHECK_EQ(argc, 4);
FSReqWrapSync req_wrap_sync;
} else { // rename(old_path, new_path)
FSReqWrapSync req_wrap_sync("rename", *old_path, *new_path);
FS_SYNC_TRACE_BEGIN(rename);
SyncCall(env, args[3], &req_wrap_sync, "rename", uv_fs_rename,
*old_path, *new_path);
SyncCallAndThrowOnError(
env, &req_wrap_sync, uv_fs_rename, *old_path, *new_path);
FS_SYNC_TRACE_END(rename);
}
}
Expand Down
1 change: 1 addition & 0 deletions typings/internalBinding/fs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ declare namespace InternalFSBinding {
function rename(oldPath: string, newPath: string, req: FSReqCallback): void;
function rename(oldPath: string, newPath: string, req: undefined, ctx: FSSyncContext): void;
function rename(oldPath: string, newPath: string, usePromises: typeof kUsePromises): Promise<void>;
function rename(oldPath: string, newPath: string): void;

function rmdir(path: string, req: FSReqCallback): void;
function rmdir(path: string, req: undefined, ctx: FSSyncContext): void;
Expand Down

0 comments on commit 610036c

Please sign in to comment.