Skip to content

Commit 3437408

Browse files
committed
fs: improve error perf of sync chmod+fchmod
1 parent bd614f3 commit 3437408

File tree

5 files changed

+169
-11
lines changed

5 files changed

+169
-11
lines changed

benchmark/fs/bench-chmodSync.js

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const fs = require('fs');
5+
const tmpdir = require('../../test/common/tmpdir');
6+
tmpdir.refresh();
7+
8+
const bench = common.createBenchmark(main, {
9+
type: ['existing', 'non-existing'],
10+
n: [1e3],
11+
});
12+
13+
function main({ n, type }) {
14+
let files;
15+
16+
switch (type) {
17+
case 'existing':
18+
files = [];
19+
20+
// Populate tmpdir with mock files
21+
for (let i = 0; i < n; i++) {
22+
const path = tmpdir.resolve(`chmodsync-bench-file-${i}`);
23+
fs.writeFileSync(path, 'bench');
24+
files.push(path);
25+
}
26+
break;
27+
case 'non-existing':
28+
files = new Array(n).fill(tmpdir.resolve(`.non-existing-file-${Date.now()}`));
29+
break;
30+
default:
31+
new Error('Invalid type');
32+
}
33+
34+
bench.start();
35+
for (let i = 0; i < n; i++) {
36+
try {
37+
fs.chmodSync(files[i], 0o666);
38+
} catch {
39+
// do nothing
40+
}
41+
}
42+
bench.end(n);
43+
}

benchmark/fs/bench-fchmodSync.js

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const fs = require('fs');
5+
const tmpdir = require('../../test/common/tmpdir');
6+
tmpdir.refresh();
7+
8+
const bench = common.createBenchmark(main, {
9+
type: ['existing', 'non-existing'],
10+
n: [1e3],
11+
});
12+
13+
function main({ n, type }) {
14+
let files;
15+
16+
switch (type) {
17+
case 'existing':
18+
files = [];
19+
20+
// Populate tmpdir with mock files
21+
for (let i = 0; i < n; i++) {
22+
const path = tmpdir.resolve(`fchmodsync-bench-file-${i}`);
23+
fs.writeFileSync(path, 'bench');
24+
files.push(path);
25+
}
26+
break;
27+
case 'non-existing':
28+
files = new Array(n).fill(tmpdir.resolve(`.non-existing-file-${Date.now()}`));
29+
break;
30+
default:
31+
new Error('Invalid type');
32+
}
33+
34+
const fds = files.map(x => {
35+
// try to open, if not return invalid fd (-1)
36+
try {
37+
return fs.openSync(x, 'r');
38+
} catch {
39+
return -1;
40+
}
41+
});
42+
43+
bench.start();
44+
for (let i = 0; i < n; i++) {
45+
try {
46+
fs.fchmodSync(fds[i], 0o666);
47+
} catch {
48+
// do nothing
49+
}
50+
}
51+
bench.end(n);
52+
53+
for (const x of fds) {
54+
try {
55+
fs.closeSync(x);
56+
} catch {
57+
// do nothing
58+
}
59+
}
60+
}

lib/fs.js

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1882,11 +1882,7 @@ function fchmod(fd, mode, callback) {
18821882
* @returns {void}
18831883
*/
18841884
function fchmodSync(fd, mode) {
1885-
fd = getValidatedFd(fd);
1886-
mode = parseFileMode(mode, 'mode');
1887-
const ctx = {};
1888-
binding.fchmod(fd, mode, undefined, ctx);
1889-
handleErrorFromBinding(ctx);
1885+
return syncFs.fchmod(fd, mode);
18901886
}
18911887

18921888
/**
@@ -1958,12 +1954,7 @@ function chmod(path, mode, callback) {
19581954
* @returns {void}
19591955
*/
19601956
function chmodSync(path, mode) {
1961-
path = getValidatedPath(path);
1962-
mode = parseFileMode(mode, 'mode');
1963-
1964-
const ctx = { path };
1965-
binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx);
1966-
handleErrorFromBinding(ctx);
1957+
return syncFs.chmod(path, mode);
19671958
}
19681959

19691960
/**

lib/internal/fs/sync.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,18 @@ function close(fd) {
8686
return binding.closeSync(fd);
8787
}
8888

89+
function chmod(path, mode) {
90+
path = getValidatedPath(path);
91+
mode = parseFileMode(mode, 'mode');
92+
return binding.chmodSync(pathModule.toNamespacedPath(path), mode);
93+
}
94+
95+
function fchmod(fd, mode) {
96+
fd = getValidatedFd(fd);
97+
mode = parseFileMode(mode, 'mode');
98+
return binding.fchmodSync(fd, mode);
99+
}
100+
89101
module.exports = {
90102
readFileUtf8,
91103
exists,
@@ -95,4 +107,6 @@ module.exports = {
95107
statfs,
96108
open,
97109
close,
110+
chmod,
111+
fchmod,
98112
};

src/node_file.cc

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2673,6 +2673,30 @@ static void Chmod(const FunctionCallbackInfo<Value>& args) {
26732673
}
26742674
}
26752675

2676+
static void ChmodSync(const FunctionCallbackInfo<Value>& args) {
2677+
Environment* env = Environment::GetCurrent(args);
2678+
2679+
const int argc = args.Length();
2680+
CHECK_GE(argc, 2);
2681+
2682+
BufferValue path(env->isolate(), args[0]);
2683+
CHECK_NOT_NULL(*path);
2684+
THROW_IF_INSUFFICIENT_PERMISSIONS(
2685+
env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());
2686+
2687+
CHECK(args[1]->IsInt32());
2688+
int mode = args[1].As<Int32>()->Value();
2689+
2690+
uv_fs_t req;
2691+
auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
2692+
FS_SYNC_TRACE_BEGIN(chmod);
2693+
int err = uv_fs_chmod(nullptr, &req, *path, mode, nullptr);
2694+
FS_SYNC_TRACE_END(chmod);
2695+
if (err < 0) {
2696+
return env->ThrowUVException(err, "chmod", nullptr, *path);
2697+
}
2698+
}
2699+
26762700

26772701
/* fs.fchmod(fd, mode);
26782702
* Wrapper for fchmod(1) / EIO_FCHMOD
@@ -2704,6 +2728,28 @@ static void FChmod(const FunctionCallbackInfo<Value>& args) {
27042728
}
27052729
}
27062730

2731+
static void FChmodSync(const FunctionCallbackInfo<Value>& args) {
2732+
Environment* env = Environment::GetCurrent(args);
2733+
2734+
const int argc = args.Length();
2735+
CHECK_GE(argc, 2);
2736+
2737+
CHECK(args[0]->IsInt32());
2738+
const int fd = args[0].As<Int32>()->Value();
2739+
2740+
CHECK(args[1]->IsInt32());
2741+
int mode = args[1].As<Int32>()->Value();
2742+
2743+
uv_fs_t req;
2744+
auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); });
2745+
FS_SYNC_TRACE_BEGIN(fchmod);
2746+
int err = uv_fs_fchmod(nullptr, &req, fd, mode, nullptr);
2747+
FS_SYNC_TRACE_END(fchmod);
2748+
if (err < 0) {
2749+
return env->ThrowUVException(err, "fchmod");
2750+
}
2751+
}
2752+
27072753
/* fs.chown(path, uid, gid);
27082754
* Wrapper for chown(1) / EIO_CHOWN
27092755
*/
@@ -3387,7 +3433,9 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
33873433
SetMethodNoSideEffect(isolate, target, "copyFileSync", CopyFileSync);
33883434

33893435
SetMethod(isolate, target, "chmod", Chmod);
3436+
SetMethod(isolate, target, "chmodSync", ChmodSync);
33903437
SetMethod(isolate, target, "fchmod", FChmod);
3438+
SetMethod(isolate, target, "fchmodSync", FChmodSync);
33913439

33923440
SetMethod(isolate, target, "chown", Chown);
33933441
SetMethod(isolate, target, "fchown", FChown);
@@ -3512,7 +3560,9 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
35123560
registry->Register(CopyFileSync);
35133561

35143562
registry->Register(Chmod);
3563+
registry->Register(ChmodSync);
35153564
registry->Register(FChmod);
3565+
registry->Register(FChmodSync);
35163566

35173567
registry->Register(Chown);
35183568
registry->Register(FChown);

0 commit comments

Comments
 (0)