Set stackTraceLimit to 0 in fileSystemEntryExists #40043
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
Tagging @amcasey due to the relation with PR #36190. Please verify that:
New tests may be a bit difficult with this specific change. Open to any testing suggestions if anyone has them. Also not sure what the associated backlog issue for this should be. |
| if (!_fs.existsSync(path)) { | ||
| return false; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Another option is to just return using existsSync and ignore directory or file and make our code more tolerant (it might already be) for finding file instead of directory or vice versa. That could give us better perf in both cases.
There was a problem hiding this comment.
Definitely. I was also wondering how necessary it was to check for file versus folder existence.
I'm guessing the current behavior of this function is this way so folders ending with .ts get ignored. Is that an edge case where a behavior change would be acceptable?
There was a problem hiding this comment.
If you make a change we can see what user tests break and see what happens
There was a problem hiding this comment.
Ended up not doing this and favoring Error.stackTraceLimit = 0 instead. I assume we wanted to try using existsSync entirely since we were worried about perf in the path exists case. Setting Error.stackTraceLimit = 0 should improve performance when files don't exist with no change to checking files that do exist.
gluxon
force-pushed
the
uvException-hotspot
branch
from
af5cd1d
to
1df3b49
Compare
gluxon
changed the title
|
I ended up removing the Since the happy path (when the path exists) no longer calls both |
|
@gluxon, it's precisely the file/dir distinction that prevented me from rolling out As an alternative, I've proposed a change to node itself: nodejs/node#33716 |
|
Without having re-reviewed the code, I'm also somewhat concerned that the exists/non-exists balance might be different in long-running watch or language service scenarios than it is during batch compilation. |
That's a great proposed change. I was quite frustrated that there's no fast way to
I ended up using the |
The exception thrown by Node.js's fs.statSync function contains a stack trace that can be expensive to compute. Since this exception isn't used by fileSystemEntryExists, we can safely set Error.stackTraceLimit to 0 without a change in behavior. --- A significant performance improvement was noticed with this change while profiling tsserver on packages within a proprietary monorepo. Specifically, my team saw high self time percentages for Node.js's uvException and handleErrorFromBinding internal functions. These functions are executed within fs.statSync when it fails to find the given path. https://user-images.githubusercontent.com/906558/90183227-220cb800-dd81-11ea-8d61-f41f89481f46.png fs.statSync: https://github.com/nodejs/node/blob/v14.4.0/lib/fs.js#L1030-L1037 handleErrorFromBinding: https://github.com/nodejs/node/blob/v14.4.0/lib/internal/fs/utils.js#L254-L269 uvException: https://github.com/nodejs/node/blob/v14.4.0/lib/internal/errors.js#L390-L443 ## Measurements After adding Error.stackTraceLimit = 0, we saw: - For a large configured project with 12,565 files, tsserver reached the projectLoadingFinish event 48.78% faster. (~46.786s vs ~31.447s) - For a medium project with 7,064 files, tsserver was 25.75% faster. (~20.897s vs ~16.618s) - For a small project with 796 files, tsserver was only a negligible 3.00% faster. (~3.545s vs ~3.442) Measurements were taken on macOS 10.15.6, Node.js 14.4.0, and a recent master commit of TypeScript (610fa28). The average of 3 runs before and after this change were taken. I would normally include .cpuprofile and isolate-*-*-*.log files, but can't post them publicly in this case. If there's any other summaries the TypeScript team would be curious about I can report them. ## fs.statSync Misses Within our monorepo, the fs.statSync misses were mostly searches for alternative file extensions of module imports. - For node_modules imports, a lot of .ts/.tsx lookups failed until the .d.ts file was found. - Within projects with a lot of JSX files, .ts files were looked for before finding the .tsx version. - In the medium scale project mentioned above, a total of 38,515 non-existent files were queried during createProgram.
|
It looks like the node12 and node14 tests timed out cloning this PR. Pushing an amended commit to restart CI. |
gluxon
force-pushed
the
uvException-hotspot
branch
from
1df3b49
to
930b81c
Compare
Awesome. Thanks to you and @sheetalkamat for the fast reviews! |
|
@typescript-bot perf test this |
|
Heya @sheetalkamat, I've started to run the perf test suite on this PR at 930b81c. You can monitor the build here. Update: The results are in! |
|
@sheetalkamat Here they are:Comparison Report - master..40043
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Would it be possible to backport this to the 4.0 branch as well, once merged? My team is eager to get this performance improvement as soon as possible. |
|
@ericjeney What kind of improvement are you seeing? When I was investigating (before this PR), it looked like it was in the 5% range, which is certainly very nice, but is not hold-the-release. Having data from real customers would make it easier to argue for a late change. |
|
@gluxon and I are on the same team, so my results are similar to his. We work on a code base of ~1,500,000 lines of TypeScript spread across ~500 packages. In addition to the benchmarks he noted above, I tested on a patched version of If not in 4.0, is this the sort of change you would ship in a patch release? Or are patch releases reserved for regressions? |
|
@ericjeney We'll have to discuss the potential risks, but those numbers are pretty exciting. I'll post more when I know more. |
|
Thank you! I just confirmed with another coworker as well, who works on a separate monorepo, and saw a >25% improvement on one of their packages (17.5s => 13s), so the improvement seems to not be isolated to only the codebase that I work on. |
|
@ericjeney If things look good in nightly builds, this will be a strong candidate for inclusion in a patch release. No promises though. |
|
Thanks for the update @amcasey! Is there anything I still need to do on our side before this can merge? |
There was a problem hiding this comment.
If it matters this much, TBH, we should consider doing this for all the falliable FS operations in sys.ts that we just eat the error on - realpath, getModifiedTime/setModifiedTime, deleteFile, createDirectory, getFileSize, writeFile, getAccessibleFileSystemEntries, fileSystemEntryExists, and watchPresentFileSystemEntry (all of them combined will probably also represent measurable savings under --build or --watch)
Simultaneously, a gripe: Why isn't Error.prototype.stack a lazier getter 😦 I feel like this could also be "fixed" in node.
|
@gluxon, nope, just wanted to hear from @sheetalkamat. 😄 Thanks again! |
|
Thanks @amcasey / @weswigham / @sheetalkamat ! |
|
@typescript-bot cherry-pick this to release-4.0 |
|
Hey @amcasey, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.0 manually. |
The exception thrown by Node.js's fs.statSync function contains a stack trace that can be expensive to compute. Since this exception isn't used by fileSystemEntryExists, we can safely set Error.stackTraceLimit to 0 without a change in behavior.
A significant performance improvement was noticed with this change while profiling tsserver on packages within a proprietary monorepo. Specifically, my team saw high self time percentages for Node.js's
uvExceptionandhandleErrorFromBindinginternal functions. These functions are executed withinfs.statSyncwhen it fails to find the given path.fs.statSync: https://github.com/nodejs/node/blob/v14.4.0/lib/fs.js#L1030-L1037handleErrorFromBinding: https://github.com/nodejs/node/blob/v14.4.0/lib/internal/fs/utils.js#L254-L269uvException: https://github.com/nodejs/node/blob/v14.4.0/lib/internal/errors.js#L390-L443Measurements
After adding
Error.stackTraceLimit = 0, we saw:projectLoadingFinishevent 48.78% faster. (~46.786s vs ~31.447s)Measurements were taken on macOS 10.15.6, Node.js 14.4.0, and a recent master commit of TypeScript (
610fa28d). The average of 3 runs before and after this change were taken.I would normally include
.cpuprofileandisolate-*-*-*.logfiles, but can't post them publicly in this case. If there's any other summaries the TypeScript team would be curious about I can report them.fs.statSync Misses
Within our monorepo, the
fs.statSyncmisses were mostly searches for alternative file extensions of module imports..ts/.tsxlookups failed until the.d.tsfile was found..tsfiles were looked for before finding the.tsxversion.createProgram.