-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Reduce file system calls during test setup. #56791
Conversation
fa895b0
to
1e4e9a6
Compare
Looks good, though there's still an open thread about another function that could use That being said, I also don't see as great of an improvement as your benchmark; before:
After:
8% faster is still good, but you had seen 75% faster... |
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.
This looks ok to me except @jakebailey s suggestion on just replacing listFiles
and getAccessibleFileSystemEntries
to use ts.sys.getAccessibleFileSystemEntries
@sheetalkamat One small problem is that |
167d81d
to
a950eeb
Compare
@@ -170,7 +143,7 @@ function createNodeIO(): IO { | |||
getWorkspaceRoot: () => workspaceRoot, | |||
exit: exitCode => ts.sys.exit(exitCode), | |||
readDirectory: (path, extension, exclude, include, depth) => ts.sys.readDirectory(path, extension, exclude, include, depth), | |||
getAccessibleFileSystemEntries, | |||
getAccessibleFileSystemEntries: ts.sys.getAccessibleFileSystemEntries!, |
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.
I don't think that it matters, but every other function in this list is an arrow function into sys
, not the actual value.
Improvements to reduce fs calls during test discovery:
opendirSync
to read the directory content. This reduces the need to callfs.existsSync
andfs.statSync
enumerateTestFiles
result to avoid having to enumerating the files againImprovements in test discovery time (anecdotal):
Performance of running all the tests does not appear to be impacted as the cost of test discovery is not a big percentage of running all tests and the work that si delayed will have to eventually be executed anyway.
Fixes #56790