perf: cached fs utils #15279
perf: cached fs utils #15279
Conversation
|
|
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI on
|
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI on
|
|
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 |
|
Some details about the updates:
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: |
|
@sapphi-red tested in his Windows machine:
|
There was a problem hiding this comment.
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.
| // 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
Could you expand on the
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. |
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 |
packages/vite/src/node/fsUtils.ts
Outdated
| } | ||
|
|
||
| export function createCachedFsUtils(config: ResolvedConfig): FsUtils { | ||
| const root = withoutTrailingSlash(config.root) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I confirmed that path.resolve(variable) and process.cwd() both always return without a trailing slash on Windows 👍
There was a problem hiding this comment.
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.
| cachedChecks: | ||
| server.fs?.cachedChecks ?? !!process.env.VITE_SERVER_FS_CACHED_CHECKS, |
There was a problem hiding this comment.
Are we planning to keep this env var, or is it only for testing?
There was a problem hiding this comment.
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.
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
Description
Note
To enable this optimization in vite@5.0.8+ set in your vite config:
Or toggle the
VITE_SERVER_FS_CACHED_CHECKSenv 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: trueis set.I tested it with https://github.com/yyx990803/vite-dev-server-perf and
resolveIdtimes 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
Cached: 19ms
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.nativeandtryResolveRealFileWithExtensions.We heavily optimized resolving during Vite 4.3 (see blog post with explanation). We have tried removing
realpathcalls 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 usingreaddirandwithFileTypes: trueso for eachdirentwe know if it is adirectory,file, orsymlink(could be either). We expand all the dirents in the path on each fs check except for the symlinks. So apart from avoidingfs.existsSync,fs.statSync, etc, we can also avoid calls torealpathfor 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
watchisnullor if there is a customwatch.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?