-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Comments
On a related note, I wonder if Node should have a |
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 |
V8 now supports optional catch bindings so the |
Yep but it will still be constructed, right? In |
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
Opened a PR to fix this - 10x faster 🎉 |
Errata: I didn't saw that @joyeecheung had already opened #24015 😃 |
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>
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>
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>
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>
The current implementation of
fs.existsSync
involves wrappingaccessSync
in a try/catch statement. While conceptually simple, it has one fundamental flaw: it means that Node has to instance anError
for each call tofs.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 callinghandleErrorFromBinding
here would be sufficient.Relevant lines:
node/lib/fs.js
Lines 229 to 236 in e35f671
The text was updated successfully, but these errors were encountered: