Wrap all imported fs methods to set error stack trace limit to zero during their execution #40125
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uring their execution
|
@typescript-bot perf test this |
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Heya @weswigham, I've started to run the perf test suite on this PR at 82dcd62. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 82dcd62. You can monitor the build here. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
| const result = {} as any; | ||
| for (const key in mod) { | ||
| if (!(mod[key] instanceof Function)) continue; | ||
| result[key] = (...args: any[]) => { |
There was a problem hiding this comment.
Worth understanding whether an allocation for this array in each call is a potential problem.
|
@weswigham Here they are:Comparison Report - master..40125
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -1135,7 +1155,7 @@ namespace ts { | |||
|
|
|||
| function getNodeSystem(): System { | |||
| const nativePattern = /^native |^\([^)]+\)$|^(internal[\\/]|[a-zA-Z0-9_\s]+(\.js)?$)/; | |||
| const _fs: typeof import("fs") = require("fs"); | |||
| const _fs: typeof import("fs") = wrapStacktraceReduction(require("fs")); | |||
There was a problem hiding this comment.
if we are doing this in sys for all functions, we should do it for all the other uses of fs too.. (nodeTypingInstaller, server etc)
There was a problem hiding this comment.
Hm, fair enough, I just didn't realize other things used fs directly rather than sys.
|
Is it really the case that we never want a stack from |
|
We never inspect the |
|
This doesn't seem like a great win for perf overall. We're introducing this overhead on every fs call, even though there's only a small number that regularly throw, and also introducing a duplicative |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is just a followup to #40043 that blanket applies the idea to all
fsmethods we use (or will use in the future), rather than just one specificstatSyncusage.