Uh oh!
There was an error while loading. Please reload this page.
Wrap all imported fs methods to set error stack trace limit to zero during their execution - #40125
Closed
Wesley Wigham (weswigham) wants to merge 1 commit into
Closed
Conversation
…uring their execution
Wesley Wigham (weswigham)
commented
Aug 19, 2020
MemberAuthor
TypeScript Bot (@typescript-bot) perf test this |
Contributor
Heya Wesley Wigham (@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! |
Contributor
Heya Wesley Wigham (@weswigham), I've started to run the tarball bundle task on this PR at 82dcd62. You can monitor the build here. |
Wesley Wigham (weswigham)
requested review from
Andrew Casey (amcasey) and Sheetal Nandi (sheetalkamat)August 19, 2020 00:32
Contributor
Hey Wesley Wigham (@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.
TypeScript Bot (typescript-bot)
commented
Aug 19, 2020
Contributor
Wesley Wigham (@weswigham) Here they are:Comparison Report - master..40125
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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)
MemberAuthor
There was a problem hiding this comment.
Hm, fair enough, I just didn't realize other things used fs directly rather than sys.
Andrew Casey (amcasey)
commented
Aug 20, 2020
Member
Is it really the case that we never want a stack from |
Wesley Wigham (weswigham)
commented
Aug 20, 2020
MemberAuthor
We never inspect the |
Ryan Cavanaugh (RyanCavanaugh)
commented
Aug 20, 2020
Member
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 |
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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
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.