Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: improve error perf of sync chmod+fchmod #49859

Closed
wants to merge 2 commits into from
Closed

fs: improve error perf of sync chmod+fchmod #49859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
395df96
fs: improve error perf of sync `chmod`+`fchmod`
CanadaHonk Sep 25, 2023
a4041fc
fs: fix `unlinkSync` typings
anonrig Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions benchmark/fs/bench-chmodSync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
'use strict';

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

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

function main({ n, type }) {
switch (type) {
case 'existing': {
for (let i = 0; i < n; i++) {
fs.writeFileSync(tmpdir.resolve(`chmodsync-bench-file-${i}`), 'bench');
}

bench.start();
for (let i = 0; i < n; i++) {
fs.chmodSync(tmpdir.resolve(`chmodsync-bench-file-${i}`), 0o666);
}
bench.end(n);
break;
}
case 'non-existing':
bench.start();
for (let i = 0; i < n; i++) {
try {
fs.chmodSync(tmpdir.resolve(`chmod-non-existing-file-${i}`), 0o666);
} catch {
// do nothing
}
}
bench.end(n);
break;
default:
new Error('Invalid type');
}
}
60 changes: 60 additions & 0 deletions benchmark/fs/bench-fchmodSync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
'use strict';

const common = require('../common');
const fs = require('fs');
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 files
for (let i = 0; i < n; i++) {
const path = tmpdir.resolve(`fchmodsync-bench-file-${i}`);
fs.writeFileSync(path, 'bench');
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');
}

const fds = files.map((x) => {
// Try to open, if not return likely invalid fd (1 << 30)
try {
return fs.openSync(x, 'r');
} catch {
return 1 << 30;
}
});

bench.start();
for (let i = 0; i < n; i++) {
try {
fs.fchmodSync(fds[i], 0o666);
} catch {
// do nothing
}
}
bench.end(n);

for (const x of fds) {
try {
fs.closeSync(x);
} catch {
// do nothing
}
}
}
16 changes: 8 additions & 8 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1908,11 +1908,10 @@ function fchmod(fd, mode, callback) {
* @returns {void}
*/
function fchmodSync(fd, mode) {
fd = getValidatedFd(fd);
mode = parseFileMode(mode, 'mode');
const ctx = {};
binding.fchmod(fd, mode, undefined, ctx);
handleErrorFromBinding(ctx);
binding.fchmod(
getValidatedFd(fd),
parseFileMode(mode, 'mode'),
);
}

/**
Expand Down Expand Up @@ -1987,9 +1986,10 @@ function chmodSync(path, mode) {
path = getValidatedPath(path);
mode = parseFileMode(mode, 'mode');

const ctx = { path };
binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx);
handleErrorFromBinding(ctx);
binding.chmod(
pathModule.toNamespacedPath(path),
mode,
);
}

/**
Expand Down
24 changes: 10 additions & 14 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2506,18 +2506,16 @@ static void Chmod(const FunctionCallbackInfo<Value>& args) {
CHECK(args[1]->IsInt32());
int mode = args[1].As<Int32>()->Value();

FSReqBase* req_wrap_async = GetReqWrap(args, 2);
if (req_wrap_async != nullptr) { // chmod(path, mode, req)
if (argc > 2) { // chmod(path, mode, req)
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
FS_ASYNC_TRACE_BEGIN1(
UV_FS_CHMOD, req_wrap_async, "path", TRACE_STR_COPY(*path))
AsyncCall(env, req_wrap_async, args, "chmod", UTF8, AfterNoArgs,
uv_fs_chmod, *path, mode);
} else { // chmod(path, mode, undefined, ctx)
CHECK_EQ(argc, 4);
FSReqWrapSync req_wrap_sync;
} else { // chmod(path, mode)
FSReqWrapSync req_wrap_sync("chmod", *path);
FS_SYNC_TRACE_BEGIN(chmod);
SyncCall(env, args[3], &req_wrap_sync, "chmod",
uv_fs_chmod, *path, mode);
SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_chmod, *path, mode);
FS_SYNC_TRACE_END(chmod);
}
}
Expand All @@ -2538,17 +2536,15 @@ static void FChmod(const FunctionCallbackInfo<Value>& args) {
CHECK(args[1]->IsInt32());
const int mode = args[1].As<Int32>()->Value();

FSReqBase* req_wrap_async = GetReqWrap(args, 2);
if (req_wrap_async != nullptr) { // fchmod(fd, mode, req)
if (argc > 2) { // fchmod(fd, mode, req)
FSReqBase* req_wrap_async = GetReqWrap(args, 2);
FS_ASYNC_TRACE_BEGIN0(UV_FS_FCHMOD, req_wrap_async)
AsyncCall(env, req_wrap_async, args, "fchmod", UTF8, AfterNoArgs,
uv_fs_fchmod, fd, mode);
} else { // fchmod(fd, mode, undefined, ctx)
CHECK_EQ(argc, 4);
FSReqWrapSync req_wrap_sync;
} else { // fchmod(fd, mode)
FSReqWrapSync req_wrap_sync("fchmod");
FS_SYNC_TRACE_BEGIN(fchmod);
SyncCall(env, args[3], &req_wrap_sync, "fchmod",
uv_fs_fchmod, fd, mode);
SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_fchmod, fd, mode);
FS_SYNC_TRACE_END(fchmod);
}
}
Expand Down
6 changes: 3 additions & 3 deletions typings/internalBinding/fs.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ declare namespace InternalFSBinding {
function access(path: StringOrBuffer, mode: number, usePromises: typeof kUsePromises): Promise<void>;

function chmod(path: string, mode: number, req: FSReqCallback): void;
function chmod(path: string, mode: number, req: undefined, ctx: FSSyncContext): void;
function chmod(path: string, mode: number): void;
function chmod(path: string, mode: number, usePromises: typeof kUsePromises): Promise<void>;

function chown(path: string, uid: number, gid: number, req: FSReqCallback): void;
Expand All @@ -76,7 +76,7 @@ declare namespace InternalFSBinding {
function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, usePromises: typeof kUsePromises): Promise<void>;

function fchmod(fd: number, mode: number, req: FSReqCallback): void;
function fchmod(fd: number, mode: number, req: undefined, ctx: FSSyncContext): void;
function fchmod(fd: number, mode: number): void;
function fchmod(fd: number, mode: number, usePromises: typeof kUsePromises): Promise<void>;

function fchown(fd: number, uid: number, gid: number, req: FSReqCallback): void;
Expand Down Expand Up @@ -203,7 +203,7 @@ declare namespace InternalFSBinding {
function symlink(target: StringOrBuffer, path: StringOrBuffer, type: number, usePromises: typeof kUsePromises): Promise<void>;

function unlink(path: string, req: FSReqCallback): void;
function unlink(path: string, req: undefined, ctx: FSSyncContext): void;
function unlink(path: string): void;
function unlink(path: string, usePromises: typeof kUsePromises): Promise<void>;

function utimes(path: string, atime: number, mtime: number, req: FSReqCallback): void;
Expand Down