Skip to content
New issue

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

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

Already on GitHub? # to your account

fs: makes existsSync stop using errors #24068

Closed
wants to merge 1 commit into from

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Nov 3, 2018

The previous implementation of existsSync was a try/catch on top
of accessSync. While conceptually sound it was a performance problem
when running it repeatedly on non-existing files, because accessSync
had to create an Error object that was immediatly discarded (because
existsSync never reports anything else than true / false).

This implementation simply checks whether the context would have caused
an exception to be thrown, but doesn't actually create it.

I benchmarked it as such:

const fs = require('fs');

console.time();
for (let t = 0; t < 100000; ++t) fs.existsSync('./doesnt-exist');
console.timeEnd();
❯ [mael-mbp?] node git:(existssync-no-errors) ✗ ❯ node --version
v11.1.0

❯ [mael-mbp?] node git:(existssync-no-errors) ✗ ❯ node test.js
default: 2543.314ms

❯ [mael-mbp?] node git:(existssync-no-errors) ✗ ❯ ./node test.js
default: 287.442ms

Fixes: #24008

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

The previous implementation of `existsSync` was a try/catch on top
of `accessSync`. While conceptually sound it was a performance problem
when running it repeatedly on non-existing files, because `accessSync`
had to create an `Error` object that was immediatly discarded (because
`existsSync` never reports anything else than `true` / `false`).

This implementation simply checks whether the context would have caused
an exception to be thrown, but doesn't actually create it.

Fixes: nodejs#24008
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 3, 2018
@arcanis arcanis mentioned this pull request Nov 3, 2018
@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2018

I believe this change is already being made in #24015?

@arcanis
Copy link
Contributor Author

arcanis commented Nov 3, 2018

Oh, my bad! I see Github still doesn't send notifs when a PR is attached to an issue .. 😅

@arcanis arcanis closed this Nov 3, 2018
@refack
Copy link
Contributor

refack commented Nov 4, 2018

Oh, my bad!

I wouldn't worry about that. Hope to see you contribute more in the future.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants