Skip to content
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

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

dragomirtitian
Copy link
Contributor

@dragomirtitian dragomirtitian commented Dec 15, 2023

Improvements to reduce fs calls during test discovery:

  1. Use opendirSync to read the directory content. This reduces the need to call fs.existsSync and fs.statSync
  2. Cache enumerateTestFiles result to avoid having to enumerating the files again
  3. Delay creation of vfs for project tests (as creating the vfs also mounts real folders which also touch the file system)

Improvements in test discovery time (anecdotal):

  1. Linux node 18 debug - ~1.4 seconds -> ~800ms
  2. Windows 10 node 20 debug - 8s -> 2s

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

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Dec 15, 2023
src/harness/harnessIO.ts Outdated Show resolved Hide resolved
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 2, 2024
@jakebailey
Copy link
Member

Looks good, though there's still an open thread about another function that could use withFileTypes.

That being said, I also don't see as great of an improvement as your benchmark; before:

Benchmark 1: hereby runtests -t "notarealtest" --no-typecheck --no-lint
  Time (mean ± σ):      2.265 s ±  0.012 s    [User: 2.540 s, System: 0.652 s]
  Range (min … max):    2.250 s …  2.286 s    10 runs

After:

Benchmark 1: hereby runtests -t "notarealtest" --no-typecheck --no-lint
  Time (mean ± σ):      2.093 s ±  0.039 s    [User: 2.351 s, System: 0.559 s]
  Range (min … max):    2.035 s …  2.155 s    10 runs

8% faster is still good, but you had seen 75% faster...

Copy link
Member

@sheetalkamat sheetalkamat left a 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

@dragomirtitian
Copy link
Contributor Author

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 getAccessibleFileSystemEntries is not exposed in sys. I exposed it as internal. hope that is ok. I rewrote listFiles to use the sys version and also removed the local implementation of getAccessibleFileSystemEntries

@@ -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!,
Copy link
Member

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.

@jakebailey jakebailey merged commit 9a0c7b1 into microsoft:main Jan 11, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Improve test discovery speed
6 participants