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

perf: cached fs utils #15279

Merged
merged 24 commits into from
Dec 12, 2023
Merged

perf: cached fs utils #15279

merged 24 commits into from
Dec 12, 2023

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Dec 7, 2023

Description

Note

To enable this optimization in vite@5.0.8+ set in your vite config:

  server: {
    fs: {
      cachedChecks: true
    }
  }

Or toggle the VITE_SERVER_FS_CACHED_CHECKS env variable.

Tip

To see the difference in resolveId, you can run the dev server using --profile, then press > P and Enter when the loading is done. A cpuprofile will be created. You can upload it and share it here if you think it would provide valuable feedback to decide if we should enable this feature by default

Implements a cached strategy for fs checks when the experimental option server.fs.cachedChecks: true is set.

I tested it with https://github.com/yyx990803/vite-dev-server-perf and resolveId times are cut in half. Also tested with the turbopack triangle benchmark using https://github.com/sapphi-red/performance-compare with the same results on my M1 pro.

An example profile diff for tryResolveRealFileWithExtensions (65ms -> 19ms)

Base: 65ms
image

Cached: 19ms
image

Context

We received a CPU profile for server start on a large app that takes a lot of time on Windows old machines, and 40% of the server time was spent resolving ids. In particular in fs.realPathSync.native and tryResolveRealFileWithExtensions.

We heavily optimized resolving during Vite 4.3 (see blog post with explanation). We have tried removing realpath calls at least for some cases before, for example, in the closed #12818.

We tried to avoid caching fs checks so far, but we changed that for the public dir in:

Details

This PR implements a cache for watched files in root (not symlinked). We construct the tree lazily using readdir and withFileTypes: true so for each dirent we know if it is a directory, file, or symlink (could be either). We expand all the dirents in the path on each fs check except for the symlinks. So apart from avoiding fs.existsSync, fs.statSync, etc, we can also avoid calls to realpath for each path we cached in the tree because we know there aren't any symlinks in their path.

The cached checks only run during dev because we need a watcher to add/unlink files and directories. For now, the optimization is disabled if watch is null or if there is a custom watch.ignore. We can later relax this restriction.

We discussed with @sheremet-va, and for Vitest to be able to use this optimization, we need to compose all the cached fs trees into a single one so several servers can share the cache of overlapping roots (important for monorepos). We can work on this in a future PR.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Dec 7, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev added the performance Performance related enhancement label Dec 7, 2023
@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 524a52a: Open

suite result latest scheduled
analogjs success success
astro success success
histoire success success
ladle success success
laravel failure failure
marko success success
nuxt failure failure
nx success success
previewjs success success
qwik success success
rakkas success success
sveltekit success success
unocss success success
vike success success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress failure success
vitest success success

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@patak-dev patak-dev marked this pull request as ready for review December 7, 2023 17:22
@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on c929f96: Open

suite result latest scheduled
analogjs success success
astro success success
histoire success success
ladle success success
laravel failure failure
marko success success
nuxt failure failure
nx success success
previewjs success success
qwik success success
rakkas success success
sveltekit success success
unocss success success
vike success success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest success success

@patak-dev
Copy link
Member Author

Watching the addition of directories and files is more performant now after a343c66, we don't drop the parent dir dirents and instead add the entry as directory_maybe_symlink or file_maybe_symlink and we resolve it with a lstat lazily if it is ever accessed. We also need this if we want to share the dirents cache for different ResolvedConfigs.

@patak-dev
Copy link
Member Author

Some details about the updates:

  • In feat: optimize tryResolveRealFileWithExtensions we avoid many normalizePath and search in the same cached fs tree calls. We could go further and optimize like this the whole tryFsResolve. We can try that in another PR, I think we need better abstractions around handling the DirentCache tree first.
  • In feat: avoid double normalizePath calls we avoid a few extra normalizePath calls. I saw in profiles that this is starting to take most of the time. If we abstract tryFsResolve we could do a single normalizePath for the whole tree. One issue I see is that I don't know if the fallback fs calls are slower when you pass them normalized paths so we should have both the fs native and the normalized path around? Something else to explore later.
  • There were fails at one point that I thought were related to Vitest having a custom watch.ignore. I pushed chore: remove watch null and custom ignored guard, and I no longer see these issues. I think we should merge the PR without this guard so more projects can test it out. We still need to review how watch.ignore and watch: null should affect the fs cache
  • In chore: remove async wip code, I remove some code that was initially intended to let us do async readdir calls. For now, the fs cache is sync and I don't know if we should change that. The readdirSync time is very low compared to everything else. I moved everything to async calls here perf: async fs calls in resolve and package handling #15211 and we didn't see a definite gain either. Maybe a better approach is to keep the tree sync only but auto-expand it in the background doing async readdir calls and assigning the result in sync.

There is also now a version sharing the fs trees, to improve Vitest in monorepo setups. I think we could review and merge that one later:

@patak-dev
Copy link
Member Author

@sapphi-red tested in his Windows machine:

Results with performance-compare on my Windows laptop. It's a 5% improvement

Before: 4202.2ms (babel), 2666.0ms (swc)
After: 4046.8ms (babel), 2491.2ms (swc)

The results with https://github.com/yyx990803/vite-dev-server-perf.
Before
1000 TS modules (50x20) loaded in: 1183ms (runs: [1254,1094,1183,1185,1152])
1000 TS modules (20x50) loaded in: 1054ms (runs: [1126,1075,1052,1035,1054])
2000 TS modules (100x20) loaded in: 2556ms (runs: [2206,2556,2529,2559,2612])
5000 TS modules (250x20) loaded in: 6398ms (runs: [6491,6396,6473,6398,6190])
10000 TS modules (400x25) loaded in: 12524ms (runs: [12954,13004,12437,12437,12524])

After
1000 TS modules (50x20) loaded in: 1067ms (runs: [1130,1020,1012,1076,1067])
1000 TS modules (20x50) loaded in: 1025ms (runs: [1017,1054,1019,1025,1081])
2000 TS modules (100x20) loaded in: 2280ms (runs: [2220,2308,2280,2198,2313])
5000 TS modules (250x20) loaded in: 6253ms (runs: [6282,6253,6023,6131,6267])
10000 TS modules (400x25) loaded in: 12132ms (runs: [12365,12132,12003,11987,12477])

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my main concern so far is the graph fs eagerness, and that we're still doing a bit of cache-check work for existsSync (and others), that maybe it isn't as bad is we swap it with a isPathWatched check instead?

I don't think they're blockers to get this experiment out for testing, but hoping we don't have to change to much if we decide to go with a different approach.

About #15294, I'm not really comfortable with doing that and I think we should avoid it 😅 Feels a bit going too far.

packages/vite/src/node/server/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/fsUtils.ts Show resolved Hide resolved
// cached fsUtils is only used in the dev server for now, and only when the watcher isn't configured
// we can support custom ignored patterns later
fsUtils = commonFsUtils
} /* TODO: Enabling for testing, we need to review if this guard is needed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be fine uncommenting this to add a guard, but if we do stablize the feature, I think we should figure out getting ignored to work.

If we do support ignored, the flow could maybe be more direct, in that we can cache the fs function calls directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may not need this guard at the end. If the user is ignoring a certain folder or using watch: null, I think we could say that he needs to restart the server when adding or removing files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm thinking about it, I don't think we invalidate the module graph for files that aren't watched, so a reload wouldn't be useful either. So maybe your idea is fine 🤔

@patak-dev
Copy link
Member Author

I think my main concern so far is the graph fs eagerness, and that we're still doing a bit of cache-check work for existsSync (and others), that maybe it isn't as bad is we swap it with a isPathWatched check instead?

Could you expand on the isPathWatched idea? About the graph fs eagerness, see #15279 (comment)

About #15294, I'm not really comfortable with doing that and I think we should avoid it 😅 Feels a bit going too far.

I think it will depend on the results that @sheremet-va sees with Vitest. If it speeds up things a lot, I don't think the extra complexity is that bad.

@patak-dev
Copy link
Member Author

  • There were fails at one point that I thought were related to Vitest having a custom watch.ignore. I pushed chore: remove watch null and custom ignored guard, and I no longer see these issues. I think we should merge the PR without this guard so more projects can test it out. We still need to review how watch.ignore and watch: null should affect the fs cache

Oh... I accidentally enabled the flag by default in a98c9b4#diff-abb3345b6e3b2ec6d297c2ebc54cca85ae4487a31bac3cc9e78457f5114adb26L955, and I see this fail again. The problem is that a file is generated in outDir and I think that isn't being watched. I think we can still release without the guard so we get more feedback.

}

export function createCachedFsUtils(config: ResolvedConfig): FsUtils {
const root = withoutTrailingSlash(config.root)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I added withoutTrailingSlash because the rest of the cache expects this. I think config.root is always without a trailing slash because we do a path.resolve(config.root) : path.cwd(). At least in UNIX, these two always return without a trailing slash. @sapphi-red could you confirm this is also the case in Windows? We could add withoutTrailingSlash in another PR if not when resolving the root, then there are some places where we could directly use root + pathInRoot instead of a path.join() call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that path.resolve(variable) and process.cwd() both always return without a trailing slash on Windows 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing! I think I will remove the new withoutTrailingSlash function then so we can take any testing to also check this more in general.

ArnaudBarre
ArnaudBarre previously approved these changes Dec 10, 2023
Copy link
Member

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is risky but way less invasive that I would have think!

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small nits but I think it's worth a try. Thanks for encapsulating the new fs utils all in one-file, that makes it easier to revisit the optimizations in the future.

Comment on lines +947 to +948
cachedChecks:
server.fs?.cachedChecks ?? !!process.env.VITE_SERVER_FS_CACHED_CHECKS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we planning to keep this env var, or is it only for testing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it only for help with testing, for example if @sheremet-va would want to easily check the effect in Vitest. I think we could remove it later.

packages/vite/src/node/fsUtils.ts Outdated Show resolved Hide resolved
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-dev patak-dev merged commit c9b61c4 into main Dec 12, 2023
15 checks passed
@patak-dev patak-dev deleted the perf/in-memory-fs-checks branch December 12, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants