Skip to content

Commit

Permalink
fs: improve error performance for rmdirSync
Browse files Browse the repository at this point in the history
  • Loading branch information
CanadaHonk committed Sep 25, 2023
1 parent 740ca54 commit 9b7260d
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 16 deletions.
44 changes: 44 additions & 0 deletions benchmark/fs/bench-rmdirSync.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict';

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

const bench = common.createBenchmark(main, {
type: ['existing', 'non-existing'],
n: [1e3],
});

function main({ n, type }) {
let files;

switch (type) {
case 'existing':
files = [];

// Populate tmpdir with mock dirs
for (let i = 0; i < n; i++) {
const path = tmpdir.resolve(`rmdirsync-bench-dir-${i}`);
fs.mkdirSync(path);
files.push(path);
}
break;
case 'non-existing':
files = new Array(n).fill(tmpdir.resolve(`.non-existing-file-${Date.now()}`));
break;
default:
new Error('Invalid type');
}

bench.start();
for (let i = 0; i < n; i++) {
try {
fs.rmdirSync(files[i]);
} catch {
// do nothing
}
}
bench.end(n);
}
17 changes: 1 addition & 16 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -1194,22 +1194,7 @@ function rmdir(path, options, callback) {
* @returns {void}
*/
function rmdirSync(path, options) {
path = getValidatedPath(path);

if (options?.recursive) {
emitRecursiveRmdirWarning();
options = validateRmOptionsSync(path, { ...options, force: false }, true);
if (options !== false) {
lazyLoadRimraf();
return rimrafSync(pathModule.toNamespacedPath(path), options);
}
} else {
validateRmdirOptions(options);
}

const ctx = { path };
binding.rmdir(pathModule.toNamespacedPath(path), undefined, ctx);
return handleErrorFromBinding(ctx);
return syncFs.rmdirSync(path, options);
}

/**
Expand Down
27 changes: 27 additions & 0 deletions lib/internal/fs/sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,24 @@

const pathModule = require('path');
const {
emitRecursiveRmdirWarning,
getValidatedPath,
stringToFlags,
getValidMode,
getStatsFromBinding,
getStatFsFromBinding,
getValidatedFd,
validateRmOptionsSync,
validateRmdirOptions,
} = require('internal/fs/utils');
const { parseFileMode } = require('internal/validators');

let rimrafSync;
function lazyLoadRimraf() {
if (rimraf === undefined)
({ rimrafSync } = require('internal/fs/rimraf'));
}

const binding = internalBinding('fs');

/**
Expand Down Expand Up @@ -86,6 +95,23 @@ function close(fd) {
return binding.closeSync(fd);
}

function rmdirSync(path, options) {
path = getValidatedPath(path);

if (options?.recursive) {
emitRecursiveRmdirWarning();
options = validateRmOptionsSync(path, { ...options, force: false }, true);
if (options !== false) {
lazyLoadRimraf();
return rimrafSync(pathModule.toNamespacedPath(path), options);
}
} else {
validateRmdirOptions(options);
}

return binding.rmdirSync(pathModule.toNamespacedPath(path));
}

module.exports = {
readFileUtf8,
exists,
Expand All @@ -95,4 +121,5 @@ module.exports = {
statfs,
open,
close,
rmdirSync
};
23 changes: 23 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1694,6 +1694,27 @@ static void RMDir(const FunctionCallbackInfo<Value>& args) {
}
}

static void RMDirSync(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

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

BufferValue path(env->isolate(), args[0]);
CHECK_NOT_NULL(*path);
THROW_IF_INSUFFICIENT_PERMISSIONS(
env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());

uv_fs_t req;
auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
FS_SYNC_TRACE_BEGIN(rmdir);
int err = uv_fs_rmdir(nullptr, &req, *path, nullptr);
FS_SYNC_TRACE_END(rmdir);
if (err < 0) {
return env->ThrowUVException(err, "rmdir", nullptr, *path);
}
}

int MKDirpSync(uv_loop_t* loop,
uv_fs_t* req,
const std::string& path,
Expand Down Expand Up @@ -3365,6 +3386,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
SetMethod(isolate, target, "rename", Rename);
SetMethod(isolate, target, "ftruncate", FTruncate);
SetMethod(isolate, target, "rmdir", RMDir);
SetMethodNoSideEffect(isolate, target, "rmdirSync", RMDirSync);
SetMethod(isolate, target, "mkdir", MKDir);
SetMethod(isolate, target, "readdir", ReadDir);
SetMethod(isolate, target, "internalModuleReadJSON", InternalModuleReadJSON);
Expand Down Expand Up @@ -3490,6 +3512,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(Rename);
registry->Register(FTruncate);
registry->Register(RMDir);
registry->Register(RMDirSync);
registry->Register(MKDir);
registry->Register(ReadDir);
registry->Register(InternalModuleReadJSON);
Expand Down

0 comments on commit 9b7260d

Please sign in to comment.