-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Wrap all imported fs methods to set error stack trace limit to zero during their execution #40125
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1125,6 +1125,26 @@ namespace ts { | |
return parseInt(version.substring(1, dot)); | ||
} | ||
|
||
function wrapStacktraceReduction(mod: any): any { | ||
const result = {} as any; | ||
for (const key in mod) { | ||
if (!(mod[key] instanceof Function)) continue; | ||
result[key] = (...args: any[]) => { | ||
// Since the error thrown by fs.statSync isn't used, we can avoid collecting a stack trace to improve | ||
// the CPU time performance. | ||
const originalStackTraceLimit = Error.stackTraceLimit; | ||
Error.stackTraceLimit = 0; | ||
try { | ||
return mod[key](...args); | ||
} | ||
finally { | ||
Error.stackTraceLimit = originalStackTraceLimit; | ||
} | ||
}; | ||
} | ||
return result; | ||
} | ||
|
||
// TODO: GH#18217 this is used as if it's certainly defined in many places. | ||
// eslint-disable-next-line prefer-const | ||
export let sys: System = (() => { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, fair enough, I just didn't realize other things used |
||
const _path: typeof import("path") = require("path"); | ||
const _os = require("os"); | ||
// crypto can be absent on reduced node installations | ||
|
@@ -1662,11 +1682,6 @@ namespace ts { | |
} | ||
|
||
function fileSystemEntryExists(path: string, entryKind: FileSystemEntryKind): boolean { | ||
// Since the error thrown by fs.statSync isn't used, we can avoid collecting a stack trace to improve | ||
// the CPU time performance. | ||
const originalStackTraceLimit = Error.stackTraceLimit; | ||
Error.stackTraceLimit = 0; | ||
|
||
try { | ||
const stat = _fs.statSync(path); | ||
switch (entryKind) { | ||
|
@@ -1678,9 +1693,6 @@ namespace ts { | |
catch (e) { | ||
return false; | ||
} | ||
finally { | ||
Error.stackTraceLimit = originalStackTraceLimit; | ||
} | ||
} | ||
|
||
function fileExists(path: string): boolean { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth understanding whether an allocation for this array in each call is a potential problem.