Skip to content

Commit

Permalink
fs: improve error performance of sync methods
Browse files Browse the repository at this point in the history
PR-URL: #49593
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
  • Loading branch information
anonrig authored and ruyadorno committed Sep 28, 2023
1 parent 1424404 commit 938471e
Show file tree
Hide file tree
Showing 10 changed files with 529 additions and 198 deletions.
42 changes: 42 additions & 0 deletions benchmark/fs/bench-accessSync.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
'use strict';

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

const tmpfile = tmpdir.resolve(`.existing-file-${process.pid}`);
fs.writeFileSync(tmpfile, 'this-is-for-a-benchmark', 'utf8');

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

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

switch (type) {
case 'existing':
path = __filename;
break;
case 'non-flat-existing':
path = tmpfile;
break;
case 'non-existing':
path = tmpdir.resolve(`.non-existing-file-${process.pid}`);
break;
default:
new Error('Invalid type');
}

bench.start();
for (let i = 0; i < n; i++) {
try {
fs.accessSync(path);
} catch {
// do nothing
}
}
bench.end(n);
}
37 changes: 37 additions & 0 deletions benchmark/fs/bench-copyFileSync.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'use strict';

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

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

function main({ n, type }) {
tmpdir.refresh();
const dest = tmpdir.resolve(`copy-file-bench-${process.pid}`);
let path;

switch (type) {
case 'invalid':
path = tmpdir.resolve(`.existing-file-${process.pid}`);
break;
case 'valid':
path = __filename;
break;
default:
throw new Error('Invalid type');
}
bench.start();
for (let i = 0; i < n; i++) {
try {
fs.copyFileSync(path, dest);
} catch {
// do nothing
}
}
bench.end(n);
}
38 changes: 38 additions & 0 deletions benchmark/fs/bench-existsSync.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

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

const tmpfile = tmpdir.resolve(`.existing-file-${process.pid}`);
fs.writeFileSync(tmpfile, 'this-is-for-a-benchmark', 'utf8');

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

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

switch (type) {
case 'existing':
path = __filename;
break;
case 'non-flat-existing':
path = tmpfile;
break;
case 'non-existing':
path = tmpdir.resolve(`.non-existing-file-${process.pid}`);
break;
default:
new Error('Invalid type');
}

bench.start();
for (let i = 0; i < n; i++) {
fs.existsSync(path);
}
bench.end(n);
}
37 changes: 37 additions & 0 deletions benchmark/fs/bench-openSync.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
'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: [1e5],
});

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

switch (type) {
case 'existing':
path = __filename;
break;
case 'non-existing':
path = tmpdir.resolve(`.non-existing-file-${process.pid}`);
break;
default:
new Error('Invalid type');
}

bench.start();
for (let i = 0; i < n; i++) {
try {
const fd = fs.openSync(path, 'r', 0o666);
fs.closeSync(fd);
} catch {
// do nothing
}
}
bench.end(n);
}
79 changes: 11 additions & 68 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ const {
validateObject,
validateString,
} = require('internal/validators');
const { readFileSyncUtf8 } = require('internal/fs/read/utf8');
const syncFs = require('internal/fs/sync');

let truncateWarn = true;
let fs;
Expand Down Expand Up @@ -243,12 +243,7 @@ function access(path, mode, callback) {
* @returns {void}
*/
function accessSync(path, mode) {
path = getValidatedPath(path);
mode = getValidMode(mode, 'access');

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

/**
Expand Down Expand Up @@ -290,23 +285,7 @@ ObjectDefineProperty(exists, kCustomPromisifiedSymbol, {
* @returns {boolean}
*/
function existsSync(path) {
try {
path = getValidatedPath(path);
} catch {
return false;
}
const ctx = { path };
const nPath = pathModule.toNamespacedPath(path);
binding.access(nPath, F_OK, undefined, ctx);

// In case of an invalid symlink, `binding.access()` on win32
// will **not** return an error and is therefore not enough.
// Double check with `binding.stat()`.
if (isWindows && ctx.errno === undefined) {
binding.stat(nPath, false, undefined, ctx);
}

return ctx.errno === undefined;
return syncFs.exists(path);
}

function readFileAfterOpen(err, fd) {
Expand Down Expand Up @@ -462,8 +441,7 @@ function readFileSync(path, options) {

// TODO(@anonrig): Do not handle file descriptor ownership for now.
if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) {
path = getValidatedPath(path);
return readFileSyncUtf8(pathModule.toNamespacedPath(path), stringToFlags(options.flag));
return syncFs.readFileUtf8(path, options.flag);
}

const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666);
Expand Down Expand Up @@ -540,11 +518,7 @@ function close(fd, callback = defaultCloseCallback) {
* @returns {void}
*/
function closeSync(fd) {
fd = getValidatedFd(fd);

const ctx = {};
binding.close(fd, undefined, ctx);
handleErrorFromBinding(ctx);
return syncFs.close(fd);
}

/**
Expand Down Expand Up @@ -590,16 +564,7 @@ function open(path, flags, mode, callback) {
* @returns {number}
*/
function openSync(path, flags, mode) {
path = getValidatedPath(path);
const flagsNumber = stringToFlags(flags);
mode = parseFileMode(mode, 'mode', 0o666);

const ctx = { path };
const result = binding.open(pathModule.toNamespacedPath(path),
flagsNumber, mode,
undefined, ctx);
handleErrorFromBinding(ctx);
return result;
return syncFs.open(path, flags, mode);
}

/**
Expand Down Expand Up @@ -1702,25 +1667,12 @@ function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) {
* }} [options]
* @returns {Stats}
*/
function statSync(path, options = { bigint: false, throwIfNoEntry: true }) {
path = getValidatedPath(path);
const ctx = { path };
const stats = binding.stat(pathModule.toNamespacedPath(path),
options.bigint, undefined, ctx);
if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) {
return undefined;
}
handleErrorFromBinding(ctx);
return getStatsFromBinding(stats);
function statSync(path, options) {
return syncFs.stat(path, options);
}

function statfsSync(path, options = { bigint: false }) {
path = getValidatedPath(path);
const ctx = { path };
const stats = binding.statfs(pathModule.toNamespacedPath(path),
options.bigint, undefined, ctx);
handleErrorFromBinding(ctx);
return getStatFsFromBinding(stats);
function statfsSync(path, options) {
return syncFs.statfs(path, options);
}

/**
Expand Down Expand Up @@ -2999,16 +2951,7 @@ function copyFile(src, dest, mode, callback) {
* @returns {void}
*/
function copyFileSync(src, dest, mode) {
src = getValidatedPath(src, 'src');
dest = getValidatedPath(dest, 'dest');

const ctx = { path: src, dest }; // non-prefixed

src = pathModule._makeLong(src);
dest = pathModule._makeLong(dest);
mode = getValidMode(mode, 'copyFile');
binding.copyFile(src, dest, mode, undefined, ctx);
handleErrorFromBinding(ctx);
syncFs.copyFile(src, dest, mode);
}

/**
Expand Down
25 changes: 0 additions & 25 deletions lib/internal/fs/read/utf8.js

This file was deleted.

0 comments on commit 938471e

Please sign in to comment.