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

Skip stat call / throwing an exception when source files don't exist #225

Merged

Conversation

robstolarz
Copy link
Contributor

I noticed that as the number of tried file paths goes up, Node.js spends a lot of time preparing a native exception to throw over the JS barrier. I narrowed this down to be a result of relying on the fs.statSync call in fileExistsSync to determine whether or not the file exists on top of whether or not it is actually a file.

Prepending a call to fs.existsSync allows us to skip the preparation time of throwing an exception at the cost of causing another IO hit. There is ostensibly some sort of tradeoff but for the large monolithic app I work on, this shaves off the vast majority of the boot and hot reload time.[1]

Happy to discuss / prepare a more detailed breakdown if necessary.

1: 6x improvement in boot time on ~6k JS source files

@maschwenk
Copy link

maschwenk commented Nov 30, 2022

Unbelievable find @robstolarz. Want to note to the maintainer and anyone who sees this PR, if you try to upgrade and use this, you may notice that you don't see the 6x benefit immediately. However, what I found is that if you for some reason have two import 'tsconfig-paths/register';'s in your code, perhaps because you are importing a package into an app that uses it, you could have a case where the improvements from this PR are being overridden when that second case of import 'tsconfig-paths/register' happens. It may make sense to notify folks who depend on this package:

  1. Of this incredible improvement
  2. Be careful that the monkey patch is not being undone. In truth, I think tsconfig-paths should log a warning when it detects that another instance of tsconfig-paths has already monkey patched require (if that's even possible. if it is lmk happy to upstream it)

@robstolarz
Copy link
Contributor Author

@jonaskello Any thoughts on when you might be able to review this? We'd like to get this change moving as soon as possible

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Base: 68.28% // Head: 68.16% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (851596a) compared to base (206e49a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
- Coverage   68.28%   68.16%   -0.12%     
==========================================
  Files           9        9              
  Lines         309      311       +2     
  Branches       95       96       +1     
==========================================
+ Hits          211      212       +1     
- Misses         92       93       +1     
  Partials        6        6              
Impacted Files Coverage Δ
src/filesystem.ts 72.00% <100.00%> (-1.92%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jonaskello
Copy link
Member

Seems like a straightforward change so I think we can just merge and release in a patch version.

@jonaskello jonaskello merged commit b732fcf into dividab:master Nov 30, 2022
@robstolarz robstolarz deleted the RS/minimize-exceptions-for-missing-files branch November 30, 2022 20:17
@jonaskello
Copy link
Member

Release in 4.1.1

@robstolarz
Copy link
Contributor Author

Thanks so much for the quick turnaround!!

marvinhagemeister added a commit to marvinhagemeister/eslint-plugin-import that referenced this pull request Jan 10, 2023
The version 4.1.1 of `tsconfig-paths` includes a performance fix, that
can improve resolve speed by 10-20% in repositories with many files.

See: dividab/tsconfig-paths#225
BPScott pushed a commit to BPScott/tsconfig-paths that referenced this pull request Dec 19, 2023
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

Successfully merging this pull request may close these issues.

3 participants