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.

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

existsSync is expensive #24008

Closed
arcanis opened this issue Nov 1, 2018 · 6 comments
Closed

existsSync is expensive #24008

arcanis opened this issue Nov 1, 2018 · 6 comments

Comments

@arcanis
Copy link
Contributor

arcanis commented Nov 1, 2018

  • Version: master
  • Platform: n/a
  • Subsystem: fs

The current implementation of fs.existsSync involves wrapping accessSync in a try/catch statement. While conceptually simple, it has one fundamental flaw: it means that Node has to instance an Error for each call to fs.existsSync made on non-existing paths. This can end up very expensive.

Would it be possible to make this function more lightweight by simply checking the return value of the libuv's access call? I'm not familiar with the Node internals, but I feel like simply returning false instead of calling handleErrorFromBinding here would be sufficient.

Relevant lines:

node/lib/fs.js

Lines 229 to 236 in e35f671

function existsSync(path) {
try {
fs.accessSync(path, F_OK);
return true;
} catch (e) {
return false;
}
}

@arcanis
Copy link
Contributor Author

arcanis commented Nov 1, 2018

On a related note, I wonder if Node should have a tryStat function that would simply return null when an error happens (to also avoid creating this Error object).

@joyeecheung
Copy link
Member

This was added in d3955d1 from a glance I think it should be alright to call the binding and handle it directly instead of using the public fs.accessSync

@jdalton
Copy link
Member

jdalton commented Nov 2, 2018

V8 now supports optional catch bindings so the e can just be omitted.

@arcanis
Copy link
Contributor Author

arcanis commented Nov 2, 2018

Yep but it will still be constructed, right? In handleErrorFromBinding

arcanis pushed a commit to arcanis/node that referenced this issue 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.

Fixes: nodejs#24008
@arcanis
Copy link
Contributor Author

arcanis commented Nov 3, 2018

Opened a PR to fix this - 10x faster 🎉

#24068

@arcanis
Copy link
Contributor Author

arcanis commented Nov 3, 2018

Errata: I didn't saw that @joyeecheung had already opened #24015 😃

@Trott Trott closed this as completed in bcc7b7a Nov 4, 2018
targos pushed a commit that referenced this issue Nov 5, 2018
Instead of throwing errors in fs.accessSync and then catching it,
handle the result from the binding directly in fs.existsSync.

Note that the argument validation errors still needs to be caught
until we properly deprecate the don't-thrown-on-invalid-arguments
behavior.

PR-URL: #24015
Fixes: #24008
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
codebytere pushed a commit that referenced this issue Nov 29, 2018
Instead of throwing errors in fs.accessSync and then catching it,
handle the result from the binding directly in fs.existsSync.

Note that the argument validation errors still needs to be caught
until we properly deprecate the don't-thrown-on-invalid-arguments
behavior.

PR-URL: #24015
Fixes: #24008
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
Instead of throwing errors in fs.accessSync and then catching it,
handle the result from the binding directly in fs.existsSync.

Note that the argument validation errors still needs to be caught
until we properly deprecate the don't-thrown-on-invalid-arguments
behavior.

PR-URL: #24015
Fixes: #24008
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 3, 2018
Instead of throwing errors in fs.accessSync and then catching it,
handle the result from the binding directly in fs.existsSync.

Note that the argument validation errors still needs to be caught
until we properly deprecate the don't-thrown-on-invalid-arguments
behavior.

PR-URL: #24015
Fixes: #24008
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants